Fix BugChecker.isSuppressed to actually look only at the current tree.

And warn about a similar pitfall in the already deprecated `ASTHelpers.getAnnotation`.

And add some tests, which show that we already had this right for `SuppressibleTreePathScanner` and for the automatic `BugChecker` behavior (from unknown commit), just not for manual `isSuppressed` calls.

Add a couple TODOs about:
- the kinds of trees on which suppressions are valid
- whether the non-deprecated `BugChecker.isSuppressed(Symbol, VisitorState)` might be a footgun.

Compare:
- https://github.com/google/error-prone/blob/18d5cdf10ec1cc798a588d5c48a9e02a8888c6a1/check_api/src/main/java/com/google/errorprone/scanner/Scanner.java#L91
- https://github.com/google/error-prone/blob/18d5cdf10ec1cc798a588d5c48a9e02a8888c6a1/core/src/main/java/com/google/errorprone/refaster/RefasterSuppressionHelper.java#L43
PiperOrigin-RevId: 580619965

check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java

@@ -17,9 +17,9 @@
 package com.google.errorprone.bugpatterns;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.errorprone.util.ASTHelpers.getDeclaredSymbol;
 import static com.google.errorprone.util.ASTHelpers.getModifiers;
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
-import static com.google.errorprone.util.ASTHelpers.getSymbol;
 
 import com.google.common.collect.ImmutableRangeSet;
 import com.google.common.collect.Iterables;
