add .NET analyzers to the sdk

[jmarolf]

This copies a the basic approach that the WebSDK took for adding analyzers.

The Goal:

Resolves roslyn-analyzers#3667

This PR:

1. Adds a new dependency on the Microsoft.CodeAnalysis.NetAnalyzers nuget package that comes from dotnet/roslyn-analyzers (for more into on what analyzers are included see here)
2. includes the analyzers from this nuget package in the SDK via a process reminiscent of how roslyn is added. via copying them into a well-known folder in the SDK
3. Adds targets to implicitly add these analyzers iff
   1. The User is targeting net5 or newer
   2. The user has not set <DisableNetAnalyzers>true</DisableNetAnalyzers>
4. Set a property <UsingNETAnalyzers>true</UsingNETAnalyzers> iff analyzers are implicitly added so we can track this information

[jmarolf]

CC: @dotnet/dotnet-analyzers

[jmarolf]

alright @dsplaisted @wli3 @sfoslund (sorry I couldn't find and alias). This is ready for review. Let me know if you have any objections to the approach here.

[sfoslund]

Looks good to me overall

[dsplaisted]

I think it would be a good idea to have a design in place for how we're going to handle versioning / compatibility of these analyzers (sounds like maybe that's called analysis level) over time. Otherwise we can't add any new analyzers (which produce warnings at least) until we work that out.

In the analyzer layout under Microsoft.NET.Sdk/analyzers, there are cs and vb folders, and both have a Microsoft.CodeAnalysis.NetAnalyzers.dll file, as well as all its localized resources, which add up to close to 5 MB. Can we use a layout where this isn't duplicated across two folders for the different languages? Could we just put both the VB and C# analyzer DLLs in the same folder?

Looking at the NetAnalyzers props and targets from the package:

• It adds .editorconfig to AdditionalFiles if it's in the project directory. What's the reason for this? Isn't .editorconfig also supposed to work anywhere up the directory tree (e.g. in the solution directory or repo root)?
• It adds flow-analysis to the Features property: <Features>$(Features);flow-analysis</Features>. What is this for? I would not recommend using a very generic property name such as Features for a Roslyn-specific property.
• What does the AddGlobalAnalyzerConfigForPackage target do? It looks like it uses a property that's not set anywhere (MicrosoftCodeAnalysisNetAnalyzersRulesVersion), and refers to a config directory that doesn't exist.

[mavasani]

• It adds .editorconfig to AdditionalFiles if it's in the project directory. What's the reason for this? Isn't .editorconfig also supposed to work anywhere up the directory tree (e.g. in the solution directory or repo root)?

This will be removed in the package. This was added as a workaround to allow editorconfig based options in analyzers prior to the compiler support for editorconfig. I will create a PR on analyzers repo to remove it.

• It adds flow-analysis to the Features property: <Features>$(Features);flow-analysis</Features>. What is this for? I would not recommend using a very generic property name such as Features for a Roslyn-specific property.

This will be removed in the package. This was added when compiler flow analysis APIs were under a feature flag. I will create a PR on analyzers repo to remove it.

• What does the AddGlobalAnalyzerConfigForPackage target do? It looks like it uses a property that's not set anywhere (MicrosoftCodeAnalysisNetAnalyzersRulesVersion), and refers to a config directory that doesn't exist.

This target generates global editorconfig files, which was recently implemented in the compiler with dotnet/roslyn#42219. Global editorconfig files are generated for the analyzer warning waves feature. Each warning wave will have a corresponding global editorconfig file to allow only specific set of analyzers to be enabled with the warning wave. More details in PR descriptions: dotnet/roslyn-analyzers#3462 and dotnet/roslyn-analyzers#3538.

Currently there are no warning waves needed for any prior release of Microsoft.CodeAnalysis.NetAnalyzers, so the package has no global editorconfig files. Once we have the first net5 release, the package will have a global editorconfig file, and the MicrosoftCodeAnalysisNetAnalyzersRulesVersion can be set appropriately in the SDK repo based on user's chosen warning wave to pass this editorconfig file into the compiler.

• Warning wave "latest": no global editorconfig, default analyzer severities of analyzers in the package are used OR
• Warning wave "net5": mapped to value for MicrosoftCodeAnalysisNetAnalyzersRulesVersion that enables passing in "net5" specific global editorconfig file to only enable the analyzers shipped in net5 release, any new analyzers in the current package added after net5 release will be disabled by default.

For example, build.zip would represent the build folder of the analyzer package, which includes the config folder with a global editorconfig file per warning wave.

[mavasani]

I have filed dotnet/roslyn-analyzers#3670 to track all the follow-up items to clean up the props and targets files in the Microsoft.CodeAnalysis.NetAnalyzers package.

[mavasani]

