`SuppressWarningsWithoutExplanation` Inconsistency Between Documentation and Implementation

[ryanpinter]

There is an inconsistency between the documentation and the actual implementation of the SuppressWarningsWithoutExplanation bug checker in Error Prone.

Description

1. The main documentation at https://errorprone.info/bugpattern/SuppressWarningsWithoutExplanation states the following:

   • In the "Introduction / One line summary" section: "Use of @SuppressWarnings should be accompanied by a comment describing why the warning is safe to ignore."
     • This implies ALL @SuppressWarnings
   • In the "The problem" section: "Suppressions for unchecked or rawtypes warnings should have an accompanying comment..."
     • This implies this check is only applicable to @SuppressWarnings for unchecked and rawtypes

2. The actual code implementation at SuppressWarningsWithoutExplanation.java:

   • Code comment states: "The Google style guide mandates this for all suppressions; this is only matching on {@code deprecation} as a trial."
   • Implementation only checks for "deprecation" suppressions as shown in:

     private static final Matcher<AnnotationTree> SUPPRESS_WARNINGS =
         allOf(
             isSameType(SuppressWarnings.class),
             hasArgumentWithValue("value", stringLiteral("deprecation")));

Expected Behavior

Based on the Google style guide referenced in the code (which mandates explanatory comments for all suppressions) and the general documentation, this check should apply to ALL @SuppressWarnings annotations, not just those with "deprecation" value.

• Note: I cannot find a reference to this rule in the current published Java style guide at https://google.github.io/styleguide/javaguide.html

Suggested Fix

1. Update the implementation to check all @SuppressWarnings annotations
2. Or update the implementation to check only @SuppressWarnings for unchecked or rawtypes
3. Or update the documentation to clearly state that this check currently only applies to "deprecation" suppressions

[cushon]

The history here is that the initial implementation of the check only looked for deprecation as a way to get started, and because documenting those seem especially valuable.

bdcc971 updates it to also check unchecked and rawtypes, and also updates the docs and an implementation comment to clarify things a bit.

[uhafner]

I understand the motivation for this rule, but this rule creates a lot of warnings for my test code that uses Mockito to create stubs (but this is not a big deal, I tend to deactivate this rule globally for my projects).

What is more important: the comment link of the warning points to https://google.github.io/styleguide/javaguide.html#s8.4.2-how-to-handle-a-warning. This link does not exist. There is no section 8 in the document. The only @SuppressWarnings reference in that document does not use a justification comment, so it makes sense to add such a section or change the link.

[graememorgan]

Thanks for the detailed points, those are all worth fixing. I wonder if this is a check that actually has value on the OSS release; it's an internal style rule, and maybe ought to be opt-in outside.

(fwiw for Mockito you might want to consider https://javadoc.io/static/org.mockito/mockito-core/3.1.0/org/mockito/junit/MockitoJUnit.html, assuming you mean the problem I think you mean.)

[cushon]

ought to be opt-in outside

It's in the DISABLED_CHECKS list, so it already should be opt-in:

error-prone/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

Line 1202 in ff3255e

public static final ImmutableSet<BugCheckerInfo> DISABLED_CHECKS =

I'll remove the link for the internal style guide reference.

We can also stop including this check in open source Error Prone if it isn't valuable outside of google, and including it is confusing.

Does anyone in the bug have thoughts on that?

@ben-manes how did you come across this in #5524, were you enabling disabled-by-default Error Prone checks to see which ones were useful to you? (I think that's a reasonable thing to do, I'm just curious how you came across this check.)

[ben-manes]

Yep. I switched Caffeine over to all enabled with opt-out a year or two ago, saw it, but ignored tbh. I am doing the same at work during idle times in order to improve quality where the reference link is important to teach them why. I did disable regardless because I don’t feel it’s necessary, especially if #4828 gets merged.