Add analyzer for banning APIs in analyzers

[RikkiGibson]

See dotnet/roslyn#63290

1. We take the existing SymbolIsBannedAnalyzer and extract everything to a base class except the specific way the set of banned symbols is obtained.
2. We source-link the new SymbolIsBannedAnalyzerBase into Microsoft.CodeAnalysis.Analyzers and implement SymbolIsBannedInAnalyzersAnalyzer. Instead of reading BannedSymbols from AdditionalFiles, we read from a specific embedded resource. We also use a different diagnostic ID so that if users really want to suppress diagnostics from this, then they can do so independently of the "regular" banned symbol diagnostic.

In order to reduce the magnitude of the break/disruption here, we've done the following:

1. Add a well-known msbuild property EnforceExtendedAnalyzerRules.
2. When this property is not specified, we warn on presence of any analyzers or generators in the project, suggesting that the user specifies <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>. But they could also specify <EnforceExtendedAnalyzerRules>false</EnforceExtendedAnalyzerRules> if they just want to shut these new warnings up entirely.
3. When <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules> is specified, we go ahead and actually subject the user to a potentially quite large amount of new errors if they are using banned APIs.

This gif roughly demonstrates the onboarding/usage experience:

[codecov]

Codecov Report

Merging #6115 (6738a96) into main (dc672ef) will decrease coverage by 0.02%.
The diff coverage is 98.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6115      +/-   ##
==========================================
- Coverage   96.05%   96.03%   -0.03%     
==========================================
  Files        1343     1348       +5     
  Lines      309713   309832     +119     
  Branches     9956     9967      +11     
==========================================
+ Hits       297490   297533      +43     
- Misses       9822     9898      +76     
  Partials     2401     2401              

[mavasani]

We source-link the new SymbolIsBannedAnalyzerBase into Roslyn.Diagnostics.Analyzers

Did you mean to link it into Microsoft.CodeAnalysis.Analyzers instead? Roslyn.Diagnostics.Analyzers is a private package only to be consumed by Roslyn repos. Microsoft.CodeAnalysis.Analyzers NuGet package contains meta analyzers for correct usage of Roslyn APIs and is a package dependency of Microsoft.CodeAnalysis NuGet package.

[mavasani]

Problem: these diagnostics will light up for anyone with a PackageReference to Microsoft.CodeAnalysis. However, it's easy to imagine that lots of projects which reference Microsoft.CodeAnalysis are not creating analyzers, and they need a clear way to opt-out.

Yes, this is going to be a problem of lot of false positives. Basically large number of Roslyn based command line tools will start seeing these warnings. I think a better approach would be to err on the side of few false negatives. I see couple of alternatives:

1. Currently, BannedApiAnalyzer only supports banning APIs in BannedSymbols.txt in the entire compilation. We can consider enhancing the format supported by BannedSymbols.txt to allow specifying banned scopes in form of symbol names, so the banned API usage will be flagged only for analyzer callbacks whose containing symbol chain contains one of the banned scope symbol. For example:

BanScope: T:N1.Type1; T:N2.Type2; N:N3
 T:BannedType; "BannedType is banned within types N1.Type1 and N2.Type2 and namespace N3"

We would also need to support a syntax to allow specifying banned scope as a type hierarchy such that usage is banned within a type and all it's sub-types AND specifying banned scope as an interface name such that usage is banned within types whose AllInterfaces contains the interface.

2. Use the approach as in this PR, but the analyzer implementation in Microsoft.CodeAnalysis.Analyzers explicitly checks if every callback's owning/containing symbol derives from DiagnosticAnalyzer or implements IIncrementalGenerator or ISourceGenerator.

Both these approaches will have false negatives if the APIs that you intend to ban are used in helper methods in some shared static/helper types, but I think that will likely be rare and acceptable false negatives.

[mavasani]

One other approach comes to mind, which might be good 2-step solution to avoid both false positives and false negatives:

1. Have your new analyzer support 2 diagnostics:
   1. First, the core banned API usage diagnostic, which is guarded by presence of a boolean semaphore entry in an additional file or project file (say an entry IsAnalyzerOrGeneratorProject = true or <IsAnalyzerOrGeneratorProject>true</IsAnalyzerOrGeneratorProject> respectively ). By default this entry will be missing and hence this diagnostic will be disabled by default.
   2. Second, a helper warning diagnostic which fires on every diagnostic analyzer or generator sub-type if the project does not contain the above semaphore entry. The code fix for this diagnostic will add the semaphore entry to enable the first diagnostic.
2. The user experience would now be as follows:
   1. For projects referencing MS.CA which do not have analyzers or generators, they should see no behavior change.
   2. For projects referencing MS.CA that have analyzers or generators, they will see new warnings with a code fix stating Do you want to enable additional warnings that will help you avoid using banned APIs in this project? If you apply the code fix, you now see warnings for banned APIs. Alternatively, you can suppress this helper diagnostic and avoid getting banned API usage warnings.

