Diagnostic configuration mechanism?

Topics: General
Apr 10, 2014 at 3:37 PM
Is there a standardized mechanism planned for configuration of individual diagnostics down to the project level? It looks like the configurable diagnostics that have been developed so far rely mainly on VS options, which isn't nearly granular enough for the kinds of rules that folks are likely to want to create. e.g.:
  1. Allowed whitespace character for a consistent leading whitespace diagnostic,
  2. Threshold value for a maximum cyclomatic complexity diagnostic, or
  3. "Keep anyway" list for an unused using statement diagnostic.
Is there an existing or planned configuration mechanism that would allow these sorts of settings to be configured at a project (+/- solution) level?
Developer
Apr 10, 2014 at 8:56 PM
Yes we've talked about the need for a configuration mechanism but haven't really settled on a design for this yet. This is on our backlog of items to address soon.
Apr 13, 2014 at 3:39 AM
Good to know. If you're open to suggestions, here's what I'd love to see (based on quite a bit of experience authoring and consuming configurable rules for both FxCop and StyleCop):
  1. Use the ruleset as the configuration store. I realize that the effective ruleset logic based on includes makes this rather tricky, but managing the rule-specific settings separately from the rule enabling/disabling and violation levels is a giant PITA for end users. It's really worth spending the time to figure out a way to get this to work, even if it's something relatively clunky like adding a "priority" attribute for the settings element and failing the ruleset validation if there's a tie for any given rule.
  2. __Base configuration on an IConfigurableAnalyzer<TConfiguration> interface.__ The contents of the setting node in the ruleset file would simply be the XML-serialized version of TConfiguration.
  3. If you're going to create a UI for this, expose a property-sheet style UI by default. Allow rule authors to opt into overriding this default UI via a custom UI if necessary/desirable (say, via an attribute on the configuration class).
To make this a bit more concrete, here's what things might look like if we're implementing a CA1502 replacement that has customizable warning and error thresholds:

Rule element in ruleset:
<Rule Id="CA1502" Action="Error">
    <Settings Priority="1">
        <WarningThreshold>10</WarningThreshold>
        <ErrorThreshold>20</ErrorThreshold>
    </Settings>
</Rule>
Diagnostic analyzer class:
public sealed class AvoidExcessiveComplexity : ConfigurableAnalyzer<AvoidExcessiveComplexityConfiguration>, IDiagnosticAnalyzer // (or whichever descendant actually makes sense for this rule)
{
    public AvoidExcessiveComplexity()
        : base(new AvoidExcessiveComplexityConfiguration())
    {
    }
    
    //…
}

Configuration class:
[DataContract(Namespace = "")]
public sealed class AvoidExcessiveComplexityConfiguration : IValidatableObject
{
    public AvoidExcessiveComplexityConfiguration()
    {
        this.WarningThreshold = 10;
        this.ErrorThreshold = 20;
    }

    [DataMember(Order = 1)]
    [Range(0, int.MaxValue)]
    public int WarningThreshold { get; private set; }

    [DataMember(Order = 2)]
    [Range(0, int.MaxValue)]
    public int ErrorThreshold { get; private set; }

    IEnumerable<ValidationResult> IValidatableObject.Validate(ValidationContext validationContext)
    {
        if (this.WarningThreshold > this.ErrorThreshold)
        {
            yield return new ValidationResult("The error threshold should be greater or equal to the warning threshold.");
        }

    }
}
Configurable analyzer base class (maybe in the framework, maybe not):
public abstract class ConfigurableAnalyzer<TConfiguration> : IConfigurableAnalyzer<TConfiguration>
{
    private readonly TConfiguration _defaultConfiguration;
    private TConfiguration _currentConfiguration;

    protected ConfigurableAnalyzer(TConfiguration defaultConfiguration)
    {
        if (defaultConfiguration == null)
        {
            throw new ArgumentNullException("defaultConfiguration");
        }

        this._defaultConfiguration = defaultConfiguration;
        this.CurrentConfiguration = defaultConfiguration;
    }

