Roslyn: TreatWarningsAsErrors ignored when severity set in EditorConfig

[ArturDorochowicz]

Workaround

Use dotnet build -warnaserror instead of dotnet build -p:TreatWarningsAsErrors=true in step 5 of the below repros.

Repro 1 (Compiler diagnostics)

Repro steps

1. dotnet new console
2. Edit the source code to:

class Program
{
    static void Main(string[] args)
    {
        // warning CS0219: The variable 'unused' is assigned but its value is never used
        int unused = 0;
    }
}

1. dotnet build -p:TreatWarningsAsErrors=true
2. Add .editorconfig with:

root = true

[*.cs]
dotnet_diagnostic.CS0219.severity = warning

1. dotnet build -p:TreatWarningsAsErrors=true

Expected behavior

I expect the build output in steps 3 and 5 to be the same, i.e. the build in both cases should fail due to CS0219 being promoted to an error.

Actual behavior

Build fails in step 3, but not in step 5. dotnet_diagnostic.CS0219.severity = warning takes precedence over /warnaserror+ passed to csc.exe from -p:TreatWarningsAsErrors=true

Repro 2 (Analyzer diagnostics)

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.8 (Latest)

Diagnostic ID

N/A

$ dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   3.1.200
 Commit:    c5123d973b

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  18.04
 OS Platform: Linux
 RID:         ubuntu.18.04-x64
 Base Path:   /usr/share/dotnet/sdk/3.1.200/

Host (useful for support):
  Version: 3.1.2
  Commit:  916b5cba26

.NET Core SDKs installed:
  3.1.200 [/usr/share/dotnet/sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.App 3.1.2 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.2 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

Repro steps

1. dotnet new console
2. dotnet add package Microsoft.CodeAnalysis.FxCopAnalyzers -v 2.9.8
3. dotnet build -p:TreatWarningsAsErrors=true
4. Add .editorconfig with:

root = true

[*.cs]
dotnet_diagnostic.CA1303.severity = default
dotnet_diagnostic.CA1801.severity = warning

1. dotnet build -p:TreatWarningsAsErrors=true

Expected behavior

I expect the build output in steps 3 and 5 to be the same, i.e. the build in both cases should fail due to two build errors: CA1303, CA1801.

Actual behavior

The output from step 3 shows build failure due to two errors: CA1303, CA1801. This is correct and expected:

$ dotnet build -p:TreatWarningsAsErrors=true
Microsoft (R) Build Engine version 16.5.0+d4cbfca49 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 35.32 ms for /home/a-x-d/dev/fx1/fx1.csproj.
Program.cs(9,31): error CA1303: Method 'void Program.Main(string[] args)' passes a literal string as parameter 'value' of a call to 'void Console.WriteLine(string value)'. Retrieve the following string(s) from a resource table instead: "Hello World!". [/home/a-x-d/dev/fx1/fx1.csproj]
Program.cs(7,35): error CA1801: Parameter args of method Main is never used. Remove the parameter or use it in the method body. [/home/a-x-d/dev/fx1/fx1.csproj]

Build FAILED.

Program.cs(9,31): error CA1303: Method 'void Program.Main(string[] args)' passes a literal string as parameter 'value' of a call to 'void Console.WriteLine(string value)'. Retrieve the following string(s) from a resource table instead: "Hello World!". [/home/a-x-d/dev/fx1/fx1.csproj]
Program.cs(7,35): error CA1801: Parameter args of method Main is never used. Remove the parameter or use it in the method body. [/home/a-x-d/dev/fx1/fx1.csproj]
    0 Warning(s)
    2 Error(s)

Time Elapsed 00:00:00.98

The build output from step 5 shows build passing with two warnings for CA1303, CA1801. Somehow editor config configuration overrides TreatWarningsAsErrors property. This is not expected and seems incorrect:

$ dotnet build -p:TreatWarningsAsErrors=true
Microsoft (R) Build Engine version 16.5.0+d4cbfca49 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 216.45 ms for /home/a-x-d/dev/fx1/fx1.csproj.
Program.cs(9,31): warning CA1303: Method 'void Program.Main(string[] args)' passes a literal string as parameter 'value' of a call to 'void Co
nsole.WriteLine(string value)'. Retrieve the following string(s) from a resource table instead: "Hello World!". [/home/a-x-d/dev/fx1/fx1.cspro
j]
Program.cs(7,35): warning CA1801: Parameter args of method Main is never used. Remove the parameter or use it in the method body. [/home/a-x-d
/dev/fx1/fx1.csproj]
  fx1 -> /home/a-x-d/dev/fx1/bin/Debug/netcoreapp3.1/fx1.dll

