/warnaserror-:DiagnosticId should not nullify editorconfig diagnostic… by mavasani · Pull Request #49449 · dotnet/roslyn

mavasani · 2020-11-17T21:38:56Z

… settings for DiagnosticId

Fixes #49446

… settings for DiagnosticId Fixes dotnet#49446

mavasani · 2020-11-20T17:33:17Z

@dotnet/roslyn-compiler for second review.

mavasani · 2020-11-23T16:04:23Z

@dotnet/roslyn-compiler for second review.

AlekseyTs · 2020-11-23T16:49:17Z

            if (tree != null && syntaxTreeOptions.TryGetDiagnosticValue(tree, id, cancellationToken, out report) ||

It looks like we are overriding the report value unconditionally. It feels like that can lead to a loss of the value we got from specificDiagnosticOptions. Are we testing scenarios like that? I think it would be better to extract config setting into a separate variable and then have an explicit logic to merge both values. #Closed

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs:201 in 23dbaaa. [](commit_id = 23dbaaa, deletion_comment = False)

AlekseyTs · 2020-11-23T16:54:54Z

            if (tree != null && syntaxTreeOptions.TryGetDiagnosticValue(tree, id, cancellationToken, out report) ||

For example, it is not obvious if we can switch to ReportDiagnostic.Error and whether that would be the right thing to do.

In reply to: 732285025 [](ancestors = 732285025)

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs:201 in 23dbaaa. [](commit_id = 23dbaaa, deletion_comment = False)

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicDiagnosticFilter.vb

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

AlekseyTs · 2020-11-23T17:01:45Z

Done with review pass (iteration 1) #Closed

mavasani · 2020-11-23T17:10:34Z

            if (tree != null && syntaxTreeOptions.TryGetDiagnosticValue(tree, id, cancellationToken, out report) ||
It looks like we are overriding the report value unconditionally. It feels like that can lead to a loss of the value we got from specificDiagnosticOptions. Are we testing scenarios like that? I think it would be better to extract config setting into a separate variable and then have an explicit logic to merge both values.

Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs:201 in 23dbaaa. [](commit_id = 23dbaaa, deletion_comment = False)

Thanks, I can refactor so we don't explicitly rely on the behavior of TryGetDiagnosticValue to set value to ReportDiagnostic.Default on false return case below:

roslyn/src/Compilers/Core/Portable/Compilation/SyntaxTreeOptionsProvider.cs

Lines 77 to 85 in f59f0b5

    
           public override bool TryGetDiagnosticValue(SyntaxTree tree, string diagnosticId, CancellationToken _, out ReportDiagnostic severity) 
        
           { 
        
               if (_options.TryGetValue(tree, out var value)) 
        
               { 
        
                   return value.DiagnosticOptions.TryGetValue(diagnosticId, out severity); 
        
               } 
        
               severity = ReportDiagnostic.Default; 
        
               return false; 
        
           }

Note that this is already tested via an existing test which passes /warnaserror- without any syntax tree options, so there is no need to add a new test, but your suggested refactoring will make this more explicit.

AlekseyTs · 2020-11-23T17:19:59Z

, I can refactor so we don't explicitly rely on the behavior of TryGetDiagnosticValue to set value to ReportDiagnostic.Default on false return case

Note, this isn't the only case I am concerned about. It is not clear to me why it would be fine to read ReportDiagnostic.Error from config in our case.

In reply to: 732298342 [](ancestors = 732298342)

AlekseyTs · 2020-11-23T17:30:03Z

Note that this is already tested via an existing test which passes /warnaserror- without any syntax tree options, so there is no need to add a new test, but your suggested refactoring will make this more explicit.

I am referring to other values that might come from config, not just lack of a value.

In reply to: 732304230 [](ancestors = 732304230,732298342)

mavasani · 2020-11-23T17:46:29Z

, I can refactor so we don't explicitly rely on the behavior of TryGetDiagnosticValue to set value to ReportDiagnostic.Default on false return case

Note, this isn't the only case I am concerned about. It is not clear to me why it would be fine to read ReportDiagnostic.Error from config in our case.

In reply to: 732298342 [](ancestors = 732298342)

Maybe I am not clear about the exact scenarios that you are concerned about. The logic works as follows:

If SpecificDiagnosticOptions has a value other than ReportDiagnostic.Default, it overrides syntax tree options from config files and determines diagnostic's effective severity. Syntax tree options from config files are ignored.
Otherwise, if config file options contains a value for an ID, that overrides and determines diagnostic's reported severity. This is true even if SpecificDiagnosticOptions has a ReportDiagnostic.Default from /warnaserror-:ID. /warnaserror- cannot nullify an explicit entry from config file (even ReportDiagnostic.Error), it can only nullify a previous /warnaserror+ specification indicating a warning being bumped to an error.
If after all the above handling, diagnostic's effective severity is a warning and /warnaserror+ is applicable for the ID, it will be bumped to an error.

I can add more tests to assert this behavior.

AlekseyTs · 2020-11-23T17:56:48Z

It is not obvious to me why it is desired to override /warnaserror-:ID from command line with /warnaserror:ID from config. Since you didn't test this scenario, I couldn't tell what the expectations should be

In reply to: 732320103 [](ancestors = 732320103)

mavasani · 2020-11-23T18:03:25Z

It is not obvious to me why it is desired to override /warnaserror-:ID from command line with /warnaserror:ID from config. Since you didn't test this scenario, I couldn't tell what the expectations should be

Sounds good - I don't have a push back to change the semantics so /warnaserror-:ID from command line will override ReportDiagnostic.Error from config for warning diagnostics. IMO, these are conflicting configurations, so pretty unlikely, but your stance seems reasonable. I'll make this adjustment and add tests.

… from bumping a warning to an error. Verified that the new test fails for the case `[InlineData(warnAsErrorMinus: true, defaultSeverity: DiagnosticSeverity.Warning, severityInConfigFile: DiagnosticSeverity.Error, expectedEffectiveSeverity: DiagnosticSeverity.Warning)]` prior to the product fix in this commit, as actual effective severity was an error. This is fixed with commit.

mavasani · 2020-11-23T20:01:30Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+        // Verify '/warnaserror-:ID' has no effect when default severity is 'Info' and config file bumps severity to 'Error'
+        [InlineData(false, DiagnosticSeverity.Info, DiagnosticSeverity.Error, DiagnosticSeverity.Error)]
+        [InlineData(true, DiagnosticSeverity.Info, DiagnosticSeverity.Error, DiagnosticSeverity.Error)]
+        public void TestWarnAsErrorMinusDoesNotNullifyEditorConfig(


@AlekseyTs Please let me know if you feel there are more combinations that would be worth testing.

AlekseyTs

LGTM (iteration 3)

mavasani · 2020-11-24T17:52:40Z

Thanks!

/warnaserror-:DiagnosticId should not nullify editorconfig diagnostic…

23dbaaa

… settings for DiagnosticId Fixes dotnet#49446

mavasani added Area-Analyzers Area-Compilers labels Nov 17, 2020

mavasani added this to the 16.9 milestone Nov 17, 2020

mavasani requested review from a team and chsienki November 17, 2020 21:38

333fred approved these changes Nov 20, 2020

View reviewed changes

AlekseyTs reviewed Nov 23, 2020

View reviewed changes

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicDiagnosticFilter.vb Outdated Show resolved Hide resolved

AlekseyTs reviewed Nov 23, 2020

View reviewed changes

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs Outdated Show resolved Hide resolved

mavasani commented Nov 23, 2020

View reviewed changes

Fix comment

2bc1677

AlekseyTs approved these changes Nov 24, 2020

View reviewed changes

mavasani merged commit c536d5b into dotnet:master Nov 24, 2020

ghost modified the milestones: 16.9, Next Nov 24, 2020

mavasani deleted the FixWarnAsErrorMinus branch November 24, 2020 17:52

dibarbet removed this from the Next milestone Dec 19, 2020

dibarbet added this to the 16.9.P3 milestone Dec 19, 2020

mavasani mentioned this pull request Oct 26, 2023

Ensure '/warnaserror-:DiagnosticId' does not nullify globalconfig severity setting #70572

Merged

Conversation

mavasani commented Nov 17, 2020

Uh oh!

mavasani commented Nov 20, 2020

Uh oh!

mavasani commented Nov 23, 2020

Uh oh!

AlekseyTs commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Nov 23, 2020

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mavasani commented Nov 23, 2020

Uh oh!

AlekseyTs commented Nov 23, 2020

Uh oh!

AlekseyTs commented Nov 23, 2020

Uh oh!

mavasani commented Nov 23, 2020

Uh oh!

AlekseyTs commented Nov 23, 2020

Uh oh!

mavasani commented Nov 23, 2020

Uh oh!

mavasani Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

mavasani commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs commented Nov 23, 2020 •

edited

Loading

AlekseyTs commented Nov 23, 2020 •

edited

Loading