dotnet/roslyn-analyzers#3671 to cleanup the props and targets files.

[mavasani]

@jmarolf To enable global editorconfig/warnings wave support, we will need to do the following changes:

1. Add logic in this repo to map from user facing AnalysisLevel warning wave property to the MicrosoftCodeAnalysisNetAnalyzersRulesVersion property, which is basically the release version of Microsoft.CodeAnalysis.NetAnalyzers.dll that ships with that warning wave.
2. Include build\configs folder in the SDK alongside build\Microsoft.CodeAnalysis.NetAnalyzers.targets that will pick the correct global editorconfig from the configs folder based on MicrosoftCodeAnalysisNetAnalyzersRulesVersion property value and pass it to the compiler.

I believe these changes can be done in a follow-up PR, we probably need tests to ensure warning waves feature with global editorconfig works as expected and does not regress.

[jmarolf]

@dsplaisted and @sfoslund thanks so much for having a look!

@dsplaisted regarding this:

I think it would be a good idea to have a design in place for how we're going to handle versioning / compatibility of these analyzers (sounds like maybe that's called analysis level) over time.

New warning (things that can break your build) can only be introduced in major release versions of .NET. The expectation is that the AnalysisLevel (or whatever we call it) will allow users to select a set of warning to enable/disable in accordance with a major release version of .NET. I would be strongly against us introducing new warnings in SDK updates. We will be responsible for setting of the validation to ensure that new warnings sneaking in is not a thing that can happen. However, bugfixes for existing rules I would expect to ship with sdk updates depending on the bugs severity.

[jmarolf]

I currently consider this PR blocked until dotnet/roslyn-analyzers#3671 is merged. I will clean up the PR based on everyone's suggestions in the meantime

[dsplaisted]

1. Add logic in this repo to map from user facing AnalysisLevel warning wave property to the MicrosoftCodeAnalysisNetAnalyzersRulesVersion property, which is basically the release version of Microsoft.CodeAnalysis.NetAnalyzers.dll that ships with that warning wave.

@mavasani @jmarolf What will the valid values of AnalysisLevel be? It sounds like you're saying net5 and latest, but I'm not sure tying it to a .NET version is the best. Would the C# language version make more sense for a user-facing property someone is going to set?

As far as the logic that will be removed from the package, we don't necessarily need to block this PR on that, as it will happen automatically once the updated analyzers package flows into the SDK.

[jmarolf]

but I'm not sure tying it to a .NET version is the best

I am not particular. At this point TFM and C# language version always rev at the same times. C# 9 is only available in .NET 5. C# 10 will only be available in .NET 6. We can pick either one as the thing to tie analyzer defaults to.

CC: @jaredpar @terrajobst and @mikadumont if they have any initial thoughts, but honestly I haven't fully designed this so I am open to any feedback here. tying warning waves to language version instead of TFM would have the nice effect that folks could use it in .NET Standard 2.0 if they really wanted to by manually changing the TFM.

[jmarolf]

What will the valid values of AnalysisLevel be?

These are my initial thoughts but its only a rough sketch: dotnet/roslyn#42198 (comment)

As you say I think we will have net5/csharp9 and latest. I also think there will be some additional added in the future but I do not think those need to be in the initial preview

[jaredpar]

We can pick either one as the thing to tie analyzer defaults to.

Analyzers are language agnostic hence tying them to the C# version seems like the incorrect approach. Even if you made each analyzer language specific and tied them to the appropriate language that is still a blocker because the VB language version isn't changing going forward.

[jmarolf]

VB language version isn't changing going forward

ah, right I forgot why I originally chose TFM weeks (months?) ago. All other details aside. I want the analyzers to feel like they are a part of the platform. The hope is that the user upgrades their TFM and sees new warning.

[dsplaisted]

The problem with using the TargetFramework is that the tooling doesn't version exactly with the runtime. So theoretically there could be a new language version and new analyzers without a new TargetFramework.

I do think that the TargetFramework should be used to infer the default analysis level. What I was questioning was whether it made sense to use something else as the user-facing property if you want to override it.

It sounds like there may not really be any better choice than TargetFramework, though.

[jaredpar]

I do think that the TargetFramework should be used to infer the default analysis level. What I was questioning was whether it made sense to use something else as the user-facing property if you want to override it.

That is the vision I had in mind.

This is essentially what C# does. The user can always use <LangVersion> to concretely specify the language version they want. However lacking an explicit user decision we will infer one from the Target Framework.

[mavasani]

I do think that the TargetFramework should be used to infer the default analysis level. What I was questioning was whether it made sense to use something else as the user-facing property if you want to override it.

I think we all agree that TargetFramework is reasonable to infer the default analysis level. Wanted to also point out @terrajobst's suggestion as a possible option for the user-facing property: dotnet/roslyn#42198 (comment)

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.