Build succeeded.

Program.cs(9,31): warning CA1303: Method 'void Program.Main(string[] args)' passes a literal string as parameter 'value' of a call to 'void Co
nsole.WriteLine(string value)'. Retrieve the following string(s) from a resource table instead: "Hello World!". [/home/a-x-d/dev/fx1/fx1.cspro
j]
Program.cs(7,35): warning CA1801: Parameter args of method Main is never used. Remove the parameter or use it in the method body. [/home/a-x-d/dev/fx1/fx1.csproj]
    2 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.35

[beyondunn]

I can confirm that I am experiencing this same issue after migrating my solution's analyzer rule severity configuration from a .ruleset to an .editorconfig file.

[mavasani]

@sharwell @jinujoseph Can you port this issue to roslyn repo and assign it to Area-Compilers?

[bddckr]

I migrated from rulesets to editorconfig for all analyzer configuration recently and our team is now manually checking CI build results for warnings. With rulesets we could easily make all analyzer diagnostics warnings locally, but errors in CI builds thanks to TreatWarningsAsErrors.

I'm looking into a workaround until a solution has been found - any ideas? It looks like for now I can just make our CI inject the following at the start of the editorconfig file:

dotnet_analyzer_diagnostic.severity = error

I don't really like this approach as we're aiming for the ability to control this behavior easily in our dev environments. For that we import some kind of User.props that allows any team member to basically turn on "build as CI" with a single MSBuild property.

Is there any other MSBuild property that controls this behavior already, or does it need a fix to make TreatWarningsAsErrors work for these, too?

[mavasani]

Tagging @agocke. I believe this behavior is by design. Editorconfig based severity settings take precedence over settings from ruleset file and command line arguments. See next comment.

[mavasani]

Actually, I think my last comment is not true. All our repos (roslyn, roslyn-analyzers, etc.) use editorconfig based configuration, we install many analyzer packages that configure bunch of code style rules as warnings for local development (for example, IDE0055 "Fix formatting") and then specify /p:TreatWarningsAsErrors = true in CI on msbuild command line and these get bumped to errors and break build.

[sharwell]

@mavasani there are two ways to treat warnings as errors in command line builds. One is an MSBuild flag (/warnAsError), which raises the severity of all messages coming from individual tools, and the other is a CSC flag (set via /p:TreatWarningsAsErrors=true), which instructs the C# compiler to increase the severity of diagnostics. Our builds set both of these at different times, so it would be good to verify that the test is specific to cases where only the latter is set.

I would expect these flags to have the highest priority.

[mavasani]

@sharwell - thanks that clears things for me. If the original repro steps use /warnaserror instead of /p:TreatWarningsAsErrors=true then the analyzer diagnostics are indeed promoted to errors and break build. So at least there is a workaround now.

I also verified that /p:TreatWarningsAsErrors=true promotes compiler warnings to errors. So the issue seems specific to analyzer diagnostics. I am going to mark this as "Area-Analyzers" and investigate further.

[mavasani]

I also verified that /p:TreatWarningsAsErrors=true promotes compiler warnings to errors. So the issue seems specific to analyzer diagnostics. I am going to mark this as "Area-Analyzers" and investigate further.

This turned out to be a red-herring. The only reason why compiler warning got promoted to an error was that there was no explicit entry in the editorconfig. Let me add a Repro 2 in the original issue description to clarify this is not specific to analyzer diagnostics, but the fact that diagnostic severity settings coming from an .editorconfig file are overriding /warnaserror from command line/MSBuild file.

[mavasani]

@agocke @sharwell I have updated the original repro steps to confirm that this issue is not specific to analyzer diagnostics.

[mavasani]

