Roslyn: [Design Proposal] Analyzer warning waves

[mavasani]

Feature request

Allow analyzer assemblies, especially the ones that ship with some SDK instead of a NuGet package, to define and participate in _analyzer warning waves_ which control the set of analyzer diagnostics that get enabled by default. This allows newer versions of the SDK to ship with newer analyzer assemblies with additional analyzer diagnostics that stay disabled by default until the SDK or the end user explicitly bumps up the analyzer warning wave number, at which point the additional analyzers light up as enabled by default.

Implementation proposal

1. Define a new MSBuild property, say AnalyzerWarningWaves, to specify a list of key-value pairs in the format <%warningWaveId%>=<%level%>, where the warningWaveId is a string identifier and level is the corresponding warning wave number. For example:

<PropertyGroup>
  <AnalyzerWarningWaves>DotNetAnalyzersWarningWave=2; CSharpLanguageAnalyzersWarningWave=3; SomeThirdPartyAnalyzerWarningWave=4</AnalyzerWarningWaves>
</PropertyGroup>

1. Define an analogous compiler command line switch /analyzerWarningWaves=<%value%>, that parses these warning waves into a Dictionary<string, int> and passes it down to the analyzer driver.
2. Each DiagnosticDescriptor reported from DiagnosticAnalyzer.SupportedDiagnostics must be able to declaratively opt into warning waves. I can think of couple of ways to do this, with the first one being my preference:
   
   
   
   1. DiagnosticDescriptor.CustomTags: Each DiagnosticDescriptor must define a custom tag in a specific chosen format to declare its minimum warning wave level, say AnalyzerWarningWaves:WaveId=1, where WaveId is a string identifier for the warning wave ID and integral suffix after = is the associated warning wave level.
   
   
   2. Add a new constructor overload to DiagnosticDescriptor that take a new optional parameter Dictionary<string, int> analyzerWarningWaves to specify the warning waves. This approach has the disadvantage of unnecessarily polluting the public API surface of DiagnosticDescriptor for a very niche scenario, while also forcing the analyzer authors to move to a newer compiler API to get this functionality, which is not required in previous approach.
   
   
3. Update AnalyzerDriver's logic that determines if an analyzer is suppressed or not to take the analyzerWarningWaves dictionary. A DiagnosticDescriptor that is marked as enabled by default will be disabled if it has opted into a specific analyzer warning wave and either that warning wave ID entry is missing from analyzerWarningWaves or it's value is lesser then the warning wave level that the descriptor specified.
4. Analyzer driver must also handle the case where an analyzer defines multiple diagnostics descriptors with different warning levels, by enabling the analyzer if at least one descriptor is enabled and then filtering out the individual diagnostics reported by the analyzer based on analyzer warning levels.

New meta-analyzers proposal

In order to help analyzer authors opt into analyzer warning waves correctly, we can author new meta-analyzers (analyzers written for analyzers). Analyzer warning waves tracking is very similar to public API tracking, with some shipped warning waves and current unshipped warning wave. We can write an analyzer/fixer analogous to public API analyzer as follows:

1. Analyzer looks for additional files named AnalyzerWarningWaves.Shipped.txt and AnalyzerWarningWaves.Unshipped.txt
2. Each warning waves file specifies sections with <%warningWaveID%> = <%level%> as the header and the list of diagnostic IDs that opt into that warning wave with the specified level in the subsequent lines.
3. A diagnostic is raised if unshipped file defines any section with warningWaveId whose level is less then equals the maximum level for that warningWaveId defined in the shipped file, if any.
4. A diagnostic/code fix is provided to ensure that every new diagnostic ID gets added to AnalyzerWarningWaves.Unshipped.txt.
5. A diagnostic is raised if the warning wave level specified in the DiagnosticDescriptor does not match the entry in AnalyzerWarningWaves shipped or unshipped files.
6. Analyzer author moves all the entries from AnalyzerWarningWaves.Unshipped.txt into AnalyzerWarningWaves.Shipped.txt when they are ready to ship and close out a specific warning wave.

