Roslyn: Fix-all to remove warning suppressions that have become unnecessary (nullability and pragma)

[jnm2]

Over time the codebases I've worked in have accumulated pragmas and now nullability warning suppressions as workarounds for limitations or bugs in Visual Studio's analyzers and third-party analyzers. There's no mechanism on the other side to bring it to my attention when the suppressions are no longer relevant because the analysis has been improved.

I knew that VS 16.5 was fixing several of things that I had added pragmas for years ago and improving nullability analysis, so I wrote https://github.com/jnm2/SuppressionCleanupTool. It initially served as means of querying where the suppressions were actually located, because it's not like I could simply search for the text ! and find only the instances where it represented the nullability operator. Once it located each suppression, it became obvious that the tool could use Roslyn as the source of truth about whether each !, = default!, and #pragma could be removed.

It's currently a console app. I was considering making this a VSIX, maybe a dotnet global tool, maybe an analyzer + code fix. The problem with the tool not running in the context of VS is that it isn't referencing the same versions of Microsoft.CodeAnalysis.Features, so even as a console app it may need to let you dynamically resolve all Roslyn dependencies from vswhere or NuGet.

@jmarolf suggested that I request this tool to be part of Roslyn itself. Having it be built in would be preferable from my vantage point:

• Everyone is interested in removing outdated cruft.
• Access to non-public Roslyn API and your code review will increase the likelihood of the tool's efficiency and correctness.
• If you allow it to run automatically by default or opt-in, and show in the error list and fade the syntax in the editor, there will be something to tell us when the cruft can be removed. I try to always leave a bug URL comment behind nowadays for easier checking (e.g.), but mentally evaluating the linked pages still requires spare time.

The only concern I have about running inside VS is that it might remove suppressions from a codebase that has users with older or newer versions of VS with a bug or analysis limitation that is not present in the version you used to remove the suppressions.

[CyrusNajmabadi]

@jmarolf suggested that I request this tool to be part of Roslyn itself. Having it be built in would be preferable from my vantage point:

Yes, i would far prefer this. I think it would make a ton of sense for roslyn to let you know when a suppression was no longer necessary.

[mavasani]

This was originally filed as https://github.com/dotnet/roslyn/issues/4262. We should write this an analyzer/code fix in the shared analyzers/code fixes layer: https://github.com/dotnet/roslyn/tree/master/src/Analyzers/Core, so that it light ups in the IDE as well as in our CodeStyle NuGet package, which allows users to reference the analyzer NuGet package and bump up the unnecessary suppressions to warning/error to break build.

[mavasani]

There was internal feature request for unused suppressions being implemented as an analyzer, that they can be configured to break build. Additionally, a FixAll operation to remove all of these would be super helpful.

Marking for design review for next IDE meeting to get traction on this.

[paulomorgado]

@mavasani, don't forget the ErrorLog (SARIF) output. This should have an ID (probably information) that can be leveraged by tools analyzing the build.

[mavasani]

I have put more thought on this, and I think we can divide this into few separate feature requests:

1. Grey out and/or squiggle global suppress message attributes that have an invalid Target string that does not resolve to any symbol in the compilation. These are potentially quite common form of broken suppressions as symbols get renamed/removed/refactored.
2. [Compiler diagnostics]: Grey out pragma suppressions or nullable suppressions that have no corresponding compiler diagnostic in the compilation that is actually being suppressed from it. These are potentially due to changes in compiler implementation OR source code changes in the user code that fixed the underlying diagnostic for which suppression was originally added. These also seem pretty common form of redundant suppressions.
3. [Analyzer diagnostics]: Grey out pragma suppressions and/or suppress message attributes (global or local applied directly to the symbol) that have no corresponding analyzer diagnostic in the compilation that is actually being suppressed from it. These are potentially due to changes in analyzer implementation OR source code changes in the user code that fixed the underlying diagnostic for which suppression was originally added. These also seem pretty common form of redundant suppressions.

(1) is pretty easy to implement as an analyzer in the IDE CodeStyle NuGet package, so it can be strongly enforced in the build, if desired. This also serves the internal feature request that we received.

(2) can also be pretty easily implemented as an analyzer in the IDE CodeStyle NuGet package, so it can be strongly enforced in the build.