Investigated a bit more here. The following code is implementing the logic as today.

1. The code to promote a warning to error only kicks if isSpecified is false
   https://github.com/dotnet/roslyn/blob/632020ae5ecdb5e7987be542ad471d7625285a41/src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs#L243-L254
2. There are couple of cases where isSpecified is true: An explicit diagnostic ID severity entry in editorconfig file (i.e. case // 2. Syntax tree level below) or ruleset file (i.e. case // 3. Compilation level): https://github.com/dotnet/roslyn/blob/632020ae5ecdb5e7987be542ad471d7625285a41/src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs#L152-L165

So the above issue does not seem to be specific to use of editorconfig in step 4 (I think it would also repro with using a ruleset file in step 4). I also think the compiler behavior is reasonable as user has explicitly requested a rule ID to be a specific severity, which wins over the fallback compilation wide warnaserror setting. Changing this behavior would also be a breaking change. TreatWarnignsAsErrors still works fine for any rule ID which is not explicitly configured in an editorconfig/ruleset.

IMO, this feels like "By-design" for compiler, with use of /warnaserror MSBuild argument as the correct approach to force all warnings to errors. I will let @agocke make the final call here.

[ArturDorochowicz]

So the above issue does not seem to be specific to use of editorconfig in step 4 (I think it would also repro with using a ruleset file in step 4).

@mavasani No, it doesn't repro with rulesets. The first two comments back this up and, additionally, I also discovered this change in behavior when migrating from rulesets to editorconfig.

[agocke]

Yes, the design was that more specific options, namely syntax-tree specific settings, would win over more general options, like warn as error.

On the other hand, I can see why the current behavior would be confusing. Let's leave this open for now.

[bddckr]

The suggested workaround does not work for us:

-warnaserror, seemingly equivalent to MSBuildTreatWarningsAsErrors (still not documented), has no equivalent of WarningsNotAsErrors. I.e. there is no MSBuildWarningsNotAsErrors currently. That seems to be tracked by https://github.com/microsoft/msbuild/issues/3062.

Due to https://github.com/xamarin/Essentials/issues/904 we're getting a few "known to be OK" MSB3277 warnings. We still want MSB3277 to show up as warnings in our builds, as a change might introduce this warning for something else than the linked Xamarin Essentials issue. (I should add: This is just an example of a "known to be OK" warning we currently experience. Not all of them have workarounds available, like the linked Xamarin issue here.)

If we use -warnaserror (or MSBuildTreatWarningsAsErrors) we are able to turn _all_ warnings into errors, but there is no way to selectively exclude some warnings from getting treated as a warning with this. Maintaining a list of all the warnings to use with MSBuildWarningsAsErrors alternatively is not feasible.

[mavasani]

I expected this scenario to work with the use of Global analyzer config files, as opposed to regular .editorconfig files. See https://github.com/dotnet/roslyn/issues/47707 for details on Global analyzer config files and differences with regular editorconfig files.

However, there seems to be another bug preventing this - I have created https://github.com/dotnet/roslyn/pull/47710 to fix this issue. After that PR is merged, following steps should work:

1. dotnet new console
2. Edit the source code to:

class Program
{
    static void Main(string[] args)
    {
        // warning CS0219: The variable 'unused' is assigned but its value is never used
        int unused = 0;
    }
}

1. dotnet build -p:TreatWarningsAsErrors=true
2. Add global analyzerconfig with:

is_global = true
dotnet_diagnostic.CS0219.severity = warning

1. dotnet build -p:TreatWarningsAsErrors=true

Steps 3 and 5 should have the same behavior now. Both should fail with an error.

[mavasani]

Based on the offline discussion with the compiler team, we are going to also fix up editorconfig so that command line options (/nowarn and /warnaserror) take precedence over editorconfig settings. Basically, fix the original issue reported here.

[mavasani]

Update

• In VS2019 16.8, you should be able to workaround this issue by using global analyzer configs as per https://github.com/dotnet/roslyn/issues/43051#issuecomment-692837489.
• In VS2019 16.9 Preview1 and later, the original reported issue should be resolved and you can use editorconfig file itself and /warnaserror will escalate diagnostics configured as warnings in editorconfig to be errors.