[mavasani]

Tag @dotnet/dotnet-analyzers @gafter @agocke

[tmat]

A couple of details:

re [1]: I think item group is a better representation than a property in msbuild for the wave configuration:

<ItemGroup>
  <AnalyzerWarningWave Include="DotNetAnalyzersWarningWave" Wave="2"/>
  <AnalyzerWarningWave Include="CSharpLanguageAnalyzersWarningWave" Wave="3"/>
  <AnalyzerWarningWave Include="SomeThirdPartyAnalyzerWarningWave" Wave="4"/>
</ItemGroup>

re [3]: Does a single diagnostic need to specify more than one wave? What's the scenario?
re [3.i]: I don't like adding it to tags. Tags should imo be opaque strings, not structured data.

while also forcing the analyzer authors to move to a newer compiler API to get this functionality

Why would that be a problem? These will be new analyzers anyways, right?

[mavasani]

@tmat Your proposal sounds good to me - I was trying to be pretty generic while also avoiding public API changes, but that is not required.

[agocke]

My understanding was that analyzers would be target framework linked. Why do we need a separate key to enable the warnings?

[mavasani]

Tagging @terrajobst, who just sent out an email on why we don't want the analyzers to be linked to target framework, but instead have a separate "analyzer waves" property that the SDK defaults and users can override and set as appropriate for them.

[agocke]

Just read it. The AnalyzerWarningWaves property seems simple enough. Can we do this without touching the analyzer driver? The complexity of the silencing analyzer warnings is too high. We need heavily resist more customization options here.

[mavasani]

The AnalyzerWarningWaves property seems simple enough. Can we do this without touching the analyzer driver

I am not sure what is the alternate proposal. Currently, only the analyzer driver understands if a specific diagnostic descriptor or analyzer is suppressed. IMO it would need a trivial code change in existing logic to determine isSuppressed by also accounting for the new custom tags/descriptor property: https://github.com/dotnet/roslyn/blob/69b3fc5b8851afaa0d8c22309e2b8409780dd78a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.cs#L316-L317

[agocke]

Well, one way would be to put the filtering logic in MSBuild, so analyzers with a lower warning wave aren't even added to the list, instead of trying to filter them out later.

[mavasani]

Well, one way would be to put the filtering logic in MSBuild

Sorry I am still not sure I complete understand the proposal - are you suggesting the command line will have a /nowarn for each new analyzer that needs to be turned off based on the analyzer wave level?

[agocke]

No, the analyzer wouldn't be passed in /analyzer

[mavasani]

No, the analyzer wouldn't be passed in /analyzer

So, then we need a separate analyzer assembly for each warning wave level? Otherwise, how will this be end user configurable to allow them to override to a specific warning wave level of their choice?

[terrajobst]

So, then we need a separate analyzer assembly for each warning wave level?

Yeah, let's not. The one thing we don't need is more assemblies floating through our system.

[terrajobst]

@tmat

re [1]: I think item group is a better representation than a property in msbuild for the wave configuration:

<ItemGroup>
  <AnalyzerWarningWave Include="DotNetAnalyzersWarningWave" Wave="2"/>
  <AnalyzerWarningWave Include="CSharpLanguageAnalyzersWarningWave" Wave="3"/>
  <AnalyzerWarningWave Include="SomeThirdPartyAnalyzerWarningWave" Wave="4"/>
</ItemGroup>

That's reasonable but we shouldn't assume that each wave has its own assembly. Also, I think we want to configure the default severity per wave. So maybe something like this?

<ItemGroup Condition="'$(WarningWave)' >= '2'">
  <DiagnosticId Include="CA123" Severity="Error" />
  <DiagnosticId Include="CA124" Severity="Warning" />
  <DiagnosticId Include="CA125" Severity="Warning" />
  <DiagnosticId Include="CA126" Severity="None" />
