MissingOverride: ignore generated method

[pacbeckh]

MissingOverride generates warnings for methods that have @Generated annotation.

In #1968 there was a fix done by skipping classes that have the annotation.
However, if just parts of the class are generated, as with Lombok, the class level @Generated annotation is missing.

I would expect methods with @Generated to be ignored/skipped.

Minimal Reproducible Example

package org.example;

import lombok.Generated;

public class AnyClassDeLombok implements AnyInterface {
  private String anything;

  @Generated
  public String getAnything() {
    return this.anything;
  }

}

A full reproducable example is available at https://github.com/pacbeckh/error-prone-missing-override-issue, run mvn clean install

Logs

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project errorproneissue: Compilation failure: Compilation failure: 
[ERROR] /Users/pacovanbeckhoven/hexagon/error-prone-missing-override-issue/src/main/java/org/example/AnyClassDeLombok.java:[12,17] [MissingOverride] getAnything implements method in AnyInterface; expected @Override
[ERROR]     (see https://errorprone.info/bugpattern/MissingOverride)
[ERROR]   Did you mean '@Override @Generated'?
[ERROR] /Users/pacovanbeckhoven/hexagon/error-prone-missing-override-issue/src/main/java/org/example/AnyClass.java:[8,18] [MissingOverride] getAnything implements method in AnyInterface; expected @Override
[ERROR]     (see https://errorprone.info/bugpattern/MissingOverride)
[ERROR]   Did you mean 'private String @Override anything;'?
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

Setup

• Operating system MacOS Ventura.
• Java version 17.0.4).
• Error Prone version 2.21.1.

[Stephan202]

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.

[cushon]

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.

[Stephan202]

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.)

[pacbeckh]

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 👌

[Stephan202]

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? :)

[cushon]

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.

[Stephan202]

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