    public TConfiguration DefaultConfiguration
    {
        get
        {
            return this._defaultConfiguration;
        }
    }

    public TConfiguration CurrentConfiguration
    {
        get
        {
            return this._currentConfiguration;
        }

        set
        {
            if (value == null)
            {
                throw new ArgumentNullException("value");
            }

            var validationResults = new List<ValidationResult>();
            if (!Validator.TryValidateObject(value, new ValidationContext(value), validationResults, true))
            {
                throw new InvalidRuleSetException(
                    string.Format(CultureInfo.InvariantCulture, "Rule settings validation failed for {0}.", this.GetType().FullName),
                    new ValidationException(validationResults.First(), null, value));
            }

            this._currentConfiguration = value;
        }
    }
}
Configurable analyzer interface (framework-defined):
public interface IConfigurableAnalyzer<TConfiguration>
{
    TConfiguration DefaultConfiguration { get; }  // Only needed for UI support.

    TConfiguration CurrentConfiguration { get; set; }
}
If you don't like the idea of adopting DataContractSerializer in general for the settings, please at least consider supporting it, either via detection of the DataContractAttribute on the config class, or by allowing the analyzer author to provide an implementation of a serializer adapter interface. Many of us nit-picky folks who find value in authoring custom rules also happen to prefer authoring immutable rule configuration classes, which wouldn't be possible (at least without a bunch of extra work) if the only supported serialization mechanism used XmlSerializer with its reliance on public setters.
Apr 14, 2014 at 7:48 PM
I forgot to mention that I do understand that the above would only work cleanly for single-diagnostic analyzers. I can see three basic choices for the more "meta" configuration design question:
  1. Configure at the analyzer level instead of the diagnostic level,
  2. Configure at the diagnostic level and have the framework pass configs for all exposed diagnostics to a configurable analyzer (e.g.: via a dictionary where the key is the diagnostic ID and the value is the config), or
  3. Configure at the diagnostic level and only support single-diagnostic configurable analyzers.
I'd recommend avoiding #1 since analyzers are internal implementation mechanisms of which diagnostic consumers should be able to remain blissfully unaware. If a diagnostic author decides to split a diagnostic into its own analyzer in a new release, the diagnostic consumers shouldn't have to change a thing about their pre-existing configurations.

That leaves #2 vs #3… Based on what I've seen for StyleCop, which is based on a similar design with analyzers that can include multiple rules, I suspect that custom diagnostic developers won't be particularly interested in creating multi-diagnostic analyzers. However, if you feel that the flexibility of #2 is worth supporting, it would be great if the mechanism at least allowed a custom diagnostic author to set up a convenient, strongly typed mechanism like the above via a custom base class. This would be possible if the framework-defined interface were changed to something like the following:
public interface IConfigurableAnalyzer
{
    IReadOnlyDictionary<string, Type> DiagnosticConfigurationTypes { get; }  // Allows analyzer to specify the types of the configs for deserialization, which won't necessarily match the default types.

    IReadOnlyDictionary<string, object> DefaultDiagnosticConfigurations { get; }  // Only needed for UI support.

    IReadOnlyDictionary<string, object> CurrentDiagnosticConfigurations { get; set; }
}
The types retriever could be omitted if DiagnosticDescriptor were extended to allow specification of a configuration type. Also, if you don't like the idea of deserializing the configs from within the framework, something like the following would at least allow custom diagnostic developers to build it into their own framework overlay:
public interface IConfigurableAnalyzer
{
    IReadOnlyDictionary<string, XmlElement> DefaultDiagnosticConfigurations { get; }  // Only needed for UI support.

    IReadOnlyDictionary<string, XmlElement> CurrentDiagnosticConfigurations { get; set; }
}
Developer
Apr 16, 2014 at 5:33 PM
Thanks, this is great feedback! Some of our thinking has been inline with what you say above although we haven't thought about serialization much. We'll definitely consider that when we get to designing the mechanism. When we do discuss it, I'll post our thoughts here.
Apr 16, 2014 at 7:02 PM
Thanks -- looking forward to it!