Note that we use similar 2-step approach to enable whole bunch of Release tracking analyzer rules for analyzer projects. We have this special helper diagnostic to enable the analyzer, it's code fix adds new additional files to the analyzer project, which in turn causes the real release tracking analyzer rules to light up. You can see a small demo of the experience below:

[RikkiGibson]

We source-link the new SymbolIsBannedAnalyzerBase into Roslyn.Diagnostics.Analyzers

Did you mean to link it into Microsoft.CodeAnalysis.Analyzers instead? Roslyn.Diagnostics.Analyzers is a private package only to be consumed by Roslyn repos. Microsoft.CodeAnalysis.Analyzers NuGet package contains meta analyzers for correct usage of Roslyn APIs and is a package dependency of Microsoft.CodeAnalysis NuGet package.

Yes, I totally mixed these up, thanks.

[RikkiGibson]

One other approach comes to mind, which might be good 2-step solution to avoid both false positives and false negatives:

That's a very promising approach. I imagine that giving a single explanatory warning would be less overwhelming to have suddenly appear in your build, compared to a potentially very large number of banned API diagnostics. So it seems pretty likely that the right thing would be to ask the author to either "flip a switch" to get the whole set of diagnostics, or just suppress the one if they want to be left alone.

(edit: I had more thoughts here but realized they were redundant with what you already wrote..)

[mavasani]

@RikkiGibson Scanning entire project for presence of analyzers or generators would mean this can only be a CompilationEnd diagnostic, i.e. build-only diagnostic. Using an MsBuild property or additional file entry as this flip would enable us to have this reported even for open files, which is a requirement for code fix support for diagnostics. We don’t support code fixes for build-only diagnostics.

[RikkiGibson]

Got it. It sounds like it would be best to just report the "please explicitly opt in or out of banned API checks" diagnostic whenever we encounter an analyzer/generator type, and it's OK if that results in many similar diagnostics being reported in the project.

[mavasani]

Yes. And I now think it would be best to have a boolean editorconfig option for user’s choice.

[RikkiGibson]

Marking draft while I work on implementing the design described in the above thread

[RikkiGibson]

@mavasani Please let me know if the general design seems to be going in the right direction. Later on I will add more coverage, e.g. when generators are present. I will also fill out the BannedSymbols text file for what we'd like to ban as a starting point.

[RikkiGibson]

Talked to @jaredpar about the design and the main concern was the evolvability of adding potentially disruptive analyzer diagnostics over time. Basically, the question is, should we instead have a single this is an analyzer project setting, versus having disparate enforce analyzer banned APIs, along with possible future enforce analyzer coding pattern X settings.

The feeling was that having a single setting for all time to opt in to all the analyzer diagnostics we think are appropriate might be the way to go, and if people have issues with specific enforcements within that, then they can just suppress the specific diagnostics by ID in whatever scope they deem appropriate.

[mavasani]

This looks great! I have minor suggestions, feel free to address/ignore as appropriate.

[mavasani]

Suggested change

	<value>Do not used APIs banned for analyzers</value>
	<value>Do not use APIs banned for analyzers</value>

[mavasani]

Another option is to package this text file as an additional file in the NuGet package, and have the analyzer access it via AdditionalFiles on the analysis context. This approach has the advantage that the user has the option to hand edit the txt file within the downloaded NuGet package on their machine to include/exclude additional banned APIs in it for customized experience.

In case you would like to take this route: We have post-build tooling in the repo that generates package specific targets file and embeds it into the NuGet package. You can add the ItemGroup to always add this txt file as an additional file by appending to the below string:

roslyn-analyzers/src/Tools/GenerateDocumentationAndConfigFiles/Program.cs

Lines 1560 to 1575 in d80e535

	CodeAnalysisAnalyzersPackageName => @"
	<!-- Target to add all 'EmbeddedResource' files with '.resx' extension as analyzer additional files -->
	<Target Name=""AddAllResxFilesAsAdditionalFiles"" BeforeTargets=""CoreCompile"" Condition=""'@(EmbeddedResource)' != '' AND '$(SkipAddAllResxFilesAsAdditionalFiles)' != 'true'"">
	<ItemGroup>
	<EmbeddedResourceWithResxExtension Include=""@(EmbeddedResource)"" Condition=""'%(Extension)' == '.resx'"" />
	<AdditionalFiles Include=""%(EmbeddedResourceWithResxExtension.Identity)"" />
	</ItemGroup>
	</Target>
	<!-- Workaround for https://github.com/dotnet/roslyn/issues/4655 -->
	<ItemGroup Condition=""Exists('$(MSBuildProjectDirectory)\AnalyzerReleases.Shipped.md')"" >
	<AdditionalFiles Include=""AnalyzerReleases.Shipped.md"" />
	</ItemGroup>
	<ItemGroup Condition=""Exists('$(MSBuildProjectDirectory)\AnalyzerReleases.Unshipped.md')"" >
	<AdditionalFiles Include=""AnalyzerReleases.Unshipped.md"" />
	</ItemGroup>",

