Ensure that warnaserror works identically for warnings configured in … #47710
Conversation
…ruleset and global config 1. Fixes dotnet#43051 2. Ensures consistent behavior between ruleset and global config Enables users to migrate away from rulesets when they desire to have rules configured as warnings by default and bump it to errors in CI with warnaserror. They cannot do this via editorconfig settings as editorconfig always overrides command line options (nowarn and warnaserror).
|
Thanks @chsienki! @dotnet/roslyn-compiler for an additional review. |
| @@ -196,6 +196,12 @@ internal static class CSharpDiagnosticFilter | |||
| { | |||
| // 4. Global analyzer config level | |||
| isSpecified = true; | |||
|
|
|||
| // '/warnaserror' should promote warnings configured in global analyzer config to error. | |||
| if (report == ReportDiagnostic.Warn && generalDiagnosticOption == ReportDiagnostic.Error) | |||
There was a problem hiding this comment.
Why do we only consider generalDiagnosticOption in this particular if block? Why not the others essentially?
There was a problem hiding this comment.
This is due to the fact that global configs are lazily parsed, while rulesets are parsed upfront by the command line parser.
- Rulesets are parsed before parsing
/warnaserrorcommand line arguments: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/CommandLine/CSharpCommandLineParser.cs,158 - When parsing
/warnaserroron command line, options from rulesets are directly bumped to errors in place: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/CommandLine/CSharpCommandLineParser.cs,828
There was a problem hiding this comment.
By the way, we can consider implementing a similar fix for settings in editorconfig files, i.e. if you specify dotnet_diagnostic.RuleId.severity = warning in an editorconfig and execute with /warnaserror, then the diagnostic gets promoted to error - that does seem more sensible behavior as /warnaserror should ideally imply one does not see any warnings, but only errors (unless of course users have a /warnaserror-:RuleId for specific rules). Customers are also requesting the fix to editorconfig behavior in #43051, but there is potential of that being a more visible breaking change for existing users as editorconfig support in compiler has been shipped for a while. We have 2 options:
- Avoid breaking change for editorconfig settings, and ask users to instead switch to global config if they desire
/warnaserrorto bump user configured warnings to errors. The approach taken currently in this PR. - Make the more correct fix of doing this change for both editorconfig and global config settings, at the risk of breaking some customers who have configured warnings in editorconfig files and run with
/warnaserror. This would be a simple change to make in this PR if we are comfortable with it.
There was a problem hiding this comment.
Where do we document what the precedence level is between all of the various options here? I was a bit surprised at first glance to see that the test verifies that the command line essentially overrides the config files. Likely my intuition is just off here. But wanted to find a full list of "A wins over B" so I could consider what other tests we may need here.
There was a problem hiding this comment.
We have #47154 tracking adding this documentation.
There was a problem hiding this comment.
@jaredpar @chsienki I plan to add the precedence level documentation tracked in #47154 to docs.microsoft.com later today.
However, the open question in my above comment #47710 (comment) still needs to be answered to define this order wrt to command line options: do we believe that command line options (/nowarn and /warnaserror) should always override diagnostic settings from all of the other sources (editorconfig, global config and rulesets)? I think that should be the case, as shown by the scenarios in #43051.
This PR ensures these semantics for global configs. I can create a follow-up PR for ensuring the same semantics for editorconfigs, if we feel it is a reasonable behavior change.
There was a problem hiding this comment.
@jaredpar Do you feel scheduling a meeting for discussing this would be preferrable? Also, should this PR be retargeted to 16.8 release branch?
There was a problem hiding this comment.
Sorry I just lost track of this.
|
Hello @mavasani! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
1 similar comment
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
ghost
merged commit 
…ruleset and global config
This change enables users to migrate away from rulesets when they desire to have rules configured as warnings by default and bump it to errors in CI with warnaserror. They cannot do this via editorconfig as diagnostic severity settings in editorconfig always override command line options (nowarn and warnaserror).