(3) is actually pretty hard to implement as an analyzer, if at all possible. By definition, this feature depends on knowing all the analyzer diagnostics reported in a file/compilation, and our analyzer ecosystem does not permit any analyzer to depend on output of another analyzer. I think we should take the approach @jnm2 has taken and implement this as a command line tool, and try to get his work merged into Roslyn (can it be possibly merged in as an additional functionality for AnalyzerRunner?)

Thoughts?

[mavasani]

@mavasani, don't forget the ErrorLog (SARIF) output. This should have an ID (probably information) that can be leveraged by tools analyzing the build.

@paulomorgado Any changes to SARIF error log would be an orthogonal feature request, that would need compiler changes and a compiler feature request. Possibly add this to https://github.com/dotnet/roslyn/issues/42489 or file a separate issue?

[paulomorgado]

@mavasani, this wouldn't be a change to the SARIF error log if this is a normal warning with an ID. Would it?

[jnm2]

I'm lost... what's SARIF?

[mavasani]

@jnm2 SARIF is an open source file format for code analysis/diagnostics issue log: https://sarifweb.azurewebsites.net/
This is generated by compiler with /errorlog switch: https://github.com/dotnet/roslyn/blob/e704ca635bd6de70a0250e34c4567c7a28fa9f6d/src/Compilers/Core/Portable/CommandLine/ErrorLogOptions.cs#L11-L14

By default, compiler generates SARIF error log file using v1 format, but there is a command line switch to use v2 format: https://github.com/dotnet/roslyn/blob/45d9c6059a210124283a8b4dfcc4601335bfd383/src/Compilers/CSharp/Portable/CSharpResources.resx#L4625-L4629

[paulomorgado]

The Roslyn compilers have a errorlog compiler option that outputs a log all compiler and analyzer diagnostics in SARIF format.

The implementation is here: https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/CommandLine/SarifErrorLogger.cs

[jnm2]

(3) is actually pretty hard to implement as an analyzer, if at all possible. By definition, this feature depends on knowing all the analyzer diagnostics reported in a file/compilation, and our analyzer ecosystem does not permit any analyzer to depend on output of another analyzer. I think we should take the approach @jnm2 has taken and implement this as a command line tool, and try to get his work merged into Roslyn (can it be possibly merged in as an additional functionality for AnalyzerRunner?)

The CLI tool was motivated by the high number of suppressions of things that are not third-party analyzers. For third-party analyzers, what would be the prompt to tell you to dig out and run the CLI tool? All the value of (3) that I can see is in detecting and notifying that action may be desired. Couldn't (3) be implemented by having the analyzer driver report which suppressions have been unused at the time that all analyzers stop running? It doesn't need to be an analyzer to be a useful IDE experience.

[mavasani]

All the value of (3) that I can see is in detecting and notifying that action may be desired. Couldn't (3) be implemented by having the analyzer driver report which suppressions have been unused at the time that all analyzers stop running? It doesn't need to be an analyzer to be a useful IDE experience.

Yes, we can implement this as an IDE feature. We can create a special IDE diagnostic source that takes all third party analyzer diagnostics as input, and reports these special unnecessary suppression diagnostics which grey out suppressions.

[paulomorgado]

Please, don't make it just an IDE feature. That'd be very useful for other tooling (like code review tooling).

[mavasani]

Please, don't make it just an IDE feature. That'd be very useful for other tooling (like code review tooling).

We can possibly expose this as a public API from Workspaces/Features layer for other tools.

[paulomorgado]

As long as it's on the SARIF file, I'm fine with it. 😄

[jnm2]

We can possibly expose this as a public API from Workspaces/Features layer for other tools.

I don't know enough about what I'm saying, but both csc.exe and the IDE run analyzers. If the tracking of used and unused suppressions was done at the level of the analyzer driver/runner, wouldn't the analyzer driver be reporting the unused suppressions to both csc.exe and the IDE?

[mavasani]

@paulomorgado @jnm2 - both your suggestions are validate and independent suggestions for changes to core compiler (even the analyzer driver that runs in build is part of core compiler). Given there is a separate team who owns that feature space, I think we should move the feature (3) for unused suppressions of analyzer diagnostics + SARIF change requests to separate issues which can be triaged and implemented independently. Lets keep this issue for features that can be implemented in IDE layer + IDE team's CodeStyle NuGet package (using existing analyzer driver support) - which would be (1) and (2) from my comment above, and yes both of these will be supported in csc.exe and IDE

[sharwell]

We took this to a design review today.