</ItemGroup>
<ItemGroup Condition="'$()' >= '3'">
  <!-- Lower to Warning -->
  <DiagnosticId Include="CA123" Severity="Warning" />
  <!-- Not needed because it's the same as in wave 2
  <DiagnosticId Include="CA124" Severity="Warning" />
  -->
  <!-- Disable -->
  <DiagnosticId Include="CA125" Severity="None" />
  <!-- Enable -->
  <DiagnosticId Include="CA126" Severity="Warning" />
</ItemGroup>

[tmat]

BTW, could we drop "analyzer" from the AnalyzerWarningWaves and use this to specify compiler warning waves as well? The feature that @gafter has been looking into?

[terrajobst]

could we drop "analyzer" from the AnalyzerWarningWaves and use this to specify compiler warning waves as well?

See my email. @jaredpar mentioned that the compiler team thinks the compiler won't need it because they would do the warnings as analyzers.

[tmat]

That's reasonable but we shouldn't assume that each wave has its own assembly.

I don't think there is an assumption like that. One analyzer can categorize their diagnostics into multiple groups, each identified by a distinct -WarningWave identifier that the analyzer reports from DiagnosticDescriptor.

[tmat]

Also, I think we want to configure the default severity per wave.

This feels like too much. What is the scenario?

[terrajobst]

This feels like too much. What is the scenario?

The idea is that each wave is stable with respect to the diagnostic output it produces. Same sources, same diagnostics. We'll do our best to define a good set of defaults for our customers, but we'll want to tweak that based on customer feedback. For example, we might want to be conservative and ship a rule initially in an off state and enable it later. Or we find that one rule is too aggressive/noisy and want to turn it off. If we have to change the diagnostic descriptor than you'd change it for all warning waves the rule appears in.

[mavasani]

The idea is that each wave is stable with respect to the diagnostic output it produces. Same sources, same diagnostics. We'll do our best to define a good set of defaults for our customers, but we'll want to tweak that based on customer feedback. For example, we might want to be conservative and ship a rule initially in an off state and enable it later. Or we find that one rule is too aggressive/noisy and want to turn it off. If we have to change the diagnostic descriptor than you'd change it for all warning waves the rule appears in.

This seems like basically we need a special ruleset file per each warning wave, which makes me think of another proposal that might be more closer to @agocke's preference of avoiding changes to analyzer driver.

1. In the analyzers repo, we can generate a specific ruleset file for each analyzer warning wave and embed it in the analyzer NuGet package, which get pulled into the SDK. We already have the tooling in the repo to auto-generate such rulesets for analyzer assemblies based on specific criteria.
2. SDK can define an MSBuild target that maps a given AnalyzerWarningWave value to a corresponding ruleset file. AnalyzerWarningWave is defaulted by the SDK and can be overridden by end users:

<Target Name="ComputeMappedRuleset">
  .. Logic to compute 'MappedRuleset' ...
</Target>
<ItemGroup>
  <AdditionalRulesets Include="$(MappedRuleset)"/>
</ItemGroup>

1. We add a new compiler command line switch /additionalRulesets:<%list_of_path_to_rulesets%> to pass these additional rulesets down to the compiler. By having this switch accept multiple additional rulesets instead of one serves couple of purposes:
   
   
   
   1. We can define separate warning waves per analyzer buckets, like say DotNetAnalyzersWave, CSharpLanguageAnalyzersWave, SomeThirdPartyAnalyzersWave, etc.
   
   
   2. This serves a long outstanding request from analyzer authors to allow ability to embed additional rulesets in their NuGet packages, which apply some base configurations which can be overridden by end user in user ruleset.
   
   
   3. We can completely address https://github.com/dotnet/roslyn/issues/42166 by either emitting separate additional rulesets for IDE and command line builds OR just passing in an extra additional ruleset that turns off suggestions from command line build.
   
   
2. Command line compiler computes Compilation.SpecificDiagnosticOptions by first applying these additional rulesets and them applying the user provided ruleset, if any. Should be as simple as extracting out the below code for each ruleset: https://github.com/dotnet/roslyn/blob/ca1a68cd5edd5727ec27b584b4dd72f782923aa3/src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs#L150-L158

