dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.12k stars 4.04k forks source link

Bulk severity configuration in .editorconfig should be applied to "disabled by default" analyzers #47046

Open bordecal opened 4 years ago

bordecal commented 4 years ago

Version Used: Visual Studio 16.7.2

Steps to Reproduce:

  1. Create a solution with an .editorconfig file and a project configured to use at least one Roslyn analyzer package that contains rule(s) that are not enabled by default (Diagnostic.IsEnabledByDefault == false).
  2. Add a bulk configuration for error severity to .editorconfig that should cover one or more of the rules that are not enabled by default (e.g.: dotnet_analyzer_diagnostic.severity = error or dotnet_analyzer_diagnostic.category-<some category that contains a disabled-by-default rule>.severity = error).
  3. Violate at least one of the disabled-by-default rules (e.g.: CA1008) in the project code.
  4. Compile the solution.

Expected Behavior: An error should be generated for each rule violation covered by the bulk configuration.

Actual Behavior: No errors are generated for a "disabled by default" rule covered by the bulk configuration. (Using a rule-specific configuration like dotnet_diagnostic.CA1008.severity = error does, however, result in errors being generated as expected.)

mavasani commented 4 years ago

Also tagging @sharwell.

This is by design and would be a pretty-big breaking change. Analyzers are disabled by default due to varying reasons (high false positive rate for generic code bases, expensive performance wise, desire for analyzer authors to explicitly decide opt-in mode of analysis, etc.) What you possibly need is a new feature request to allow specifying a new configuration entry kind to explicitly apply to disabled by default analyzers.

bordecal commented 4 years ago

@mavasani In that case, a documentation update is probably needed as well since the topic for this feature explicitly mentions that all rules will be affected. The manner in which it explains the precedence of bulk and rule-level configuration also reinforces the message that the bulk configuration is a convenience shortcut to applying the severity to all rules (or all rules in a category). I find it quite difficult to imagine that anyone who reads that section would come away with an expectation that use of the bulk configuration mechanism would have a different result than applying the desired severity to each target rule individually.

mavasani commented 4 years ago

@bordecal I have pushed a PR to add a note to the docs section. It should be live in a day or two.

eatdrinksleepcode commented 3 years ago

I don't understand this design. If someone specifically states that they want to enable all rules in a category, then all rules in the category should be enabled. They already have the ability to disable individual rules if a particular rule causes any issues.

If there is a rule that is so problematic that you think it shouldn't be enabled even when the user has specifically said to enable all rules, and even though they have the ability to disable it individually, then a) maybe it shouldn't be a rule at all, and b) maybe it should be in its own "problematic" category.

If this design can't be changed due to back compat, can we get a dotnet_analyzer_diagnostic.category-<category>-all.severity switch that will actually affect all of the rules in the category?

dnperfors commented 5 months ago

This is a rather annoying issue we encounterd recently. We used the <AnalysisLevelSecurity>latest-all</AnalysisLevelSecurity> (and for other categories as well, with sometimes different settings), but we want to have errors instead of warnings for the security category specifically. Clearly this is not possible by using the dotnet_analyser_diagnostic.categeory-Security.severity = error. Using <CodeAnalysisTreatWarningsAsErrors>true</CodeAnalysisTreatWarningsAsErrors> is not really an option, because most warnings we don't want to be breaking. Overriding all the security rules is also not desirable since that will mean we have to make sure we stay on top of new rules (which is what the latest analysislevel is meant for.

Would it be such a strange idea to get a category specific CodeAnalysisTreatWarningsAsErrors option in the project file so that we at least have a way of doing this?