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;
    }
}

3. dotnet build -p:TreatWarningsAsErrors=true
4. Add .editorconfig with:

root = true

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

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

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

[ghost]

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.

[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
   

   roslyn/src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs

   Lines 243 to 254 in 632020a

   	bool promoteToAnError()
   	{
   	Debug.Assert(report == ReportDiagnostic.Default);
   	Debug.Assert(generalDiagnosticOption == ReportDiagnostic.Error);
   	// If we've been asked to do warn-as-error then don't raise severity for anything below warning (info or hidden).
   	return severity == DiagnosticSeverity.Warning &&
   	// In the case where /warnaserror+ is followed by /warnaserror-:<n> on the command line,
   	// do not promote the warning specified in <n> to an error.
   	!isSpecified;
   	}

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):

   roslyn/src/Compilers/CSharp/Portable/Compilation/CSharpDiagnosticFilter.cs

   Lines 152 to 165 in 632020a

   	bool isSpecified = false;
   	if (tree != null && tree.DiagnosticOptions.TryGetValue(id, out var treeReport))
   	{
   	// 2. Syntax tree level
   	report = treeReport;
   	isSpecified = true;
   	}
   	else if (specificDiagnosticOptions.TryGetValue(id, out var specificReport))
   	{
   	// 3. Compilation level
   	report = specificReport;
   	isSpecified = true;
   	}

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 dotnet/msbuild#3062.

Due to xamarin/Essentials#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 #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 #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;
    }
}

3. dotnet build -p:TreatWarningsAsErrors=true
4. Add global analyzerconfig with:

is_global = true
dotnet_diagnostic.CS0219.severity = warning

5. dotnet build -p:TreatWarningsAsErrors=true

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

[mavasani]

@maxild Maybe the following docs will help clarify differences between global configs and regular editorconfigs:

• https://docs.microsoft.com/dotnet/fundamentals/code-analysis/configuration-files
• Editorconfig vs Global analyzerconfig #47707

One of the primary clients for global configs are NuGet packages and SDKs, but we also expect end users to add them to their repos. One example is Roslyn repo itself: https://github.com/dotnet/roslyn/tree/master/eng/config/globalconfigs

[maxild]

Okay, but then I guess as an end-user you will either use editorconfig (and have precedence/conflicts resolved by longest path, last-line-in-file wins in the file system), or use global analyzer files (and have the tooling merge them somehow(?) resolving conflicts). To use both seems a bit confusing (reading the docs now on resolving conflicts)