[mavasani]

Couple of additional things to note:

1. On second thought, the above proposal can likely be implemented using additional or base _editorconfig files_ instead of rulesets. This would then also act as a way to supply compilation level fallback editorconfig file(s), which has been a requested feature to close the ruleset gap. We likely only apply severity configuration based entries from such an editorconfig to compilation level diagnostic options.
2. This feature can be also used to support the "Category" opt-in scenario we discussed few times, where users decide to opt into turning on all analyzer warnings for specific categories, such as security, performance, etc. Our analyzer package can spit out these additional _enabling_ rulesets/editorconfigs for each category. User can opt to turn on all rules from a specific category by setting some property, and an MSBuild target can append these additional enabling rulesets/editorconfigs to the command line.

[terrajobst]

@mavasani

In the analyzers repo, we can generate a specific ruleset file for each analyzer warning wave and embed it in the analyzer NuGet package, which get pulled into the SDK.

I like the idea of using a file format that was designed to handle this, instead of using MSBuild. But do you envision this generation to happen in every build or does this only happen when we build the SDK itself? IOW, these are static files on the customer's machine, right?

We can define separate warning waves per analyzer buckets, like say DotNetAnalyzersWave, CSharpLanguageAnalyzersWave, SomeThirdPartyAnalyzersWave, etc.

Why do we need multiple waves? Can't just have one version number in the SDK that all the built-in analyzers are related to?

On second thought, the above proposal can likely be implemented using additional or base _editorconfig files_ instead of rulesets.

I was about to ask that. Seems editor.config is the way to go.

[mavasani]

But do you envision this generation to happen in every build or does this only happen when we build the SDK itself?

We generate these configuration files (rulesets and editorconfigs) as part of every CI build and embed it into all the analyzer NuGet packages produced in the repo. Documentation of these configuration files for FxCopAnalyzers package is at https://docs.microsoft.com/en-us/visualstudio/code-quality/analyzer-rule-sets. Similar files are generated for https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.NetAnalyzers. I presume SDK can just pull in all required configuration files directly from the NuGet package and embed it statically.

IOW, these are static files on the customer's machine, right?

Yes, correct.

Why do we need multiple waves? Can't just have one version number in the SDK that all the built-in analyzers are related to?

I would let you and Jared decide upon whether it might be a valid scenario for customers to opt into different warning waves for platform analyzers and language analyzers. If not, it seems fine to have a single one. I was hoping the overall design can be flexible to allow any other SDK with similar requirements as ours to be able to plug into this model and have their own warning waves (for example ASP.NET analyzers, threading analyzers etc. which ship in their own SDKs should probably also switch to this model).

I was about to ask that. Seems editorconfig is the way to go.

Yeah, I agree but @agocke should be the one to make a call here.

[agocke]

Yeah editorconfig seems good too. Jared and I were previously talking about a "global" editorconfig for exactly these type of scenarios (you want to include an editorconfig for user code in your NuGet package).

It obviously needs to be designed, but it would be useful.

[terrajobst]

I would let you and Jared decide upon whether it might be a valid scenario for customers to opt into different warning waves for platform analyzers and language analyzers. If not, it seems fine to have a single one. I was hoping the overall design can be flexible to allow any other SDK with similar requirements as ours to be able to plug into this model and have their own warning waves (for example ASP.NET analyzers, threading analyzers etc. which ship in their own SDKs should probably also switch to this model).

I think it depends on what the warning wave is. One way to do it is making it the major.minor of the SDK itself. This way, it's a number customers can reason about and we don't have to perform miracles to maintain the number. Then I think letting 3rd parties use that too is sensible. We most certainly shouldn't include the 3rd version number because that is tied to VS trains and nobody understands that. Flip side is that we can only add/remove/change verbosity in major.minor changes of the SDK, but that doesn't seem too restrictive IMHO.

[jmarolf]

