Different .editorconfig severity strings are used to specify IDE code style options and dotnet_diagnostic.severity

[mavasani]

Version Used:
Latest master source

I found out we use different editorconfig strings for severities specified through our code style options and the ones that the compiler looks for:

DiagnosticSeverity	IDE Code style term	Compiler dotnet_diagnostic.severity term
Error	"error"	"error"
Warning	"warning"	"warn"
Info	"suggestion"	"info"
Hidden	"silent" or "refactoring"	"hidden"
Suppress	"none"	"suppress"

So, user needs to use different terms for specifying severies, which seems very confusing:

[*.cs]

# CA1822: Mark members as static
dotnet_diagnostic.CA1822.severity = warn

# IDE0059: Unnecessary assignment of a value
csharp_style_unused_value_assignment_preference = discard_variable:warning

Compiler generates the following diagnostic if term warning is used for dotnet_diagnostic.CA1822.severity

CSC : warning InvalidSeverityInAnalyzerConfig: The diagnostic 'ca1822' was given an invalid severity 'warning' in the analyzer config file

IDE code style:
Documentation: https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference?view=vs-2019#language-code-styles
Implementation: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/NamingStyles/EditorConfig/EditorConfigSeverityStrings.cs,5

Compiler:

roslyn/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs

Line 158 in 2f57d11

ReportDiagnostic? severity;

I believe the compiler terms should be switched to follow the already shipped terminology or at least the compiler should allow the terms from both columns in the table above

[mavasani]

Tagging @jasonmalinowski @agocke

[jasonmalinowski]

Agreed, we should just make these the same. @agocke any concerns with updating the compiler list?

[jasonmalinowski]

Raising @jaredpar as @agocke won't be able to look at this for a few days. @jaredpar this is a request to change the strings the new .editorconfig compiler support accepts to be consistent with the existing properties. Who can help look at tihs?

[jaredpar]

@333fred can take a look here.

[333fred]

This should be a fairly easy change to make, just changing the strings we match against here: https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs#L160-L193. I'd be in favor of just standardizing to the already-shipped IDE terms.

[jasonmalinowski]

@333fred Is this something you are able to write or OK if we send the pull request?

[jasonmalinowski]

@agocke: I think @333fred and I were curious if you had any concerns about this?

[agocke]

For a little more context: this is not a bug, I chose to use ReportDiagnostic deliberately. Changing this code to use the IDE names isn't restoring consistency, it's just trading one inconsistency for another. The compiler API uses, and has always used, the ReportDiagnostic enum to represent diagnostic configuration.

For some unknown reason, the IDE created new names for ruleset:

None
Hidden
Info
Warning
Error
Default

Then, I guess someone else decided those weren't good either, so we get the following names for the IDE editorconfig:

None
Silent
Suggestion
Warning
Error

and drop "default" entirely.

Now the proposal is that we basically use the existing editorconfig names and add back default. This would be somewhat consistent with existing editorconfig, but would now be inconsistent with the API using ReportDiagnostic. I can see some value here for people porting their existing editorconfig files but, to be honest, the whole format of the suppression system has changed anyway. I think the appropriate way to do this is to have a code fixer that can help people move from the old format to the new format. If we have that, I'm not sure keeping the old name is really that important.

However, if we don't have the code fixer, or we don't think it's usable for a large segment of users, then maybe the consistency has some value, but I want to be clear on the tradeoff we're making.

[jasonmalinowski]

The compiler API uses, and has always used, the ReportDiagnostic enum to represent diagnostic configuration.

That's a set of names only understood by analyzer authors and compiler developers, which most definitely isn't the set of users we are aiming for here.

@mavasani do you know more about the history of the naming here?

[agocke]

@jasonmalinowski Right, that's why I'm not really that opposed to changing it if we have good reason, it just seems like if there are already massive changes to the format (i.e., the new dotnet_diagnostic line) why not just use the original compiler names? The new ones don't really seem better to me.