Precedence for conflicting severity entries from a ruleset file and EditorConfig or global AnalyzerConfig files is undefined. (https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files#severity-options)

Is this going to changed (warning or some deterministic precedence rule?) I hope this means that conflicts between

1. rule set
2. editorconfig/globalconfig

are undefined. So this is not a problem, if migrating 100 percent away from rule sets. Any severity in editorconfig will always win over a conflicting .globalconfig severity, right? and .globalconfig conflicts between two or more (global analyzer config) files are sort of unresolved, causing a build warning, and the config to be ignored by the compiler.

I can see in your roslyn example that global analyzer files are only used for configuring severity. So a severity configured using what I call Code style syntax for severity setting

root = true

[*.cs]
dotnet_style_qualification_for_field = true:warning

in an .editorconfig will win over the same severity configured using what I call diagnostic-ID based syntax for severity setting

is_global = true

dotnet_style_qualification_for_field = true
dotnet_diagnostic.RuleID.severity = warning

in a .globalconfig. This way it seems the fixed/static configuration should be in .editorconfig, and "layers" of global analyzer config can be imported if needed?

Are there any documentation of how to best write configs considering all the options for both a split in file types (editorconf=file-based vs globalconf="virtual/flat"-based) and the different ways to specify syntax for an ID above. I guess code styles (language, naming, format/whitespace etc) should all go in .editorconfig. And only severity configuration should go into global analyzer files. What do you guys think?

[mavasani]

Any severity in editorconfig will always win over a conflicting .globalconfig severity, right?

Yes

and .globalconfig conflicts between two or more (global analyzer config) files are sort of unresolved, causing a build warning, and the config to be ignored by the compiler.

Yes, though we very recently added a feature to allow conflict resolution between global configs be specifying levels: #49834

Code style syntax for severity setting

Please avoid using this syntax for configuring severity. There are known problems with this and will soon be deprecated: #44201. Instead use the following simple notion:

1. Use Code style syntax for specifying option value (without severity)
2. Use the diagnostic-ID based syntax for specifying severity

Both the above can be specified in either an editorconfig or a globalconfig.

[mavasani]

Are there any documentation of how to best write configs considering all the options for both a split in file types (editorconf=file-based vs globalconf="virtual/flat"-based) and the different ways to specify syntax for an ID above

Realistically, there should be no split. You should ideally choose global config for folder agnostic project based option/severity specifications AND you choose .editorconfig for folder based option/severity specification. Generally the latter should suffice, but there might be some cases you need the former.

[mavasani]

I guess code styles (language, naming, format/whitespace etc) should all go in .editorconfig. And only severity configuration should go into global analyzer files.

No, this is not true. All option/severity configurations for analyzers can go either in an editorconfig or globalconfig. All non-analyzer related settings should go into editorconfig.

[maxild]

Maybe other IDEs (vscode, rider) will not be able to work with option/severity config from .globalconfig? Haven't tested it yet, but will soon!

Based on your answers, it seems like the following conclusions can be made (about end-user config):

1. Use core (https://editorconfig.org/) options from original .editorconfig (everybody knows that...not an interesting point)
2. Do not configure option and severity on the same line (Deprecate 'severity' field for IDE code style editorconfig syntax #44201) (I have a few questions about that later)
   i. Use Code style syntax for specifying option value (without severity)
   ii. Use the diagnostic-ID based syntax for specifying severity
3. Add any (dotnet/csharp) 'code styles' and/or 'code quality' option and/or severity configuration to whatever file you like:
   i. editorconfig if you need globbing and file-system based precedence for the option or severity.
   ii. global analyzer config (named .globalconfig) if you need file-system agnostic precedence for the option or severity, and have any conflicts under control(!?!?!). (that is until Support global analyzer config levels: #49834, you should always use a single .globalconfig in the repo root....you should probably always have a single file anyway, unless you need to conditionally bring it in via msbuild control flow....I still guess that multiple files will come from nuget/msbuild tooling)
   iii. But have a preference for .editorconfig file(s) (they are probably sufficient and are better known and more idiomatic in the community).

Questions:

a. Why do roslyn and roslyn-analyzers use this soon deprecated syntax?

• https://github.com/dotnet/roslyn/blob/master/.editorconfig#L48-L62
• https://github.com/dotnet/roslyn-analyzers/blob/master/.editorconfig#L38-L76

b. How do I translate dotnet_style_qualification_for_field into some RuleID (in this case ID0003 and ID0009 found here) ? (this seems like a manual/tedious process)

[mavasani]

Your conclusions above all seem to be valid

Why do roslyn and roslyn-analyzers use this soon deprecated syntax?

Just haven't got to cleaning it up, and we still have devs that are used to the old syntax, so it will hopefully die down with time.

How do I translate dotnet_style_qualification_for_field into some RuleID (in this case ID0003 and ID0009 found here) ? (this seems like a manual/tedious process)

Yes, unfortunately, it is quite a manual process right now. You can reference this index to help you: https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/#index. Our long term goal is to have an editorconfig/globalconfig UX that allows you to select a specific code style and context menus to configure the style and severity, such that the tooling will automatically update the config file and you will not have to manually deciper/update these files. @jmarolf is actively working on such a UX.

[maxild]

@mavasani Thanks for the link.

Our long term goal is to have an editorconfig/globalconfig UX that allows you to select a specific code style and context menus to configure the style and severity, such that the tooling will automatically update the config file and you will not have to manually deciper/update these files. @jmarolf is actively working on such a UX.

Are there any public specs for such UX, or is it to early for that ?

(In 16.8 I can configure severity from a multitude of places: error list, references->analyzers in solution explorer, alt+enter tips in editor etc), and I have allready analyzed the AnalysisMode/AnalysisLevel/Category based editorconfig-severity files from the CodeStyle/CodeQuality nupkgs (`build/config' subfolder).

I guess the UX will be part of vs2019 16.9+. Right now I have installed win10/vs2019 in vmware player, just to work with editorconfig-based config on Linux (The UX in vs2019 16.8.x is way better than in rider/vscode). I could actually dogfood 16.9 previews, but right now my main milestone is getting to work on 16.8 (i.e. sdk-5.0.102) accross workstations running win/macos/linux).

[maxild]

For anyone interested in working on 16.8.x bits (5.0.1nn, equal to this sdk patch-line I guess?) you have to duplicate all your severity config in .editorconfig and .globalconfig, and then define all options in the .editorconfig. This way both live analysis and IDE/CLI build work wrt errors.

[maxild]

@mavasani I still have to use dotnet msbuild -t:Rebuild -warnaserror instead of dotnet msbuild -t:Rebuild -p:TreatWarningsAsErrors=true. As a workaround I used the undocumented dotnet msbuild -t:Rebuild -p:MSBuildTreatWarningsAsErrors=true, because I needed msbuild property (not msbuild switch/option CLI argument).

I am on SDK version 5.0.202

[sharwell]

@maxild What version of the compiler are you using? You can check this by adding this code to a source file:

#error version

This issue is fixed starting in the 3.9 compiler.

[maxild]

$ dotnet /usr/share/dotnet/sdk/5.0.202/Roslyn/bincore/csc.dll -version
3.9.0-6.21160.10 (59eedc33)

BTW @sharwell I wish dotnet --info was a bit more informative wrt csc, nuget, msbuild versions. See dotnet/sdk#15496. Would make issue handling (cross-platform, cross-arch) much easier without me having to write script myself, or remembering undocumented preprocessor directives #version

[JimBobSquarePants]

@sharwell Maybe I'm missing something but this still appears to be an issue.

dotnet --info
5.0.301 [C:\Program Files\dotnet\sdk]

Given a .globalconfig file or .editorconfig with the following:

dotnet_analyzer_diagnostic.severity = warning

And property in the .csproj

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>

The build will succeed despite StyleCop analyzer config warnings.

Removing that explicit rule makes things work as expected.

I discovered this when raising an issue in a popular .editorconfig template repo.

RehanSaeed/EditorConfig#56 (comment)

I experimented with

<WarningsAsErrors />

As set via Properties UI in Visual Studio but that didn't seem to work at all.

[RehanSaeed]

@mavasani @sharwell This is still an issue as per @JimBobSquarePants post above. Can this be re-opened?

[sharwell]

Hi @JimBobSquarePants,

Can you follow the steps here to determine the compiler version used in the build?
#43051 (comment)

Note that the version of CSC used in the build may or may not be the one included in the dotnet SDK (it can vary by project). The #error version line is the most reliable way to get the exact compiler version used for a given build.

Thanks,
Sam

[JimBobSquarePants]

@sharwell Following your instructions I get this.

version: '3.10.0-4.21329.37 (246ce641)'. Language version: 9.0.	

I can confirm that the sample project listed here RehanSaeed/EditorConfig#56 still demonstrates an issue.

[sharwell]

@JimBobSquarePants @RehanSaeed Thank you for the additional details. I was able to verify that the original bug above has been fixed (i.e. the dotnet_diagnostic.ID.severity syntax works correctly with TreatWarningsAsErrors). Can you file a new bug for the behavior of dotnet_analyzer_diagnostic.severity and dotnet_analyzer_diagnostic.category-Category.severity (a different setting)?

[RehanSaeed]

@sharwell I've raised #55541.