Roslyn: Allow an analyzer to suppress diagnostics

[sharwell]

There are several situations where it would be helpful for an analyzer, or an analyzer assembly, to have the ability to suppress diagnostics reported by another analyzer or as a warning by the compiler. For example:

• StyleCop Analyzers rule to "Use 'this.'" could automatically disable the IDE warning with the same intent
• Analyzers could suppress compiler warnings like CS1591 using application-specific logic

Such a "Diagnostic Filter" feature would likely run after analysis is complete, and be provided a list of reported diagnostics with the ability to suppress specific diagnostics. It would be helpful if the diagnostics were treated as suppressed in the Error List window, with the suppression source set to the name of the filter responsible for suppressing it.

[KrisVandermotten]

This raises a lot of questions.

First of all, it seems that such an analyzer should not be able to suppress compiler errors. After all, a true compiler error (as opposed to a warning that got converted into an error by Treat Warnings as Errors) causes the compiler to not compile, and there is nothing an analyzer can do to change that.

Secondly, It seems to me that this should apply to all true errors, not only those reported by the compiler, but those reported by other analyzers as well.

Also, as in #12702, it seems that this would often be just a means to fight symptoms, instead of fighting the disease. If an analyzer reports an error incorrectly, the analyzer should be fixed, instead of creating a "workaround analyzer".

Finally, analyzers can already provide a code fix provider, and such a code fix provider can insert a #pragma warning disable xxxx. into the offending code, and they can provide a FixAllProvider. That seems to be a pretty good alternative to me. Moreover, it makes the decision to suppress certain diagnostics explicit, which I think is a good thing.

Indeed, the proposed feature would suppress implicitly. If the feature is a way to fight the symptoms of incorrect analyzers, what if a diagnostic suppressing analyzer has itself a bug, and suppresses diagnostics that shouldn't be suppressed? I'm sure we wouldn't want yet another level of workaround analyzers that can "unsuppress" those. With a code fix provider on the other hand, that problem doesn't exist, as you can just not apply the code fix or delete the generated @pragma.

The proposal feels like a very big gun to kill a very small fly. Note that this is not a "people might shoot themselves in the foot" argument, as I don't believe in such arguments. I'm just referring to the assumed cost to build it, compared to the net benefit.

[sharwell]

@KrisVandermotten thanks for taking the time to write up all those thoughts! It's some great feedback. :smile:

First of all, it seems that such an analyzer should not be able to suppress compiler errors.

This is correct. A compiler error means it is impossible to proceed under the required semantics of the language. A compiler warning means it is possible to proceed, though the result may not be what the programmer originally intended.

Secondly, It seems to me that this should apply to all true errors, not only those reported by the compiler, but those reported by other analyzers as well.

This is not correct. One important use case of this feature is enabling users to increase the severity of a warning to being an error by using the new feature to filter out cases that might otherwise prevent them from doing so.

Also, as in #12702, it seems that this would often be just a means to fight symptoms, instead of fighting the disease.

Sometimes there are other situations at play. For example, one might encounter one "rare" situation where analyzer A from person X conflicts with analyzer B from person Y. Person X claims the solution is removing B, while Person Y claims the solution is removing analyzer A. Neither has interest in taking on the responsibility of maintaining compatibility with unrelated software, yet you now have a potential solution if both X and Y (and more specifically, A and B) are otherwise meaningful for your application.

In other cases, such as #12702, the implementation may have a very long period of time between initial revelation of the problem and a proposal actually making it's way through to becoming a change in the compiler. As was mentioned, "generated code" as a concept is a weakly-defined heuristic that changes from release to release. It might be suitable as an input to analyzers, but doesn't lend itself well to being a filter for warnings produced by a compiler because long-term language compatibility rules dictate that the introduction of a warning is the same thing as the introduction of an error. When "very long" becomes "longer than the development lifecycle of my application", then what started as a workaround becomes more of a requirement.

Finally, analyzers can already provide a code fix provider, and such a code fix provider can insert a #pragma warning disable xxxx. into the offending code, and they can provide a FixAllProvider. That seems to be a pretty good alternative to me. Moreover, it makes the decision to suppress certain diagnostics explicit, which I think is a good thing.

Some diagnostics are not possible to suppress in this manner. Two examples are:

• CS1570/CS1587 if you have a RESX string where the value represents a documentation comment and the individual lines in the value don't start with leading spaces.
• StyleCop Analyzers diagnostic SA1633, as described in DotNetAnalyzers/StyleCopAnalyzers#1701.

Both of these might be considered bugs, but neither has a specific time frame when they'll be fixed or a better alternative is expected to exist.

In other case, the ongoing maintenance policy of a project used a language feature in a manner very much other than intended, but it ended up working for them. For example, the suppression approach was flatly rejected when I proposed Microsoft/perfview#293 (originally part of Microsoft/perfview#227), because even though it's the "right" way to suppress warnings it caused usability problems which were bigger than the side effects otherwise seen from what some people would consider abuse of a language feature.

The proposal feels like a very big gun to kill a very small fly.

In a way, it is. But it's also an issue that comes up pretty frequently in other terms. For example, this feature might give a user tighter control over the meaning of "generated code", which currently provides very similar suppression functionality but is not configurable.

[KrisVandermotten]

