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

Precedence of diagnostic severity settings from compilation options, editorconfig and global analyzerconfig #44804

Closed
mavasani opened this issue Jun 2, 2020 · 4 comments · Fixed by #45871
Assignees
Labels
Area-Compilers Bug IDE-CodeStyle Built-in analyzers, fixes, and refactorings
Milestone

Comments

@mavasani
Copy link
Member

mavasani commented Jun 2, 2020

Extracted out from #37876 (comment) and #43051 (comment)

Currently, diagnostic severity configuration settings can be provided to the compiler in following ways:

  1. CompilationOptions.SpecificDiagnosticOptions (populated from command line /nowarn, /warnaserror and ruleset files)
  2. Editoconfig files
  3. Global analyzer config files

When there are entries for the same diagnostic ID from multiple sources, following precedence order is followed today:

  1. Editorconfig wins over the other two.
  2. Global analyzer config diagnostic severity setting wins over SpecificDiagnosticOptions.

This has led to following 2 core issues:

  1. Given that global analyzer config files will invariably come from tooling/NuGet packages, it seems wrong for the settings from them to override the user provided command line SpecificDiagnosticOptions. I think we need to implemented Andy's suggestion in Cannot configure severity with editorconfig for analyzer diagnostics with Location.None #37876 (comment) to address this. This would also lead to some work on the IDE side. For example, all places where we read the ruleset file to generate SpecificDiagnosticOptions would need to now read global analyzer config files (for example here). I think we need to do this change soon, otherwise it would become a breaking change in future.

  2. TreatWarningsAsErrors ignored when severity set in EditorConfig #43051 shows a side effect of editorconfig settings winning over command line SpecificDiagnosticOptions causing TreatWarningsAsErrors to not work as user expected. Changing this now would likely be a big breaking change, and defeat the original purpose of allowing per-file or directory overrides to project level diagnostic configuration. Do we have any alternatives/workarounds here which can help users?

I think we need to make a final call on the precedence order and call out the supported scenarios and recommended workarounds where required.

@mavasani
Copy link
Member Author

mavasani commented Jun 2, 2020

@mavasani
Copy link
Member Author

mavasani commented Jun 3, 2020

Actually, putting some more thought into this, I think @agocke's proposal to add global analyzer config diagnostic settings to CompilationOptions.SpecificDiagnosticOptions should resolve all the issues here.

  • Diagnostic settings from global analyzer config have the least precedence
  • TreatWarningsAsErrors ignored when severity set in EditorConfig #43051 can be resolved by recommending users to move their diagnostic entries from editorconfig to global analyzer config file. Basically, any diagnostic settings that need lesser precedence then command line arguments should be added to global analyzer config file and any per-file or per-directory overrides that must take precedence over command line arguments should go in editorconfig.
  • Global analyzer config becomes a complete 1-to-1 replacement for ruleset files, i.e. resolves Feature gaps between ruleset and .editorconfig  #38042 and one should never ever need to create a ruleset again.

@chsienki @agocke Can we prioritize this change for 16.7 before we ship the first release with global analyzer config support?

@agocke
Copy link
Member

agocke commented Jun 3, 2020

I will not have time to work on this for 16.7

@gafter gafter added this to Triage in Compiler: Warning Waves Jun 5, 2020
@chsienki chsienki self-assigned this Jun 24, 2020
@chsienki chsienki added this to the 16.7.P4 milestone Jun 24, 2020
@mavasani mavasani added the IDE-CodeStyle Built-in analyzers, fixes, and refactorings label Jul 15, 2020
@jinujoseph jinujoseph modified the milestones: 16.7.P4, 16.7 Jul 26, 2020
@jinujoseph jinujoseph modified the milestones: 16.7, 16.8 Aug 20, 2020
@mavasani
Copy link
Member Author

I believe @chsienki already has a PR to fix it: #45871. I update the PR description to make this explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug IDE-CodeStyle Built-in analyzers, fixes, and refactorings
4 participants