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.

Sign up for GitHub

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

Ensure that warnaserror works identically for warnings configured in … #47710

Merged
1 commit merged into from
Sep 25, 2020
Merged

Ensure that warnaserror works identically for warnings configured in … #47710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,12 @@ internal static ReportDiagnostic GetDiagnosticReport(
{
// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only consider generalDiagnosticOption in this particular if block? Why not the others essentially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to the fact that global configs are lazily parsed, while rulesets are parsed upfront by the command line parser.

  1. Rulesets are parsed before parsing /warnaserror command line arguments: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/CommandLine/CSharpCommandLineParser.cs,158
  2. When parsing /warnaserror on command line, options from rulesets are directly bumped to errors in place: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/CommandLine/CSharpCommandLineParser.cs,828

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Avoid breaking change for editorconfig settings, and ask users to instead switch to global config if they desire /warnaserror to bump user configured warnings to errors. The approach taken currently in this PR.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have #47154 tracking adding this documentation.

Copy link
Contributor Author

@mavasani mavasani Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredpar Do you feel scheduling a meeting for discussing this would be preferrable? Also, should this PR be retargeted to 16.8 release branch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just lost track of this.

{
report = ReportDiagnostic.Error;
}
}
else
{
Expand Down
38 changes: 38 additions & 0 deletions src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12814,6 +12814,44 @@ void M()
VerifyOutput(dir, src, additionalFlags: new[] { "/warnaserror+", "/analyzerconfig:" + globalConfig.Path }, includeCurrentAssemblyAsAnalyzerReference: false);
}

[Theory, CombinatorialData]
[WorkItem(43051, "https://github.com/dotnet/roslyn/issues/43051")]
public void WarnAsErrorIsRespectedForForWarningsConfiguredInRulesetOrGlobalConfig(bool useGlobalConfig)
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("temp.cs").WriteAllText(@"
class C
{
void M()
{
label1:;
}
}");
var additionalFlags = new[] { "/warnaserror+" };
if (useGlobalConfig)
{
var globalConfig = dir.CreateFile(".globalconfig").WriteAllText($@"
is_global = true
dotnet_diagnostic.CS0164.severity = warning;
");
additionalFlags = additionalFlags.Append("/analyzerconfig:" + globalConfig.Path).ToArray();
}
else
{
string ruleSetSource = @"<?xml version=""1.0"" encoding=""utf-8""?>
<RuleSet Name=""Ruleset1"" Description=""Test"" ToolsVersion=""15.0"">
<Rules AnalyzerId=""Compiler"" RuleNamespace=""Compiler"">
<Rule Id=""CS0164"" Action=""Warning"" />
</Rules>
</RuleSet>
";
_ = dir.CreateFile("Rules.ruleset").WriteAllText(ruleSetSource);
additionalFlags = additionalFlags.Append("/ruleset:Rules.ruleset").ToArray();
}

VerifyOutput(dir, src, additionalFlags: additionalFlags, expectedErrorCount: 1, includeCurrentAssemblyAsAnalyzerReference: false);
}

[Fact]
[WorkItem(44087, "https://github.com/dotnet/roslyn/issues/44804")]
public void GlobalAnalyzerConfigSectionsOverrideCommandLine()
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/VisualBasic/Portable/Compilation/VisualBasicDiagnosticFilter.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
ElseIf syntaxTreeOptions IsNot Nothing AndAlso syntaxTreeOptions.TryGetGlobalDiagnosticValue(id, cancellationToken, report) Then
' 4. Global analyzer config level
isSpecified = True

' '/warnaserror' should promote warnings configured in global analyzer config to error.
If report = ReportDiagnostic.Warn AndAlso generalDiagnosticOption = ReportDiagnostic.Error Then
report = ReportDiagnostic.Error
End If
Else
report = If(isEnabledByDefault, ReportDiagnostic.Default, ReportDiagnostic.Suppress)
End If
Expand Down
33 changes: 33 additions & 0 deletions src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10091,6 +10091,39 @@ dotnet_diagnostic.BC42024.severity = warning;
VerifyOutput(dir, src, includeCurrentAssemblyAsAnalyzerReference:=False, expectedWarningCount:=1, additionalFlags:={"/nowarn:BC42024", globalOption, specificOption})
End Sub

<Theory, CombinatorialData>
<WorkItem(43051, "https://github.com/dotnet/roslyn/issues/43051")>
Public Sub WarnAsErrorIsRespectedForForWarningsConfiguredInRulesetOrGlobalConfig(useGlobalConfig As Boolean)
Dim dir = Temp.CreateDirectory()
Dim src = dir.CreateFile("temp.vb").WriteAllText("
Class C
Private Sub M()
Dim a As String
End Sub
End Class")
Dim additionalFlags = {"/warnaserror+"}

If useGlobalConfig Then
Dim globalConfig = dir.CreateFile(".globalconfig").WriteAllText($"
is_global = true
dotnet_diagnostic.BC42024.severity = warning;
")
additionalFlags = additionalFlags.Append("/analyzerconfig:" & globalConfig.Path).ToArray()
Else
Dim ruleSetSource As String = "<?xml version=""1.0"" encoding=""utf-8""?>
<RuleSet Name=""Ruleset1"" Description=""Test"" ToolsVersion=""15.0"">
<Rules AnalyzerId=""Compiler"" RuleNamespace=""Compiler"">
<Rule Id=""BC42024"" Action=""Warning"" />
</Rules>
</RuleSet>
"
dir.CreateFile("Rules.ruleset").WriteAllText(ruleSetSource)
additionalFlags = additionalFlags.Append("/ruleset:Rules.ruleset").ToArray()
End If

VerifyOutput(dir, src, additionalFlags:=additionalFlags, expectedErrorCount:=1, includeCurrentAssemblyAsAnalyzerReference:=False)
End Sub

<Fact>
<WorkItem(44087, "https://github.com/dotnet/roslyn/issues/44804")>
Public Sub GlobalAnalyzerConfigSpecificDiagnosticOptionsOverrideGeneralCommandLineOptions()
Expand Down