Add support for respecting custom configured severity settings in editorconfig

[mavasani]

Overview of the issue

Current IDE code style options support the following editorconfig syntax:

option_name = option_value:severity

For example:

[*.cs]
dotnet_style_qualification_for_field = true:warning

Command line compiler supports configuring severity of a specific diagnostic ID with the following editorconfig syntax:

dotnet_diagnostic.RuleID.severity = severity

This former syntax (option_name = option_value:severity) allows configuring both code style option value and severity. However, the severity setting from this syntax is only respected when executing the corresponding analyzer in the IDE live analysis. This syntax is not recognized by the command line compiler and/or the analyzer driver, so the severity setting above is redundant when executing from an explicit command line build. Additionally, with the command line compiler optimization implemented in #43546, we no longer execute hidden/suggestion analyzers in command line builds, unless the user has explicitly specified the analyzer diagnostic ID as a warning or error with the dotnet_diagnostic.RuleID.severity = severity syntax. All IDE code style rules are hidden or suggestions by default. This essentially means that the only way to enforce an IDE code style rule on command line build from our CodeStyle NuGet package is by using the latter compiler recognized syntax.

#44201 suggested tackling this issue by deprecating the :severity suffix in option_name = option_value:severity syntax. However, there has been customer pushback on this as almost all existing editorconfigs out there which have IDE code style option entries use :severity. Note that :severity suffix was mandatory in this syntax up until the end of the last year, when we made it optional. This presents another case that uses will face a hard time figuring out why IDE code style option entries that specify :warning or :error doesn't cause them to be executed on command line builds.

Original implementation proposal

The feature request here is for the compiler/analyzer driver to support decoding the :severity suffix in editorconfig entries with format option_name = option_value:severity. Additionally, the compiler will need to add some new API/mechanism for diagnostic analyzers to specify a mapping from its supported option_names to diagnostic ID. There are two possible ways that come to my mind for this:

1. Add new overloads for DiagnosticDescriptor constructor that take a string[] optionNames or ImmutableArray<string> optionNames parameter, and the analyzer driver can create a option_name <-> diagnosticId mapping
2. Allow the existing customTags parameter on the DiagnosticDescriptor ctors to have a special format to specify supported option names, say option_names = name1;name2;name3

Latest implementation proposal

See comment below: #52991 (comment)

Public API proposal corresponding to the latest implementation proposal

See comment below: #52991 (comment)

[mavasani]

Tagging @jaredpar @chsienki from the compiler/editorconfig side.
Tagging @jmarolf @CyrusNajmabadi @sharwell from the IDE side.

Jared/Chris - for context, we discussed this issue in last week's IDE design meeting and came up the conclusion of writing a proposal for the compiler feature request and then scheduling a quick sync-up meeting between all of us to get compiler team's opinion. I'll start an email thread and setup an appropriate meeting.

[mavasani]

Ah, forgot to tag @jasonmalinowski who did most of the IDE work for editorconfig support.

[Youssef1313]

I'd say to consider choosing a design that makes it easier in the future to allow third-party analyzers to use this syntax as well.

[mavasani]

@Youssef1313 Absolutely - my proposal above does not restrict this feature to any specific analyzer package.

[jmarolf]

1. Add new overloads for DiagnosticDescriptor constructor that take a string[] optionNames or ImmutableArray optionNames parameter, and the analyzer driver can create a option_name <-> diagnosticId mapping
2. Allow the existing customTags parameter on the DiagnosticDescriptor ctors to have a special format to specify supported option names, say option_names = name1;name2;name3

really like this proposal @mavasani

[mavasani]

We discussed this amongst the compiler and IDE teams and identified that the original proposal in this issue does not meet all requirements. Specifically, for naming style analyzers that use dynamically generated editorconfig key names for setting severity, analyzers cannot statically report all its supported editorconfig keys in its descriptor. We decided to go with @sharwell's proposal instead:

