Roslyn: Bulk configuration/suppression of analyzer diagnostics

[mavasani]

User scenarios

Lot of internal discussions and external requests in the area of analyzer adoption have pointed to a need for a better _bulk diagnostic configuration_ story based on real world user scenarios, such as:

1. Default severity configuration to reduce analyzer noise and remove analyzer adoption barriers: I want to configure the default severity of all analyzer diagnostics (from every analyzer package that I have installed in my project) without having to configure every single diagnostic ID. For example, demote all analyzer diagnostics to be _suggestions by default_, so they are equivalent to IDE code style suggestions, which do not affect my CI builds and do not pollute my editor with squiggles and tons of warnings and errors. This breaks the analyzer adoption barrier as installing a new analyzer package has no drastic implications on my workflow. It allows me to opportunistically fix analyzer diagnostics whenever I want to address them and selectively promote individual diagnostic IDs that I want to execute in CI to warning/error severity with explicit diagnostic ID based configuration entries. This allows the end users to choose opt-in vs opt-out model for analyzer execution in CI.
2. Category based configuration to prioritize buckets of analyzer diagnostics: I want to configure all diagnostics with a specific diagnostic category, without having to manually identify and configure every single diagnostic ID in that category. For example, bump up all security rules to errors, demote all performance issues to suggestions, etc. These are extremely common operations during targeted code quality work such as security push, performance push, maintenance push, etc.
3. Suppress all current and future analyzer diagnostics inside a specific file or folder: say legacy code folders or documents based on extensions (for example, generated code with special extensions: https://github.com/dotnet/roslyn/issues/3705), etc. This issue has long been known as one of the biggest analyzer adoption barrier that needs an end user configuration point to reduce analyzer noise. Requiring end users to add custom autogenerated comments at top of their generated code files may not always be feasible or scalable and is also an unnecessary burden for customers. Performing a baseline operation in Visual Studio (Build and suppress active issues command in Analyze menu) only suppresses diagnostics from current project/solution snapshot and also generates a very hard to maintain list of in-source suppressions (pragmas or suppress message attributes).

Difficulties with current support

Each of the above actions are very common when installing a new analyzer package, or doing targeted code analysis cleanup work (for example, security push, performance push, code maintenance push, etc.). Current support for diagnostic configuration requires user to configure each diagnostic ID with a distinct configuration entry per diagnostic ID reported by every analyzer from all of the consumed analyzer packages. This approach becomes even more untenable with analyzer package upgrades, which generally brings in bunch of new analyzers and diagnostics IDs and requires the user to manually update their per-ID configuration lists to include the new diagnostic IDs.

Design Proposal - editorconfig options for analyzer driver

In past, adding any such bulk diagnostic configuration support would have required providing new MSBuild and command line compiler switches or updating existing ones, which had a high implementation and design cost and higher risk of bugs/regressions. With the recently added support in VS2019 16.3 to pass in project's .editorconfig files into the compiler for analyzer configuration, implementing this feature has become considerably easier.

Proposal is to _allow end users to specify editorconfig options that are respected by the core analyzer driver in the compiler layer_. Proposed formats for analyzer driver options:

# Format 1 - configure default severity for all analyzer diagnostics
dotnet_analyzer_diagnostic.severity = error | warning | suggestion | silent | none

# Format 2 - configure default severity for analyzer diagnostics with a specific 'diagnostic_category'
dotnet_analyzer_diagnostic.<%diagnostic_category%>.severity = error | warning | suggestion | silent | none

# Format 3 - suppress all/category-based analyzer diagnostics for specific files/folders
dotnet_analyzer_diagnostic.suppress = true | false
dotnet_analyzer_diagnostic.<%diagnostic_category%>.suppress = true | false

Using the above supported formats, the user scenarios listed in this issue can be addressed with following simple configuration entries:

# Scenario 1 - switching all analyzer diagnostics to be suggestions by default, and promoting individual diagnostics to warning/error
[*.cs]
dotnet_analyzer_diagnostic.severity = suggestion
dotnet_diagnostic.CA1801.severity = warning
dotnet_diagnostic.CA1802.severity = error

# Scenario 2 - category based diagnostic configuration
[*.cs]
dotnet_analyzer_diagnostic.security.severity = error
dotnet_analyzer_diagnostic.correctness.severity = warning
dotnet_analyzer_diagnostic.performance.severity = suggestion

# Scenario 3 - suppress all analyzer diagnostics, except security ones, on my generated code
[*.MyGeneratedFileExtension.cs]
dotnet_analyzer_diagnostic.suppress = true
dotnet_analyzer_diagnostic.security.suppress = false

Possible future enhancements:

1. Allow bulk configuration of diagnostics from specific analyzer package or for all diagnostics with a specific diagnostic prefix, say dotnet_analyzer_diagnostic.CA.severity = ... would only configure diagnostics with a CA prefix. I suggest we don't add such a functionality until it is required and we can provide specific user scenarios where the other proposed formats are not sufficient.

[ivanbasov]

@mavasani should we consider using project files for folder / file specific purposes?

[mavasani]

should we consider using project files for folder / file specific purposes?

IMO, that would be a separate and much broader feature request - ability to tag folders and/or files with special attributes (such as generated code etc.). If such a feature were implemented and this metadata information somehow threaded into the compiler and hence the analyzer driver, it could use this data to implicitly suppress diagnostics in generated code. However, the proposal here is scoped just to analyzer configuration, hence IMO much more suitable for analyzer config (editorconfig) files. Additionally, note that the scenarios here are much broader - for example, user may want to suppress analyzer diagnostics in a files/folders that are "legacy code", but may be not necessarily want to have this defined in a project file.

[jaredpar]

@agocke

[agocke]

I'm not sure I understand how this works. What if I have the following?

[*.cs]
dotnet_analyzer_diagnostic.severity = suggestion

[*morespecific*.cs]
dotnet_analyzer_diagnostic.severity = error

There's a seeming contradiction in terms between "default severity for the whole driver" and "default severity for a set of files"

[mavasani]

When analyzer driver attempts to fetch the option value with key = dotnet_analyzer_diagnostic.severity for diagnostic's source tree, it should give the appropriate value. So, if diagnostic is flagged on a file morespecific.cs, then it will be an error and for rest of the files that do not match the regex it will be suggestion. Isn't that the same behavior if any analyzer queries for the option with name dotnet_analyzer_diagnostic.severity for your example?

[agocke]

Ah, that makes sense. This configuration isn't supported by the compiler API, so it would only be for analyzer diagnostics, right?

[mavasani]

Ah, that makes sense. This configuration isn't supported by the compiler API, so it would only be for analyzer diagnostics, right?

Yes, I don't think bulk configuration is useful for compiler diagnostics (they all have same category and prefix). IMO the feature is only useful for analyzer diagnostics and the functionality should be implemented in the analyzer driver - most likely a post-processing step here after the compiler's filtering has been applied.

[vatsalyaagrawal]

@agocke please let us know if there is any concern regarding this proposal.

@mavasani can share a draft PR soon.

[agocke]

Let's split some of these things out. I think the dotnet_diagnostic_analyzer.severity string is fine, no objections.

I don't like the dotnet_analyzer_diagnostic.suppress = true syntax. It seems like a duplicate of the severity syntax.

dotnet_analyzer_diagnostic.security.severity = error seems useful, but the deviation from diagnostic ID in the middle is unfortunate. Are there any alternatives for this syntax? Perhaps ones that contain the word "category" somewhere?

[mavasani]

I don't like the dotnet_analyzer_diagnostic.suppress = true syntax. It seems like a duplicate of the severity syntax.

Note that the intent here is different from dotnet_diagnostic_analyzer.severity. Latter is changing the default severity, so that we don't accidentally override explicitly configured diagnostics with dotnet_diagnostic.DiagnosticId.severity = .... For dotnet_analyzer_diagnostic.suppress = true we want to explicitly force override the effective severities of all configured and unconfigured diagnostics to be none to ensure that we don't report any diagnostics.

dotnet_analyzer_diagnostic.security.severity = error

May be dotnet_analyzer_diagnostic.category_security.severity = error, but that mixes a keyword (category) with user supplied value (security, performance, etc.).

[agocke]

For dotnet_analyzer_diagnostic.suppress = true we want to explicitly force override the effective severities of all configured and unconfigured diagnostics to be none to ensure that we don't report any diagnostics.

Then I really don't like it. The general guide for diagnostic configuration in Roslyn is local > general. This would violate that convention by allowing a more general rule (configure all diagnostics) to override a more local rule (configure these specific diagnostics). The user can achieve the same behavior by removing their specific diagnostic configuration.

May be dotnet_analyzer_diagnostic.category_security.severity = error, but that mixes a keyword (category) with user supplied value (security, performance, etc.).

Yeah, I don't love it...

dotnet_analyzer_diagnostic.category.security.severity = error maybe? It's still different from dotnet_diagnostic.id, but at least it's more obviously different?

[agocke]

Actually, we could use category-security. That's not a valid C# identifier, so it even produces a warning if you try to use it in a pragma (#pragma warning disable abc-def). We should probably stay away from _, which is legal in a C# identifier.

[mavasani]

Then I really don't like it. The general guide for diagnostic configuration in Roslyn is local > general. This would violate that convention by allowing a more general rule (configure all diagnostics) to override a more local rule (configure these specific diagnostics). The user can achieve the same behavior by removing their specific diagnostic configuration.

I don’t think that is scalable for this scenario. User has their primary editorconfig at the root of their repo or solution with bunch of dotnet_diagnostic entries specific to each ID. Now they want a simple entry to turn off all diagnostics in their generated code or legacy code sub-folders. With your proposed design, they would need to add an explicit suppression entry for every diagnostic configured in any ancestor editorconfig. I think we should special case this scenario to allow such an overriding suppression entry.

[mavasani]

category-security looks fine to me.

[agocke]

I don’t think that is scalable for this scenario. User has their primary editorconfig at the root of their repo or solution with bunch of dotnet_diagnostic entries specific to each ID. Now they want a simple entry to turn off all diagnostics in their generated code or legacy code sub-folders. With your proposed design, they would need to add an explicit suppression entry for every diagnostic configured in any ancestor editorconfig. I think we should special case this scenario to allow such an overriding suppression entry.

User convenience is less important to me than users being able to see an editorconfig file and know that the most specific configuration wins.

[mavasani]

User convenience is less important to me than users being able to see an editorconfig file and know that the most specific configuration wins.

Sounds reasonable to me. We can always come back and add it in future if there is feedback. Tagging @sharwell to confirm he is fine with this limitation for bulk suppression in generated code.

[mavasani]

@vatsalyaagrawal I think we have consensus on the design for me to start working on the implementation. Thanks @agocke .

[agocke]

Hmm one problem though, this is the doc comment for DiagnosticDescriptor.Category:

The category of the diagnostic (like Design, Naming etc.). For example, for CA1001: "Microsoft.Design"

The presence of a . in the category string would make any editorconfig string problematic.

[mavasani]

I don't think we normally have a . in the category. Even our FxCop analyzer diagnostics use simple category names such as Design, Security, etc. Any case, we will try to find an analyzer option key with name $"diagnostic_analyzer_diagnostic.category_{diagnostic.Category}.severity, so entries such as diagnostic_analyzer_diagnostic.category_A.B.C.severity will work, even if not the most syntactically pleasant.

[agocke]

Yup, that works for now, but makes more . options difficult. If we can discourage this (probably by changing the doc comment) that would be a good idea.

[vatsalyaagrawal]

Thanks a lot @agocke and @mavasani for driving this conversation.

[mavasani]

@agocke We brought up the final design for review in IDE design meeting and felt the agreed upon proposals are reasonable, i.e. avoid the proposed Format 3 (dotnet_analyzer_diagnostic.suppress = true) to prevent confusion with Format 1 (dotnet_analyzer_diagnostic.severity = ...). However, it was proposed that we should add a separate option to explicitly mark files as generated code:

dotnet_analyzer_diagnostic.generated_code = true | false

This will have following implementation semantics:

1. We will modify AnalyzerDriver.IsGeneratedCode to now respect this option. This will allow the end users to override AnalyzerDriver's heuristic to determine if a specific file is generated code or not, without requiring to change their sources or file extensions to match this heuristic. This is a primary request in https://github.com/dotnet/roslyn/issues/3705 that we want to address.
2. We will change the Workspace service IGeneratedCodeRecognitionService to also start respecting this option, so that the IDE features that use this service also behave in a much more user friendly way. This will be done in a separate follow-up PR.

Let us know if this proposal sounds reasonable to you.

[mavasani]

Actually, dotnet_analyzer_diagnostic prefix is probably not the best for generated code option as it is not scoped to just the analyzer diagnostics anymore. Maybe dotnet_generated_code = true | false? @sharwell?

[sharwell]

@mavasani It's somewhat related to editorconfig/editorconfig#399. We could go so far as this:

# The default is 'auto'
generated_code = auto | true | false

[mavasani]

Awesome @sharwell - let’s go with your proposed format.

[mavasani]

Fixed with https://github.com/dotnet/roslyn/pull/38886 in release/dev16.5-preview1