You would then need to update https://github.com/dotnet/roslyn-analyzers/blob/main/nuget/Microsoft.CodeAnalysis.Analyzers/Microsoft.CodeAnalysis.Analyzers.Package.csproj to include this file as an AnalyzerNupkgFile, similar to

roslyn-analyzers/nuget/PerformanceSensitiveAnalyzers/Microsoft.CodeAnalysis.PerformanceSensitiveAnalyzers.Package.csproj

Line 22 in 3393ecf

<AnalyzerNupkgFile Include="PerformanceSensitiveAttribute.cs" />

Feel free to ignore this suggestion completely :-)

[RikkiGibson]

Let's track this as a suggestion, and if we find that people need this in order to onboard to the new analyzer, then we'll add it at that point. #6132

[mavasani]

Should we move this attribute type check upfront in this method? I presume it will be cheaper then the code to look for the option setting?

[mavasani]

It would be awesome if this supported a code fix that either creates or appends to an .editorconfig file and adds <%option%> = true entry. Can you file a tracking issue for adding this code fix in future?

[RikkiGibson]

#6133

[mavasani]

Another option is to move this file into the shared utilities project, similar to the common base type shared across analyzer packages here: https://github.com/dotnet/roslyn-analyzers/blob/main/src/Utilities/Compiler/DoNotCatchGeneralUnlessRethrown.cs

[RikkiGibson]

I wasn't able to make this work. I might not have gotten the configuration of the shproj or some #if directives set up properly.

[mavasani]

Keep the fields in the file sorted?

[jaredpar]

My one worry here is that we're tying a lot to editor config. What is the story for customers that are building analyzers but don't use editor config today? How easy is it for them to get to a place where they're specifying this setting?

Is there a good single line discussion about why we didn't use an msbuild property here?

Probably want to include @KathleenDollard for documentation and discussion.

[RikkiGibson]

It seems like roslyn-analyzers is geared toward keeping the msbuild properties which affect analyzer behavior a small/closed set. https://github.com/dotnet/roslyn-analyzers/blob/89942866e0fde9a71327be082183b6b3b972a08b/src/Utilities/Compiler/Options/MSBuildPropertyOptionNames.cs

@mavasani might have insight about whether an msbuild property could be problematic here.

I could certainly imagine people wanting to enable these errors in some source files but not others. But even if we use an msbuild property, which applies to the whole project, granular suppressions can be accomplished through the normal analyzer diagnostic suppression mechanisms.

[mavasani]

It seems like roslyn-analyzers is geared toward keeping the msbuild properties which affect analyzer behavior a small/closed set

@RikkiGibson We don't have any restriction as such. These are the MSBuild properties that flow to the analyzers via compiler generated globalconfig file. Given this feature was added pretty recently, very few analyzers have been added which require an MSBuild property as an option.

@mavasani might have insight about whether an msbuild property could be problematic here.

I think this would be just fine. This does seem like a better design as theoretically the flag you are adding is a project level property, not per-file or per-directory one.

[RikkiGibson]

The following CI failure is happening consistently.

##[error].packages\coverlet.msbuild\3.1.2\build\coverlet.msbuild.targets(71,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Unable to read beyond the end of the stream.
   at System.IO.BinaryReader.FillBuffer(Int32 numBytes)
   at System.IO.BinaryReader.ReadInt32()
   at Coverlet.Core.Coverage.CalculateCoverage() in /_/src/coverlet.core/Coverage.cs:line 414
   at Coverlet.Core.Coverage.GetCoverageResult() in /_/src/coverlet.core/Coverage.cs:line 161
   at Coverlet.MSbuild.Tasks.CoverageResultTask.Execute() in /_/src/coverlet.msbuild.tasks/CoverageResultTask.cs:line 83

Main branch builds appear to be clean so I don't know what's going wrong.

[RikkiGibson]

Added a demo gif to the PR description.

[RikkiGibson]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s), but failed to run 1 pipeline(s).

[RikkiGibson]

failed to run 1 pipeline(s)

wh-which one? will it be forever a mystery?

[Youssef1313]

@RikkiGibson Should all "analyzers" in these messages be updated to "analyzers or generators"

[yfital]

I'm sorry, I can't find any documentation or recommendation on what to actually do in some cases, for instance:

Analyzers should not read their settings directly from environment variables

The reason why we added Env is to support simple "debugging" tools in case of errors as it might be challenging sometimes with analyzers.

Is there a way to pass from VS an option towards the analyzer, and if so, how?