@@ -283,14 +283,29 @@ private boolean isSuppressed(SuppressWarnings suppression) {
    * bug checker.
    */
   public boolean isSuppressed(Tree tree, VisitorState state) {
-    Symbol sym = getSymbol(tree);
+    Symbol sym = getDeclaredSymbol(tree);
+    /*
+     * TOOD(cpovirk): At least for @SuppressWarnings, should our suppression checks look for
+     * annotations only on the kinds of trees that are covered by SuppressibleTreePathScanner? Or,
+     * now that @SuppressWarnings has been changed to be applicable to all declaration locations,
+     * should we generalize SuppressibleTreePathScanner to look on all those locations?
+     */
     return sym != null && isSuppressed(sym, state);
   }
 
   /**
    * Returns true if the given symbol is annotated with a {@code @SuppressWarnings} or other
    * annotation that disables this bug checker.
    */
+  /*
+   * TODO(cpovirk): Would we consider deleting this overload (or at least making it `private`)? Its
+   * callers appear to all have access to a Tree, and callers might accidentally pass
+   * getSymbol(tree) instead of getDeclaredSymbol(tree), resulting in over-suppression. Fortunately,
+   * the Tree probably provides all that we need, at least for looking for @SuppressWarnings. It
+   * does *not* provide all that we need for looking for any @Inherited suppression annotations (if
+   * such annotations are something that we (a) support and (b) want to support), but we can always
+   * call getDeclaredSymbol inside the implementation where necessary.
+   */
   public boolean isSuppressed(Symbol sym, VisitorState state) {
     ErrorProneOptions errorProneOptions = state.errorProneOptions();
     boolean suppressedInGeneratedCode =

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -1070,8 +1070,14 @@ public static boolean shouldKeep(Tree tree) {
    *
    * @deprecated If {@code annotationClass} contains a member that is a {@code Class} or an array of
    *     them, attempting to access that member from the Error Prone checker code will result in a
-   *     runtime exception. Instead, operate on {@code sym.getAnnotationMirrors()} to
-   *     meta-syntactically inspect the annotation.
+   *     runtime exception. Instead, operate on {@code getSymbol(tree).getAnnotationMirrors()} to
+   *     meta-syntactically inspect the annotation. Note that this method (and the {@code
+   *     getSymbol}-based replacement suggested above) looks for annotations not just on the given
+   *     tree (such as a {@link MethodTree}) but also on the symbol referred to by the given tree
+   *     (such as on the {@link MethodSymbol} that is being called by the given {@link
+   *     MethodInvocationTree}). If you want to examine annotations only on the given tree, then use
+   *     {@link #getAnnotations} (or a direct call to a {@code getAnnotations} method declared on a
+   *     specific {@link Tree} subclass) instead.
    */
   @Nullable
   @Deprecated

core/src/test/java/com/google/errorprone/bugpatterns/BugCheckerTest.java

@@ -18,14 +18,20 @@
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
 
 import com.google.common.collect.ImmutableList;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
 import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreePath;
+import com.sun.source.util.TreePathScanner;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -43,17 +49,17 @@ public void isSuppressed_withoutVisitorState() {
             "    int unsuppressed;",
             "    // BUG: Diagnostic contains: []",
             "    @SuppressWarnings(\"foo\") int unrelatedSuppression;",
-            "    // BUG: Diagnostic contains: [Suppressible]",
+            "    // BUG: Diagnostic contains: [Suppressible, SuppressibleTps, ManualIsSuppressed]",
             "    @SuppressWarnings(\"Suppressible\") int suppressed;",
-            "    // BUG: Diagnostic contains: [Suppressible]",
+            "    // BUG: Diagnostic contains: [Suppressible, SuppressibleTps, ManualIsSuppressed]",
             "    @SuppressWarnings(\"Alternative\") int suppressedWithAlternativeName;",
-            "    // BUG: Diagnostic contains: [Suppressible]",
+            "    // BUG: Diagnostic contains: [Suppressible, SuppressibleTps, ManualIsSuppressed]",
             "    @SuppressWarnings(\"all\") int allSuppressed;",
-            "    // BUG: Diagnostic contains: [Suppressible]",
+            "    // BUG: Diagnostic contains: [Suppressible, SuppressibleTps, ManualIsSuppressed]",
             "    @SuppressWarnings({\"foo\", \"Suppressible\"}) int alsoSuppressed;",
-            "    // BUG: Diagnostic contains: [Suppressible]",
+            "    // BUG: Diagnostic contains: [Suppressible, SuppressibleTps, ManualIsSuppressed]",
             "    @SuppressWarnings({\"all\", \"foo\"}) int redundantlySuppressed;",
-            "    // BUG: Diagnostic contains: [Suppressible]",
+            "    // BUG: Diagnostic contains: [Suppressible, SuppressibleTps, ManualIsSuppressed]",
             "    @SuppressWarnings({\"all\", \"OnlySuppressedInsideDeprecatedCode\"}) int"
                 + " ineffectiveSuppression;",
             "    // BUG: Diagnostic contains: []",
@@ -79,6 +85,14 @@ public void isSuppressed() {
             "    @SuppressWarnings(\"all\") int allSuppressed;",
             "    @SuppressWarnings({\"foo\", \"Suppressible\"}) int alsoSuppressed;",
             "    @SuppressWarnings({\"all\", \"foo\"}) int redundantlySuppressed;",
+            "    System.out.println(s(() -> {",
+            "      // BUG: Diagnostic contains: ",
+            "      int insideCalToMethodWhoseDeclarationHasASuppression;",
+            "    }));",
+            "  }",
+            "  @SuppressWarnings(\"all\")",
+            "  String s(Runnable r) {",
+            "    return \"\";",
             "  }",
             "}")
         .doTest();
@@ -102,6 +116,64 @@ public void isSuppressed_customSuppressionAnnotation() {
         .doTest();
   }
 
+  @Test
+  public void isSuppressed_suppressibleTreePathScanner() {
+    CompilationTestHelper.newInstance(SuppressibleTreePathScannerCheck.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  void m() {",
+            "    // BUG: Diagnostic contains:",
+            "    int unsuppressed;",
+            "    // BUG: Diagnostic contains:",
+            "    @SuppressWarnings(\"foo\") int unrelatedSuppression;",
+            "    @SuppressWarnings(\"Suppressible\") int suppressed;",
+            "    @SuppressWarnings(\"Alternative\") int suppressedWithAlternativeName;",
+            "    @SuppressWarnings(\"all\") int allSuppressed;",
+            "    @SuppressWarnings({\"foo\", \"Suppressible\"}) int alsoSuppressed;",
+            "    @SuppressWarnings({\"all\", \"foo\"}) int redundantlySuppressed;",
+            "    System.out.println(s(() -> {",
+            "      // BUG: Diagnostic contains: ",
+            "      int insideCalToMethodWhoseDeclarationHasASuppression;",
+            "    }));",
+            "  }",
+            "  @SuppressWarnings(\"all\")",
+            "  String s(Runnable r) {",
+            "    return \"\";",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void isSuppressed_manualIsSuppressed() {
+    CompilationTestHelper.newInstance(ManuallySuppressibleCheck.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  void m() {",
+            "    // BUG: Diagnostic contains:",
+            "    int unsuppressed;",
+            "    // BUG: Diagnostic contains:",
+            "    @SuppressWarnings(\"foo\") int unrelatedSuppression;",
+            "    @SuppressWarnings(\"Suppressible\") int suppressed;",
+            "    @SuppressWarnings(\"Alternative\") int suppressedWithAlternativeName;",
+            "    @SuppressWarnings(\"all\") int allSuppressed;",
+            "    @SuppressWarnings({\"foo\", \"Suppressible\"}) int alsoSuppressed;",
+            "    @SuppressWarnings({\"all\", \"foo\"}) int redundantlySuppressed;",
+            "    System.out.println(s(() -> {",
+            "      // BUG: Diagnostic contains: ",
+            "      int insideCalToMethodWhoseDeclarationHasASuppression;",
+            "    }));",
+            "  }",
+            "  @SuppressWarnings(\"all\")",
+            "  String s(Runnable r) {",
+            "    return \"\";",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void isSuppressed_disableWarningsInGeneratedCode() {
     CompilationTestHelper.newInstance(CustomSuppressibilityCheck.class, getClass())
@@ -142,7 +214,11 @@ public void isSuppressed_disableWarningsInGeneratedCode() {
   public static final class LegacySuppressionCheck extends BugChecker
       implements VariableTreeMatcher {
     private final ImmutableList<BugChecker> checks =
-        ImmutableList.of(new SuppressibleCheck(), new CustomSuppressibilityCheck());
+        ImmutableList.of(
+            new SuppressibleCheck(),
+            new CustomSuppressibilityCheck(),
+            new SuppressibleTreePathScannerCheck(),
+            new ManuallySuppressibleCheck());
 
     @Override
     @SuppressWarnings("deprecation") // testing deprecated method
@@ -181,4 +257,74 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
       return describeMatch(tree);
     }
   }
+
+  @BugPattern(
+      name = "SuppressibleTps",
+      altNames = {"Suppressible", "Alternative"},
+      summary =
+          "Should be suppressible with `@SuppressWarnings` but is implemented with"
+              + " SuppressibleTreePathScanner",
+      severity = ERROR)
+  public static final class SuppressibleTreePathScannerCheck extends BugChecker
+      implements CompilationUnitTreeMatcher {
+    @Override
+    public Description matchCompilationUnit(
+        CompilationUnitTree tree, VisitorState stateForCompilationUnit) {
+      new SuppressibleTreePathScanner<Void, Void>(stateForCompilationUnit) {
+        @Override
+        public Void visitVariable(VariableTree tree, Void unused) {
+          state().reportMatch(describeMatch(tree));
+          return null;
+        }
+
+        private VisitorState state() {
+          return stateForCompilationUnit.withPath(getCurrentPath());
+        }
+      }.scan(tree, null);
+      return NO_MATCH;
+    }
+  }
+
+  @BugPattern(
+      name = "ManualIsSuppressed",
+      altNames = {"Suppressible", "Alternative"},
+      summary =
+          "Should be suppressible with `@SuppressWarnings` but is implemented with manual"
+              + " isSuppressed checks",
+      severity = ERROR)
+  public static final class ManuallySuppressibleCheck extends BugChecker
+      implements CompilationUnitTreeMatcher {
+    @Override
+    public Description matchCompilationUnit(
+        CompilationUnitTree compilationUnit, VisitorState stateForCompilationUnit) {
+      new TreePathScanner<Void, Void>() {
+        @Override
+        public Void scan(Tree tree, Void unused) {
+          if (isSuppressed(tree, state())) {
+            return null;
+          }
+          return super.scan(tree, unused);
+        }
+
+        @Override
+        public Void scan(TreePath path, Void unused) {
+          if (isSuppressed(path.getLeaf(), stateForCompilationUnit.withPath(path))) {
+            return null;
+          }
+          return super.scan(path, unused);
+        }
+
+        @Override
+        public Void visitVariable(VariableTree tree, Void unused) {
+          state().reportMatch(describeMatch(tree));
+          return null;
+        }
+
+        private VisitorState state() {
+          return stateForCompilationUnit.withPath(getCurrentPath());
+        }
+      }.scan(stateForCompilationUnit.getPath(), null);
+      return NO_MATCH;
+    }
+  }
 }