1. All IDE code style analyzers will enable themselves by default in command line builds by reporting a descriptor with warning severity. This will prevent the analyzer driver from completely filtering them out from execution in command line builds.
2. Expose AnalysisContext.MinimumReportedSeverity on the analysis context. This will be warning for command line builds, and hidden for IDE case.
3. IDE code style analyzers will use the MinimumReportedSeverity and option_name = option_value:severity entries in editorconfig to decide if they should execute or turn off completely. This way the compiler does not need to decode these special entries and severity from option_name = option_value:severity entries are also respected in command line builds.

[jcouv]

Adding a note to record that I ran into this issue: the compiler layer is currently not sufficiently enforcing naming convention on local functions. (here's most recent relevant change)
Thanks

[333fred]

API Review

• Generally looks good
• Let's rename CustomConfigurable to CustomSeverityConfigurable to be more specific about what is configurable

Conclusion : Approved

[jaredpar]

Won't supporting this syntax in the compiler represent a potential breaking change? Essentially customers today have .editorconfig settings that produce say warnings in the IDE but not the command line compiler cause it's ignored. Once these settings are supported though the command line compiler would support them and builds that didn't warn before will potentially start warning?

If that analysis is correct then we need to think about an opt-in mechanism, probably tied to TFM, to enable this. Risky to be on by default.

[mavasani]

Won't supporting this syntax in the compiler represent a potential breaking change? Essentially customers today have .editorconfig settings that produce say warnings in the IDE but not the command line compiler cause it's ignored. Once these settings are supported though the command line compiler would support them and builds that didn't warn before will potentially start warning?

If that analysis is correct then we need to think about an opt-in mechanism, probably tied to TFM, to enable this. Risky to be on by default.

This is correct analysis. It seems fine to make this opt-in and tie it to say an MSBuild property that can be explicitly enabled by the user OR implicitly enabled with a minimum value of AnalysisLevel (TFM essentially). When enabled, we emit a special entry in our tooling generated globalconfig that passes MSBuild properties to analyzers, and have the CodeStyle analyzer package respect this option to turn on the new functionality. Does that sound like a reasonable approach?

[mavasani]

@jaredpar Please see draft PR #70727 for the recommended opt-in proposal for this feature via a new MSBuild property, EnforceCodeStyleSeverityInBuild. I verified that globalconfig option with key build_property.EnforceCodeStyleSeverityInBuild gets passed to the analyzers with that PR, and I can update #69844 to pull in the commits from that PR and enable this new functionality only if that property is true. Let me know if that addresses your concerns. Thanks!

[mavasani]

Filed dotnet/docs#38540 to update docs for this support.

Before #69844 got merged, the severity specified in option_name = option_value:severity was only respected in the IDE live analysis, not on command line builds. The documentation correctly mentions this information.
However, with the above change, the severity specified in option_name = option_value:severity will get respected on build as well. Note that to prevent any breaking changes, this functionality is not enabled by default, unless the AnalysisLevel or AnalysisLevelStyle is greater than or equals 9.0. This functionality will get enabled by default if users set AnalysisLevel value to preview on .NET 8 SDK and also in the future when we release .NET 9 SDK and users migrate their project to net9 without setting any AnalysisLevel value - for net9 TFM we would default AnalysisLevel to 9.0 and hence this feature will get turned on by default.

[Youssef1313]

@mavasani What happens if there is a "conflict" in severity values? e.g, dotnet_diagnostic.ID.severity = some_severity and option_name_corresponding_to_ID = value:another_severity?

My understanding from the PR description is that option_name = value:severity will override the other value? I've seen many cases where people use option_name = value:severity with low severity and then using a higher severity when setting dotnet_diagnostic.ID.severity, so that is kind-of a regression for those expecting the analyzers to be running then silently they are no longer running?

Or am I misunderstanding something?

[mavasani]

@mavasani What happens if there is a "conflict" in severity values? e.g, dotnet_diagnostic.ID.severity = some_severity and option_name_corresponding_to_ID = value:another_severity?

My understanding from the PR description is that option_name = value:severity will override the other value? I've seen many cases where people use option_name = value:severity with low severity and then using a higher severity when setting dotnet_diagnostic.ID.severity, so that is kind-of a regression for those expecting the analyzers to be running then silently they are no longer running?

Or am I misunderstanding something?

@Youssef1313, yes we expect everyone to eventually use the code style option based syntax for value and severity configuration as that avoids the use of these opaque IDs for code styles. I assume we can write a simple analyzer to flag cases when we see conflicting entries like you mentioned above, but that needs support for reporting diagnostics in analyzer config files #64214

[Youssef1313]

Yes, having such analyzer is going to be perfect.

[tfadler]

@mavasani

You marked this as duplicated and pointed us here.

https://developercommunity.visualstudio.com/t/VS-2022-code-analysis-ignores-editorconf/10584696

I certainly can be missing something but I think what we are asking for (and worked prior) was the following

With options that affect more than one rule (like dotnet_style_coalesce_expression), we wanted to turn on the option and turn off one of the rules.

So we do what you said to do with
dotnet_style_coalesce_expression = true:error

But like IDE0029 and IDE0030

We want to turn off IDE0270
dotnet_diagnostic.IDE0270.severity = none

In the text in the linked issue it seemed to me to indicate we just need to move to
option_name = option_value:severity

but we are doing that.

I don't see how to cherry pick turning off a given rule by only referencing option_name when a given option can cover more than one rule.

If you do not intend to support this anymore, can you make that clear? Also, our point in the ticket was also we are seeing inconsistent behavior between inside the Visual Studio and command line building for this.

Again. I can be missing something. Thank you for your help and guidance.

[mavasani]

@tfadler Thank you for the clarification of your query, and let me try to be more precise here.

• The fact that you are seeing a change in default behavior with 17.9 is not intentional. We added guards for this feature to only turn on by default if you are targeting net9 or later OR have explicitly set AnalysisLevel to 9 or greater, neither of which should be happen by just upgrading the VS version to 17.9. It seems there is a bug in this feature implementation when you have a mix of severity settings from dotnet_diagnostic.IDExxxx.severity and option_name = option_value:severity for some overlap of code style option and IDExxxx rule, and that is leading to this change in default behavior. This regression is tracked with It seems dotnet_diagnostic.IDE0058.severity in .editorconfig started to be ignored with today's VS update (13 feb 2024) #72094. I will be prioritizing work on investigating this issue and hopefully have a fix out soon I have a PR out to fix this regression: Guard the 'CustomSeverityConfigurable' tag with AnalysisLevel 9.0 #72238.

• Having said the above, the core issue you are seeing above only comes if you have a mix of severity settings in your editorconfig from both these syntaxes. I believe my simplistic suggestion/workaround of just avoid all dotnet_diagnostic.IDExxxx.severity entries does not work for your scenario, so let me refine the suggestion. The correct approach/workaround here is to ensure that severity configuration for all related code style rules happens via just one kind of syntax, not using a mix of both. Let me try to expand for different cases of code style option(s) to IDExxxx rule(s) mappings:

  1. Code style option which has a 1:1 mapping with a single IDExxxx diagnostic ID: This is a simple case, you should just remove the dotnet_diagnostic.IDExxxx.severity entry and instead specify severity with option_name = option_value:severity. Specifying dotnet_diagnostic.IDExxxx.severity and option_name = option_value without a severity suffix should also work, but unnecessarily adds complexity to your editorconfig. We recommend the former approach.
  2. Code style option which has 1:many mapping with multiple IDExxxx diagnostic IDs: If you wish all the IDExxxx code style rules associated with this option to have the same severity, it is recommended you follow the same advice as prior case and use option_name = option_value:severity for setting severity. Otherwise, if you want a hybrid configuration where there are different configured severities for the mapped IDExxxx rules, you should configure the severities with dotnet_diagnostic.IDExxxx.severity and use option_name = option_value without any severity suffix to configure the code style option value.
  3. Code style options which have many:1 mapping to a single IDExxxx diagnostic ID: Similar to the first case, there are two possible ways to approach here, but we recommend you completely avoid dotnet_diagnostic.IDExxxx.severity entries and instead use option_name = option_value:severity for all the options, regardless of whether you want all these code style violations to have same severity or different ones
  4. Code style options which have many:many mapping to a multiple IDExxxx diagnostic IDs: I do not know if this is a valid scenario for any existing code styles. Please correct me if I am wrong