What we want is to be able for the .NET org to dogfood analyzer waves in addition to making it easy for customers to onboard. We've all agreed that the default, simplest, way for users to get an analyzer wave is to have them auto-opted-in via their target framework (TFM). This means that for the majority of users, when they ask us why they are getting a specific warning we are going to ask what TFM they are targeting. To give us this information they will need to look at their project files.

If project files are already a thing that users will need to be aware of to understand analyzer warning waves (and I think this a good thing since TFM and language version work the same way today) I would argue that whatever mechanism we are going to have the user configure should also exist in the project file.

re [1]: I think item group is a better representation than a property in msbuild for the wave configuration:

@tmat I would need more explanation here. Users currently don't need to add items to their projects in .NET Core for almost every scenario as globbing takes care of that. I don't think asking users to add an item group (when they _must_ have a property group) is the most user friendly design.

Before we go off figuring out how we are going to implement this I would like to talk about the user facing experience and design that a bit.

<ItemGroup>
  <AnalyzerWarningWave Include="DotNetAnalyzersWarningWave" Wave="2"/>
  <AnalyzerWarningWave Include="CSharpLanguageAnalyzersWarningWave" Wave="3"/>
  <AnalyzerWarningWave Include="SomeThirdPartyAnalyzerWarningWave" Wave="4"/>
</ItemGroup>

In the example above we are talking about calling this feature AnalyzerWarningWave and including a name and a wave number. I would like to point out that this feature is explicitly _not_ about just warnings but about setting the defaults for a whole suite of analyzers, some of which will be warning, suggestion, etc. So having warning wave in the title is getting us into NullableContextOptions territory. I also don't want to keep a table explaining what wave 2 represents. We already have enough of those in the platform. I would propose that while we shouldn't tie this feature to TFM we can use TFMs as the entry point to explain it to users.

Below is a proposal where we remove WarningWave from the name. I am certain this is not going to be the final name of the option but lets try it on for size :)

We also could use the tfm to indicate the warning wave. I like this approach because when a user says "I want to target a newer framework but keep my old warnings" I can tell them to just specify their old TFM.

Defaults

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <!--Implicitly opting into a set of analyzers --> 
    <TargetFramework>net5</TargetFramework>
  </PropertyGroup>

</Project>

Opting into different platform warning levels

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <!--Opting into newer analysis wave that comes from the sdk-->
    <AnalysisLevel>net5</AnalysisLevel>
  </PropertyGroup>

</Project>

Opting into different buckets of analyzers

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <!--Opting into newer analysis wave that comes from the sdk-->
    <AnalysisLevel>net5</AnalysisLevel>
    <!--Opting into an analysis wave that flags performance errors -->
    <AnalysisLevel>net5-performance</AnalysisLevel>
     <!--Opting into an analysis wave that flags localization errors -->
    <AnalysisLevel>net5-localization</AnalysisLevel>
  </PropertyGroup>

</Project>

Opting into the latest

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <AnalysisLevel>latest</AnalysisLevel>
    <AnalysisLevel>latest-performance</AnalysisLevel>
    <AnalysisLevel>latest-localization</AnalysisLevel>
  </PropertyGroup>

</Project>

opting into all analysis

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <AnalysisLevel>net5-all</AnalysisLevel>
  </PropertyGroup>

</Project>

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <AnalysisLevel>latest-all</AnalysisLevel>
  </PropertyGroup>

</Project>

Potential Problems with this design

Q: How will analyzer authors be able to add their own warning waves?

I am going to say up-front that I don't think this is necessary. I think the implementation should allow the analyzer to know what the currently active warning wave is, but I am not convinced it will be common (or ever occur) that an analyzer author will need to define their own warning wave. I also thing this makes the story more complex for the average user.

Q: how will this play with .editorconfig?

I am envisioning .editorconfig (that a user specifies, we may implement this feature by passing in an .editorconfig) will layer on top of whatever the analyzer warning wave specifies. The warning wave if the defaults and then the user is free to specify whatever they want on top of that. Presumably they should be able to turn on an analyzer assembly that is disabled by their currently active warning wave via .editorconfig.

Other things we should discuss

