Roslyn: 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:
https://github.com/dotnet/roslyn/blob/2f57d11ecc36d11c06fc862dcb0de37c96d305d2/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs#L158

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.

[jasonmalinowski]

Those are the names that our users do have in .editorconfig files _today_ -- it's not helping users if some parts of the file require one set of terms and another part of the file require another set of terms. I agree thta it's not clear if the names are "better" either, but consistency with a thing users know seems more valuable over consistency over things they've never seen.

[agocke]

@jasonmalinowski Yup, that's true. I'll change it.

[jasonmalinowski]

@agocke If you need somebody else to do it we're happy to do it over here too.

[agocke]

@jasonmalinowski Sounds fine to me, you can go ahead.

[mavasani]

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

The chosen naming for the ReportDiagnostic enum came from the terminology used for ruleset files. One of the core priorities of the diagnostics team that initially worked on adding the analyzer API and infrastructure in the IDE and compiler layers was to ensure ease of migration for existing post-build managed code analysis users to the new analyzer based model. This meant the existing configuration mechanism (ruleset files and suppress message attribute suppressions) had to be supported - this was a unanimous design choice made by the IDE, compiler and diagnostics team back then and has significantly helped the customers till date as there was no editorconfig based diagnostic configuration support and all other configuration means in the compiler were not sufficient and powerful enough.

It was unfortunate that the chosen IDE code style severity terms did not match the existing terms used in the compiler layer and ruleset files. I tend to agree with @agocke that using the IDE code style terms for the new editorconfig support will also introduce inconsistencies, not only with the ReportDiagnostic API, but also for end users who move from the ruleset based configuration to editorconfig based configuration. Having said that, I do agree with @jasonmalinowski that this is probably the better evil amongst the two, naming inconsistencies in the same file are more painful, especially given we want to deprecate rulesets.

One other option might be to allow the compiler to support both the IDE code style and ruleset terminologies (both columns in the table in the issue description), with the former being the recommended terminology.

[jasonmalinowski]

I have no objection to both the IDE and compiler supporting both values, now appreciating that the .editorconfig naming is going to cause transition problems as well. So I guess we have two options:

1. Compiler adopts IDE .editorconfig naming, and rulesets are the odd one out.
2. Update both compiler and IDE to accept each other's names.

@agocke any preference between them? And @mavasani if you had to pick a preference which would it be?

[mavasani]

I have no strong preference towards either approach. If I had to pick one, I would pick 2. if implementation/test cost is not significantly higher. Otherwise, 1. is reasonable.

[agocke]

My preference is we pick the existing editorconfig terms. I think it's going to be by far the most common format going forward and one option is simpler than two.

As far as the API goes, the compiler will always consume the ReportDiagnostic enum values, but realistically the enum and the names are two different things.

At the end of the day, I suppose I'm arguing that we soft deprecate ruleset.

[jasonmalinowski]

At the end of the day, I suppose I'm arguing that we soft deprecate ruleset.

I suspect nobody will argue against that. ;-)