MissingOverride: ignore generated method about error-prone HOT 7 CLOSED

While looking into (the different but related) PicnicSupermarket/error-prone-support#716, I noticed that Lombok by default adds @SuppressWarnings("all") to generated code. The Immutables library does the same.

So an alternative works-out-of-the-box-for-all-checks solution could be the following:

diff --git a/check_api/src/main/java/com/google/errorprone/SuppressionInfo.java b/check_api/src/main/java/com/google/errorprone/SuppressionInfo.java
index cb23c08dea..3b891b9cc5 100644
--- a/check_api/src/main/java/com/google/errorprone/SuppressionInfo.java
+++ b/check_api/src/main/java/com/google/errorprone/SuppressionInfo.java
@@ -85,7 +85,8 @@ public class SuppressionInfo {
       return SuppressedState.SUPPRESSED;
     }
     if (suppressible.supportsSuppressWarnings()
-        && !Collections.disjoint(suppressible.allNames(), suppressWarningsStrings)) {
+        && (suppressWarningsStrings.contains("all")
+            || !Collections.disjoint(suppressible.allNames(), suppressWarningsStrings))) {
       return SuppressedState.SUPPRESSED;
     }
     if (suppressible.suppressedByAnyOf(customSuppressions, state)) {
diff --git a/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java b/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java
index 4693b70ef8..f6485134a0 100644
--- a/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java
+++ b/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java
@@ -99,6 +99,7 @@ import java.io.Serializable;
 import java.lang.annotation.Annotation;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.BiPredicate;
@@ -269,8 +270,12 @@ public abstract class BugChecker implements Suppressible, Serializable {
   }
 
   private boolean isSuppressed(SuppressWarnings suppression) {
-    return suppression != null
-        && !Collections.disjoint(Arrays.asList(suppression.value()), allNames());
+    if (suppression == null) {
+      return false;
+    }
+
+    List<String> suppressions = Arrays.asList(suppression.value());
+    return suppressions.contains("all") || !Collections.disjoint(suppressions, allNames());
   }
 
   /**

If the Error Prone maintainers think that this is an acceptable solution then I wouldn't mind writing some unit tests for this and opening a PR.

from error-prone.

I have some mixed feelings about @SuppressWarnings("all").

Agree. Both suggestions SGTM, though a separate check is perhaps a bit more versatile, as it would also cover suppressions targeted at other static analysis tools.

W.r.t. opening a PR: I've added it to my TODO list for the coming days; need to first focus on a few other topics. (The modified methods and surrounding SuppressionInfo and BugChecker classes don't yet have (directly) associated tests. I suppose creating such classes is a bit nicer than modifying some "random" other class.)

from error-prone.

Thanks for the pointers @cushon! Eventually I settled on option (2). I filed #4076 with a proposal.

from error-prone.

I think this would solve a lot of issues, as this is not the only check that is conflicting with Lombok.
Especially since the @Generated annotations are not always added on e.g. class level.

Thanks @Stephan202 and @cushon for the quick response 👌

from error-prone.

Thanks for working on this @Stephan202!

I'm happy to be pragmatic about either of the options you described. If writing the test with CompilationTestHelper is cleaner, I think it's OK to define the test in core. I would also be OK with yet another copy of the boilerplate in SignaturesTest to set up a javac invocation.

from error-prone.

If the Error Prone maintainers think that this is an acceptable solution then I wouldn't mind writing some unit tests for this and opening a PR.

That SGTM overall. I think your proposal handles that, but it would be good to have test coverage to double-check it doesn't apply to 'undisableable' checks.

I have some mixed feelings about @SuppressWarnings("all").

• Our internal style is to prefer specific suppressions with a comment explaining them (e.g. @SuppressWarnings("unchecked") // safe covariant cast).

• But just ignoring @SuppressWarnings("all") doesn't seem very friendly, and recognizing it might address some of the issues with the interaction between Error Prone and the Lombok language.

• One possibility might be to have yet another flag to configure whether all is supported, or yet another check to discourage its use.

from error-prone.

Practical question for @cushon :)

Context: if made @VisibleForTesting, writing tests for the (effectively deprecated) method in BugChecker isn't so hard, as can be seen on this branch. Part of the SuppressionInfo changes could also be tested this way if its constructor is made @VisibleForTesting.

But: complete testing of the modified methods, without changing method visibility, requires constructing suitable Symbol instances. For this I see two options:

1. Using some boilerplate code to compile dummy code like in SignaturesTest.
2. Using a CompilationTestHelper, which would require defining the test class in core rather than check_api.

I think option (2) will yield the most robust and maintainable code, and this is the approach also taken for SuggestedFixes and SuggestedFixesTest, but splitting code across modules like this is not optimal. So before I present a PR that requires rework: any opinions on the preferred approach? :)

from error-prone.