From 5ce8792e980373533dd1c3e4baa10ed9e826a5ca Mon Sep 17 00:00:00 2001 From: cpovirk Date: Wed, 8 Nov 2023 12:35:14 -0800 Subject: [PATCH] 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 --- .../errorprone/bugpatterns/BugChecker.java | 19 ++- .../google/errorprone/util/ASTHelpers.java | 10 +- .../bugpatterns/BugCheckerTest.java | 160 +++++++++++++++++- 3 files changed, 178 insertions(+), 11 deletions(-) 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 73cfa5b9584..b853bc629b5 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 @@ -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,7 +283,13 @@ 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); } @@ -291,6 +297,15 @@ public boolean isSuppressed(Tree tree, VisitorState 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 = diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 1834ec74145..3dd9a4175a8 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/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 diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/BugCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/BugCheckerTest.java index ad435d73b35..5e3fce12ba5 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/BugCheckerTest.java +++ b/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 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(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() { + @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; + } + } }