Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bordecal opened this issue Aug 21, 2020 · 5 comments
Labels
Area-Analyzers Feature Request Need Design Review The end user experience design needs to be reviewed and approved.
Milestone

Comments

@bordecal
Copy link

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
Copy link
Contributor

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.

@sharwell sharwell added Area-Analyzers Resolution-By Design The behavior reported in the issue matches the current design Feature Request Need Design Review The end user experience design needs to be reviewed and approved. and removed Resolution-By Design The behavior reported in the issue matches the current design labels Aug 22, 2020
@jinujoseph jinujoseph added this to the Backlog milestone Aug 25, 2020
@bordecal
Copy link
Author

@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 added a commit to mavasani/roslyn-analyzers that referenced this issue Aug 31, 2020
Closes dotnet#4100

**Proposal**
This new property would allow users to switch between following 3 analysis modes:

1. **Default mode**: Analyzers with enabled by default and severity values that we ship today. It will continue to be the default analysis mode.
2. **OptIn or AllDisabled or Conservative mode**: All analyzers are disabled by default. Only the ones that user explicitly enables in their editorconfig/ruleset will be enabled with the explicitly configured severity.
3. **OptOut or AllEnabled or Aggressive mode**: Majority of analyzers get enabled as warnings, similar to the current FxCop analyzers package. We still want some security rules and no longer recommended rules to be disabled, but for all practical purposes, we can consider this to be the most aggressive analysis mode for the package.

The property should work amicably with AnalysisLevel, i.e. if user sets AnalysisLevel to 5.0, the aggressive, all-enabled mode only turns on rules shipped in or before 5.0 to warnings. Rules shipping after 5.0 will still be turned off by default.

**Targeted scenarios**

1. **User requests**: We already have a few user requests to enable aggressive analysis mode, similar to FxCop analyzers:
   1. dotnet#4100: This was requested from a customer from Jon’s blog.
   2. dotnet/roslyn#47046: This was requested from a customer trying out our bulk configuration editorconfig functionality to switch NetAnalyzers to be as aggressive as FxCopAnalyzers
This feature enables users who are super passionate about analyzers and code quality to easily switch to an opt-out, aggressive, analysis mode, which seems fantastic to me! At the same time, it helps users who like complete opt-in model with explicit control over each rule that is enabled in their configuration file.

2. **Path to deprecation for FxCopAnalyzers package**: Currently, we ship identical CA rules in FxCopAnalyzers and NetAnalyzers package/SDK. The only difference is default severity and enabled by default values for rules. Adding this AnalysisMode knob means existing FxCop analyzer customers can easily migrate to the .NET SDK based analyzers and get equivalent functionality and this allows us to deprecate this package soon. This will make the overall code quality analyzers story much cleaner from an end user perspective.
@mavasani
Copy link
Contributor

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

@eatdrinksleepcode
Copy link

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
Copy link

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?

@CyrusNajmabadi CyrusNajmabadi moved this from Need Proposal to Complete in IDE: Design review Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Feature Request Need Design Review The end user experience design needs to be reviewed and approved.
Projects
Status: Complete
Development

No branches or pull requests

6 participants