google
[draft] initial implementation of emitting warnings for unneeded suppressions
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
@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.
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.
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.
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.