• We are creating two separate issues relating to the maintenance of the Target property of global suppressions.
• We are tracking the elimination of unnecessary nullability suppressions as #25372 (compiler) and #34714 (IDE)
• We are creating an issue requesting compiler identification of unnecessary suppressions and a corresponding IDE issue relating to reporting and code fixes (a similar pair to the previous item)

[mavasani]

We are creating two separate issues relating to the maintenance of the Target property of global suppressions

Filed https://github.com/dotnet/roslyn/issues/44176

We are creating an issue requesting compiler identification of unnecessary suppressions and a corresponding IDE issue relating to reporting and code fixes (a similar pair to the previous item)

I put some more thought into this and I don't think this is possible to write as an IDE analyzer/fixer that can be enforced on build. Even if the core analyzer driver reports a hidden diagnostic for unnecessary suppression, this diagnostic cannot be consumed by any analyzer. Analyzers can only invoke core compiler APIs or hidden diagnostics reported by core compiler layer (such as unnecessary usings), but cannot rely on any other analyzer or output from analyzer driver. I believe this feature can only be implemented as a post-build tool OR an IDE-only feature.
I filed 2 separate issues here:

1. https://github.com/dotnet/roslyn/issues/44177 (Analyzer/fixer for unnecessary pragma suppressions for compiler diagnostics): Can be implemented as an analyzer/fixer as it relies only on compiler diagnostics.
2. https://github.com/dotnet/roslyn/issues/44178 (Unnecessary pragma/SuppressMessage suppressions for analyzer diagnostics): Does not seem feasible to be implemented as an analyzer/fixer.

[mavasani]

Also filed https://github.com/dotnet/roslyn/issues/44288: _Find all references and Inline rename should handle symbol references in SuppressMessageAttribute_

FYI: I have a PR out for #44176 (Add analyzer/fixer for unnecessary global SuppressMessageAttributes with invalid 'Scope' and/or 'Target')

[mavasani]

Update

We added support for the following features in 16.7/16.8:

1. Unnecessary pragma suppressions for compiler and analyzer diagnostics are greyed out in the IDE. This analyzer is not supported on build. https://github.com/dotnet/roslyn/pull/44848
   

2. Unnecessary inline/local SuppressMessageAttribute based suppressions for analyzer diagnostics are greyed out in the IDE. This analyzer is not supported on build. https://github.com/dotnet/roslyn/pull/45765
   

3. Invalid global SuppressMessageAttribute based suppressions which have an invalid Target and/or Scope are greyed out in the IDE. This analyzer is supported on build. https://github.com/dotnet/roslyn/pull/44287
   

4. Legacy FxCop format SuppressMessageAttribute showed performance problems in IDE as well as build. So, now we show an IDE suggestion to convert this to Roslyn format SuppressMessageAttribute with a code fix. This analyzer is supported on build.
   https://github.com/dotnet/roslyn/pull/44440

5. Rename/Find All References now understands references to symbols within the target string of global SuppressMessageAttribute suppressions. https://github.com/dotnet/roslyn/pull/44368
   

Pending features:

1. Unnecessary nullable suppressions ! operator (https://github.com/dotnet/roslyn/issues/25372): I prototyped reporting these within the compiler layer (see https://github.com/dotnet/roslyn/compare/master...mavasani:UnnecessaryNullableSuppression?expand=1), but I came to a conclusion that this is not feasible, but someone from compiler team needs to try it out to confirm/reject that theory (most like @jcouv). See https://github.com/dotnet/roslyn/issues/25372#issuecomment-639647979 for details. IMO, given nullable warnings are based on flow analysis and effects actual type analysis via control flow, the only realistic way to implement this would be to take the same approach as taken by @jnm2, where you try to remove the suppression, and see if new binding new code produces additional diagnostics or not.
2. Enable unnecessary pragma suppressions and unnecessary inline/local SuppressMessageAttribute based suppressions on build - I am not sure if this is feasible given the current design of analyzers (no analyzer can depend on output of another analyzer). We should likely open a separate feature request if this is required.

At this point, I am not sure if there is anything actionable work remaining in this issue. We can track any additional work/features with separate issues. I propose we close this issue now.

[jnm2]

This is exciting! Thanks for working on this!

Closing sounds good to me. I'm interested in https://github.com/dotnet/roslyn/issues/25372 and can follow it there, and if someone is interested in build-time warnings they can open a new issue.