[draft] initial implementation of emitting warnings for unneeded suppressions

[msridhar]

See #886. This change requires various cleanups, better naming, documentation, etc., and it doesn't handle all cases, but I wanted to post it to get feedback on whether this approach seems reasonable or needs to be reworked in some way.

For the most part this PR piggybacks on the extant tracking of which suppressions are active and when checkers are being suppressed. If a warning would be emitted by a checker and there is an active suppression of that warning, the suppression is marked as "used." Then, in Scanner, warnings are emitted for unused suppressions when restoring the old suppression information. Custom suppression annotations are not yet handled. Also we don't yet report the warning on the proper @SuppressWarnings annotation directly and support auto-fixing.

The main complication is that checkers may discover and use suppression information in ways that are hard to automatically detect. E.g., see this logic in UngroupedOverloads; NullAway also has such custom logic. If these uses of suppression annotations are not detected, it will lead to suppressions falsely being reported as unused. To avoid that, for now checkers need to opt in to supporting unused warning suppression checking. I opted in JdkObsolete as an example, as it does not seem to have any non-standard checking of suppressions. If this approach makes sense, maybe we can find a way to automatically opt in more checkers in the future (or just do so by hand). I also added an API to enable checkers to expose their custom uses of suppressions, and made some initial use of it in this WIP NullAway change.

If this approach looks reasonable / acceptable I'll clean up the changes and make them ready for a proper review. FYI @cushon @cpovirk

[msridhar]

@cushon was curious if you had any thoughts on this approach and whether it's acceptable. Happy to do more cleanup if you feel it's needed before you can take a quick-ish look. And I do intend to implement proper support for auto-fix before requesting a proper review.

[ben-manes]

PMD recently added this feature and allowed me to clear out 38 stale suppressions. We ran this draft and cleaned up a bunch of those too.

[cushon]

Thanks for working on this!

I took a quick pass. I think this makes sense overall, I will try to do some internal testing.

[cushon]

I wonder about refactoring to avoid repeating this logic between the disjoint call above and here.

[msridhar]

Good idea, fixed in 7c5c0d5

[cushon]

Is it possible to suppress UnusedSuppressionChecker findings? This isn't a feature request to do that, I'm just thinking through it.

I suppose one reason to want that would be if an Error Prone check name shadowed a suppression string used by another tool, and Error Prone didn't report the findings but the other tool would.

[msridhar]

I guess I'm not sure about this. On the one hand, I think it's probably generally good for all warnings to be suppressible somehow. Javac is working on adding a similar check and there you can suppress the unneeded suppression warning.

On the other hand, I wasn't sure if users would always run with this unused suppression check enabled. That could potentially negatively impact build times, since without this new check, Error Prone completely skips running checkers on sub-trees where the warnings would be suppressed. But maybe users don't rely on that?

In any case, I can add this support if we decide to do it.

[msridhar]

I took a quick pass. I think this makes sense overall, I will try to do some internal testing.

Sounds good! Let me know how things look after that.

[cushon]

This makes the enclosing class no longer @Immutable. (The immutable checker isn't enabled for the external build.)

[msridhar]

Do we need to maintain the immutability of the class? Right now we mutate the usedSuppressWarningStrings set when we detect that a suppression has actually caused a warning to be suppressed; mutable state seemed like the simplest option for that. If we want the enclosing class to remain immutable, any suggestions on where else to track that a suppression was used? We could thread an extra parameter through a lot of code to track this, I guess, but it seems like would be a much more invasive change (didn't think through it deeply).

[cushon]

We have been trying to discourage new uses of this API. Is it actually needed here, the default severity for UnusedSuppressionChecker is WARNING?

[msridhar]

When I try to use UNUSED_SUPPRESSION_CHECKER.describeMatch(tree) I get a crash running tests:

    An exception has occurred in the compiler (21.0.8). Please file a bug against the Java compiler via the Java bug reporting page (https://bugreport.java.com) after checking the Bug Database (https://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
    java.util.NoSuchElementException: No value present
    	at java.base/java.util.Optional.get(Optional.java:143)
    	at com.google.errorprone.matchers.Description.severity(Description.java:78)
    	at com.google.errorprone.ErrorProneAnalyzer.lambda$finished$3(ErrorProneAnalyzer.java:213)
    	at com.google.errorprone.VisitorState.reportMatch(VisitorState.java:311)
    	at com.google.errorprone.SuppressionInfo.warnOnUnusedSuppressions(SuppressionInfo.java:132)
    	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:63)
    	at com.google.errorprone.scanner.ErrorProneScannerTransformer.apply(ErrorProneScannerTransformer.java:43)
[...]

Same thing when I get rid of the overrideSeverity call. Any suggestions?

[cushon]

I took a quick pass. I think this makes sense overall, I will try to do some internal testing.

Sounds good! Let me know how things look after that.

Still waiting on results for the testing, but I left a couple of small comments on things I noticed while importing it to set that up.