I am talking about manipulating the project file for all this since that is how we've delt with cases like this before (TargetFramework, LangVersion, etc.) but for all of these cases we have some sort of designer support in the project property pages. Would this also require work there as well?

[mavasani]

@jmarolf I believe all the concerns you have raised were based on my initial proposal, to which @tmat proposed changes. If we instead go with the second proposal as mentioned in https://github.com/dotnet/roslyn/issues/42198#issuecomment-595437894, then almost all of your concerns go away:

1. Compiler needs to implement a new command line switch to provide fallback or compilation level editorconfig files, which are directly reflected in CompilationOptions.SpecificDiagnosticOptions
2. Each SDK that wants to support warning waves for analyzers that they ship, need to define their own MSBuild targets and properties - they can have their own custom logic to map those property values to relevant editorconfig files that can be passed to the compiler. More specifically, for .NET5 SDK, we are free to define whatever property we wish, with whatever format we decide for the warning values and have our own mapping mechanism to map to the relevant editorconfig files to pass to the compiler.

I think we should use this issue to only track the design and implementation of compiler work in (1), as that is the only functionality that we need in Roslyn. Once we have this functionality, we can implement the rest of the logic natively inside the SDK targets and can also address all the other open issues outside of Roslyn code base (easy category based opt-in, https://github.com/dotnet/roslyn/issues/42166, etc.). We should move rest of the design questions that you raised to a separate issue in SDK repo.

[jmarolf]

I bring it up because I think it will inform some of our implementation decisions.

As I understand it, the amended proposal would require each analyzer nuget package to define a set of .editorconfig files that it passes to the compiler. Which .editorconfig is passed in depends on the props/targets that are included in the nuget package.

Perhaps I am mis-understanding something but this means that MSBuild is still responsible for computing which analyzers are active and passing it to the compiler. Why not simply pass in the set of analyzer dlls and the diagnostic ids with their severity. Why use .editorconfig at all for this? Isn't it just extra complexity for the compiler?

[mavasani]

I have extracted out the core Roslyn feature request into a separate issue: #42219. I think it might be best to pull out all the SDK property/versioning based discussions into a separate issue in SDK repo and close this issue.

Why not simply pass in the set of analyzer dlls and the diagnostic ids with their severity

The only way that diagnostic severities can be explicitly configured today are via editorconfig files (and rulesets as legacy). I think we should stick with all diagnostic severity configurations to be designed from editorconfig files. Designing a yet another mechanism to supply diagnostic IDs with their severity to compiler sounds redundant. Any case, please feel free to add your suggestions to #42219

[jmarolf]

On further introspection I agree @mavasani. The compiler already tricked the compiler team into implementing editorconfig support so we might as well capitalize on it :)

[mavasani]

I am going to close this issue as the original proposal is no longer valid and the compiler feature request has already been split into a separate issue.

@terrajobst @mikadumont @jmarolf - it would be good to file a separate issue on the SDK repo for design of the user facing MSBuild properties and experience for configuring .NET platform warning waves and opt in/out from specific category buckets. The proposal from Jon here could be good starting point for this issue.

[mikadumont]

Update to @jmarolf's proposal:

New .NET 5 projects
A user creates a new .NET 5 project which will automatically have the SDK analyzers included by default:

<!--Implicitly opting into a set of analyzers -->
<TargetFramework>net5.0</TargetFramework>

Existing .NET projects targeting TFM before .NET 5
If a user has an existing project that does not target .NET 5 or newer they will not get any SDK analyzers included in their project. However, a user can get the SDK analyzers by using the MSBuild property:

<TargetFramework>netcoreapp3.1</TargetFramework>
<!--Opting into newer analysis wave that comes from the sdk-->
<!—We assume that SDKs ship more often than we version the TFM-->
<AnalysisLevel>5.1</AnalysisLevel>

Other analysis level options:

<!—Latest version, even if in preview-->
<AnalysisLevel>preview</AnalysisLevel>

<!—Latest release version, i.e. not in preview-->
<AnalysisLevel>latest</AnalysisLevel>