One important use case of this feature is enabling users to increase the severity of a warning to being an error by using the new feature to filter out cases that might otherwise prevent them from doing so.

Sure, but then it was a warning in the first place, and was later treated as an error.

When I said "true error", I meant a diagnostic that was originally reported as an error by some analyzer, as opposed to a warning that was treated as an error later on. I think that these "true errors" should be protected from suppression, though I must admit it is difficult to put my finger on it why exactly that is. I guess that it is because by definition an error is "Something not allowed by the rules of the language or other authority." If that's the case, then some other analyzer should not be able to make something that is not allowed, allowed again.

Note that when understood this way, this is not incompatible with the important use case you mention.

[sharwell]

When I said "true error", I meant a diagnostic that was originally reported as an error by some analyzer,

I believe even in these cases it is currently possible for a rule set file to override the reported severity to downgrade it to a warning.

[sharwell]

"Something not allowed by the rules of the language or other authority." If that's the case, then some other analyzer should not be able to make something that is not allowed, allowed again.

Also, keep in mind that it is conceivable for a group of analyzers to all leverage this filtering ability within the context of a larger analysis process. For example, suppose an organization wants to apply rules { R1, ..., R30 }, with error-severity enforcement except in conditions X and Y which apply to all rules. It might be easier to implement and review the individual rules for accuracy without the exception, and then apply the exception as a filter on top of that. The "true errors" then reasonably become the set of errors which were reported by an analyzer and remain after filtering.

[KrisVandermotten]

I believe even in these cases it is currently possible for a rule set file to override the reported severity to downgrade it to a warning.

It might be easier to implement and review the individual rules for accuracy without the exception, and then apply the exception as a filter on top of that.

Those are very valid points indeed.

[sharwell]

Here's another case which would be easier if this feature existed:
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/2509

The affected user could apply this logic independently of StyleCop Analyzers without requiring the creation of a new option that affects everyone.

[Evangelink]

We (at SonarSource) would also be interested in such kind of feature in order to provide a better user experience with issues that have been marked as won't fix or False Positive on our server side (SonarQube).

Link to our MMF.

[Evangelink]

Sorry for the ping but I am wondering if there is any traction on your side for this feature?

[mavasani]

I believe such diagnostic filter extensions would need to work very similar to code fixers. Following analyzer API additions might suffice:

1. Add a new _virtual_ property ImmutableArray<string> SuppressibleDiagnosticIds { get; } to DiagnosticAnalyzer type.
2. Add a new API to AnalysisContext to register callbacks for suppression: void RegisterSuppressionAction(SuppressionAnalysisContext). These callbacks will be invoked _after_ all analysis is complete and we have computed all compiler and analyzer diagnostics for the entire compilation.
   3.

public struct SuppressionAnalysisContext
{
   // Reported "configurable" diagnostics with IDs from SuppressibleDiagnosticIds. 
   ImmutableArray<Diagnostic> ReportedDiagnostics { get; }

   // Suppress a reported diagnostic, which must be from ReportedDiagnostics.
   void SuppressDiagnostic(Diagnostic);

   // Share semantic model between providers that need to perform semantic analysis to determine suppressions
   SemanticModel GetSemanticModel(SyntaxTree);

   Compilation Compilation { get; }

   CancellationToken CancellationToken { get; }
}

@sharwell @jinujoseph @tmat Can we bring this up at the design meeting? https://github.com/dotnet/roslyn/issues/30172 has higher priority now as we fade out unused private members, and we need to implement this for Dev16.

[kendrahavens]

Design meeting notes:

• Allow analyzer authors to suppress other diagnostics.
• Allow user to disable diagnostic suppression by either uninstalling packages that provided it or via a new view.
  
  
  
  • Add flag or a new view filter in the Error List for displaying diagnostics that have been suppressed.
  
  
  • User should also be able to edit what is suppressed in this view. (Maybe call it “override diagnostic” not “suppressed diagnostic” if that is clearer.)
  
  
• Allow package authors to specify a filter that determines if a diagnostic with a certain ID is suppressed.
• A suppression cannot also provide diagnostics. All diagnostics will be reported first and not allow a circular ability to suppress each other.
  
  
  
  • Two analyzers can suppress each other. (But suppressions can’t suppress other suppressions. 😊)
  
  
• Don’t allow suppressing diagnostics that originated as an error by analyzer authors. Suppression is still allowed by end users. This includes errors reported by other analyzers.
  
  
  
  • We can change this in future releases if we hear from users that they need to be able to suppress errors that give false positives. This may be common if the compiler is missing some context.
  
  
  • If this is commonly requested in the future, we should consider a “Not suppressible” tag for analyzers.
  
  
  • Open question: How to handle “not configurable” versus security error?
  
  
• How does an author suppressing a different analyzer know that the analyzer it is suppressing has been changed?
  
  
  
  • Authors doing the suppression should have the most context on what to suppress, meaning they should take package version into account and update if something they are suppressing changes behavior.
  
  
  • The suppressed analyzer isn’t necessarily updating at the same rate at the analyzer doing the suppression. We agree that this case of versioning won’t be common enough to cause major problems and that updating is always expected to cause versioning issues.
  
  

[mavasani]

Fixed with https://github.com/dotnet/roslyn/pull/36067 in VS2019 16.3