From 2a22240124cf793e748dfffcedebf43124438d3c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 13 Feb 2025 17:44:25 -0800 Subject: [PATCH] initial implementation of warnings for unneeded suppressions --- .../google/errorprone/ErrorProneOptions.java | 15 ++ .../google/errorprone/SuppressionInfo.java | 166 +++++++++++++- .../com/google/errorprone/VisitorState.java | 27 ++- .../errorprone/bugpatterns/BugChecker.java | 13 +- .../errorprone/scanner/ErrorProneScanner.java | 35 ++- .../google/errorprone/scanner/Scanner.java | 19 +- .../errorprone/bugpatterns/JdkObsolete.java | 5 + .../refaster/RefasterSuppressionHelper.java | 11 +- .../WarnOnUnneededSuppressionsTest.java | 209 ++++++++++++++++++ 9 files changed, 471 insertions(+), 29 deletions(-) create mode 100644 core/src/test/java/com/google/errorprone/suppress/WarnOnUnneededSuppressionsTest.java diff --git a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java index 91e0f5c38d4..f754878b854 100644 --- a/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java +++ b/check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java @@ -58,6 +58,7 @@ public class ErrorProneOptions { private static final String IGNORE_SUPPRESSION_ANNOTATIONS = "-XepIgnoreSuppressionAnnotations"; private static final String DISABLE_ALL_CHECKS = "-XepDisableAllChecks"; private static final String DISABLE_ALL_WARNINGS = "-XepDisableAllWarnings"; + private static final String WARN_ON_UNNEEDED_SUPPRESSIONS = "-XepWarnOnUnneededSuppressions"; private static final String IGNORE_UNKNOWN_CHECKS_FLAG = "-XepIgnoreUnknownCheckNames"; private static final String DISABLE_WARNINGS_IN_GENERATED_CODE_FLAG = "-XepDisableWarningsInGeneratedCode"; @@ -93,6 +94,10 @@ public boolean isDisableAllChecks() { return disableAllChecks; } + public boolean isWarnOnUnneededSuppressions() { + return warnOnUnneededSuppressions; + } + /** * Severity levels for an error-prone check that define how the check results should be presented. */ @@ -153,6 +158,7 @@ abstract static class Builder { private final boolean suggestionsAsWarnings; private final boolean enableAllChecksAsWarnings; private final boolean disableAllChecks; + private final boolean warnOnUnneededSuppressions; private final boolean isTestOnlyTarget; private final boolean isPubliclyVisibleTarget; private final ErrorProneFlags flags; @@ -171,6 +177,7 @@ private ErrorProneOptions( boolean suggestionsAsWarnings, boolean enableAllChecksAsWarnings, boolean disableAllChecks, + boolean warnOnUnneededSuppressions, boolean isTestOnlyTarget, boolean isPubliclyVisibleTarget, ErrorProneFlags flags, @@ -187,6 +194,7 @@ private ErrorProneOptions( this.suggestionsAsWarnings = suggestionsAsWarnings; this.enableAllChecksAsWarnings = enableAllChecksAsWarnings; this.disableAllChecks = disableAllChecks; + this.warnOnUnneededSuppressions = warnOnUnneededSuppressions; this.isTestOnlyTarget = isTestOnlyTarget; this.isPubliclyVisibleTarget = isPubliclyVisibleTarget; this.flags = flags; @@ -260,6 +268,7 @@ private static class Builder { private boolean suggestionsAsWarnings = false; private boolean enableAllChecksAsWarnings = false; private boolean disableAllChecks = false; + private boolean warnOnUnneededSuppressions = false; private boolean isTestOnlyTarget = false; private boolean isPubliclyVisibleTarget = false; private boolean ignoreSuppressionAnnotations = false; @@ -344,6 +353,10 @@ public void setDisableAllChecks(boolean disableAllChecks) { this.disableAllChecks = disableAllChecks; } + public void setWarnOnUnneededSuppressions(boolean warnOnUnneededSuppressions) { + this.warnOnUnneededSuppressions = warnOnUnneededSuppressions; + } + public void setTestOnlyTarget(boolean isTestOnlyTarget) { this.isTestOnlyTarget = isTestOnlyTarget; } @@ -367,6 +380,7 @@ public ErrorProneOptions build(ImmutableList remainingArgs) { suggestionsAsWarnings, enableAllChecksAsWarnings, disableAllChecks, + warnOnUnneededSuppressions, isTestOnlyTarget, isPubliclyVisibleTarget, flagsBuilder.build(), @@ -419,6 +433,7 @@ public static ErrorProneOptions processArgs(Iterable args) { case SUGGESTIONS_AS_WARNINGS_FLAG -> builder.setSuggestionsAsWarnings(true); case ENABLE_ALL_CHECKS -> builder.setEnableAllChecksAsWarnings(true); case DISABLE_ALL_CHECKS -> builder.setDisableAllChecks(true); + case WARN_ON_UNNEEDED_SUPPRESSIONS -> builder.setWarnOnUnneededSuppressions(true); case COMPILING_TEST_ONLY_CODE -> builder.setTestOnlyTarget(true); case COMPILING_PUBLICLY_VISIBLE_CODE -> builder.setPubliclyVisibleTarget(true); case DISABLE_ALL_WARNINGS -> builder.setDisableAllWarnings(true); 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 da48ddffb1c..7721489c162 100644 --- a/check_api/src/main/java/com/google/errorprone/SuppressionInfo.java +++ b/check_api/src/main/java/com/google/errorprone/SuppressionInfo.java @@ -16,25 +16,33 @@ package com.google.errorprone; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; + import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.Immutable; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Suppressible; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.Tree; import com.sun.source.util.SimpleTreeVisitor; +import com.sun.source.util.Trees; import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.processing.JavacProcessingEnvironment; import com.sun.tools.javac.util.Name; import com.sun.tools.javac.util.Pair; import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import org.jspecify.annotations.Nullable; /** * Immutable container of "suppression signals" - annotations or other information gathered from @@ -61,11 +69,85 @@ public class SuppressionInfo { private final boolean inGeneratedCode; + @BugPattern(summary = "Warns when a @SuppressWarnings is not needed", severity = WARNING) + private static final class UnusedSuppressionChecker extends BugChecker {} + + private static final UnusedSuppressionChecker UNUSED_SUPPRESSION_CHECKER = + new UnusedSuppressionChecker(); + + // for tracking unneeded suppressions + static class SuppressionInfoForSymbol { + private final @Nullable Symbol symbol; + private final ImmutableSet suppressWarningStringsForSymbol; + private final Set usedSuppressWarningStrings = new HashSet<>(); + private final ImmutableSet customSuppressionsForSymbol; + private final @Nullable SuppressionInfoForSymbol parent; + + public SuppressionInfoForSymbol( + @Nullable Symbol symbol, + ImmutableSet suppressWarningStringsForSymbol, + ImmutableSet customSuppressionsForSymbol, + @Nullable SuppressionInfoForSymbol parent) { + this.symbol = symbol; + this.suppressWarningStringsForSymbol = suppressWarningStringsForSymbol; + this.customSuppressionsForSymbol = customSuppressionsForSymbol; + this.parent = parent; + } + + public void markSuppressionStringAsUsed(String suppressionName) { + if (suppressWarningStringsForSymbol.contains(suppressionName)) { + usedSuppressWarningStrings.add(suppressionName); + } else if (parent != null) { + parent.markSuppressionStringAsUsed(suppressionName); + } else { + throw new IllegalArgumentException("Suppression string not found: " + suppressionName); + } + } + } + + private final @Nullable SuppressionInfoForSymbol infoForClosestSymbol; + + public void updatedUsedSuppressions(Suppressed suppressed) { + String suppressionName = suppressed.getSuppressionName(); + if (suppressionName != null && suppressed.isUsed()) { + infoForClosestSymbol.markSuppressionStringAsUsed(suppressionName); + } + } + + public void warnOnUnusedSuppressions(VisitorState state) { + if (infoForClosestSymbol == null) { + return; + } + for (String warn : infoForClosestSymbol.suppressWarningStringsForSymbol) { + if (!infoForClosestSymbol.usedSuppressWarningStrings.contains(warn)) { + Tree tree = + Trees.instance(JavacProcessingEnvironment.instance(state.context)) + .getTree(infoForClosestSymbol.symbol); + Description description = + UNUSED_SUPPRESSION_CHECKER + .buildDescription(tree) + .setMessage("Unnecessary @SuppressWarnings(\"" + warn + "\")") + .overrideSeverity(WARNING) + .build(); + state.reportMatch(description); + } + } + } + private SuppressionInfo( Set suppressWarningsStrings, Set customSuppressions, boolean inGeneratedCode) { + this(suppressWarningsStrings, customSuppressions, inGeneratedCode, null); + } + + private SuppressionInfo( + Set suppressWarningsStrings, + Set customSuppressions, + boolean inGeneratedCode, + @Nullable SuppressionInfoForSymbol infoForClosestSymbol) { this.suppressWarningsStrings = ImmutableSet.copyOf(suppressWarningsStrings); this.customSuppressions = ImmutableSet.copyOf(customSuppressions); this.inGeneratedCode = inGeneratedCode; + this.infoForClosestSymbol = infoForClosestSymbol; } private static boolean isGenerated(Symbol sym) { @@ -82,18 +164,29 @@ private static boolean isGenerated(Symbol sym) { public SuppressedState suppressedState( Suppressible suppressible, boolean suppressedInGeneratedCode, VisitorState state) { if (inGeneratedCode && suppressedInGeneratedCode) { - return SuppressedState.SUPPRESSED; + return new Suppressed(null); } if (suppressible.supportsSuppressWarnings() && (suppressWarningsStrings.contains("all") || !Collections.disjoint(suppressible.allNames(), suppressWarningsStrings))) { - return SuppressedState.SUPPRESSED; + String name; + if (suppressWarningsStrings.contains("all")) { + name = "all"; + } else { + // find the first name that suppresses this check + name = + suppressible.allNames().stream() + .filter(suppressWarningsStrings::contains) + .findAny() + .get(); + } + return new Suppressed(name); } if (suppressible.suppressedByAnyOf(customSuppressions, state)) { - return SuppressedState.SUPPRESSED; + return new Suppressed(null); } - return SuppressedState.UNSUPPRESSED; + return Unsuppressed.UNSUPPRESSED; } /** @@ -128,14 +221,20 @@ public Void visitClass(ClassTree node, Void unused) { * @param sym The {@code Symbol} for the AST node currently being scanned * @param state VisitorState for checking the current tree, as well as for getting the {@code * SuppressWarnings symbol type}. + * @param warnOnUnneededSuppressWarningsStrings */ public SuppressionInfo withExtendedSuppressions( - Symbol sym, VisitorState state, Set customSuppressionAnnosToLookFor) { + Symbol sym, + VisitorState state, + Set customSuppressionAnnosToLookFor, + Set warnOnUnneededSuppressWarningsStrings) { boolean newInGeneratedCode = inGeneratedCode || isGenerated(sym); boolean anyModification = newInGeneratedCode != inGeneratedCode; /* Handle custom suppression annotations. */ Set lookingFor = new HashSet<>(customSuppressionAnnosToLookFor); + // TODO what if we have nested suppressions with the same customSuppression annotation? + // Is the inner one unused? Let's just say no for now. We'll report on the outermost one. lookingFor.removeAll(customSuppressions); Set newlyPresent = ASTHelpers.annotationsAmong(sym, lookingFor, state); Set newCustomSuppressions; @@ -151,6 +250,7 @@ public SuppressionInfo withExtendedSuppressions( Name suppressLint = ANDROID_SUPPRESS_LINT.get(state); Name valueName = VALUE.get(state); Set newSuppressions = null; + Set newWarnOnUnneededSuppressions = new HashSet<>(); // Iterate over annotations on this symbol, looking for SuppressWarnings for (Attribute.Compound attr : sym.getAnnotationMirrors()) { if ((attr.type.tsym == state.getSymtab().suppressWarningsType.tsym) @@ -167,6 +267,9 @@ public SuppressionInfo withExtendedSuppressions( newSuppressions = new HashSet<>(suppressWarningsStrings); } newSuppressions.add(suppressedWarning); + if (warnOnUnneededSuppressWarningsStrings.contains(suppressedWarning)) { + newWarnOnUnneededSuppressions.add(suppressedWarning); + } } } } else { @@ -187,11 +290,56 @@ public SuppressionInfo withExtendedSuppressions( if (newSuppressions == null) { newSuppressions = suppressWarningsStrings; } - return new SuppressionInfo(newSuppressions, newCustomSuppressions, newInGeneratedCode); + + SuppressionInfoForSymbol newInfoForClosestSymbol = + new SuppressionInfoForSymbol( + sym, + ImmutableSet.copyOf(newWarnOnUnneededSuppressions), + ImmutableSet.copyOf(newlyPresent), + infoForClosestSymbol); + return new SuppressionInfo( + newSuppressions, newCustomSuppressions, newInGeneratedCode, newInfoForClosestSymbol); } - public enum SuppressedState { - UNSUPPRESSED, - SUPPRESSED + public sealed interface SuppressedState permits Suppressed, Unsuppressed { + boolean isSuppressed(); + } + + public static final class Unsuppressed implements SuppressedState { + public static final Unsuppressed UNSUPPRESSED = new Unsuppressed(); + + private Unsuppressed() {} + + @Override + public boolean isSuppressed() { + return false; + } + } + + public static final class Suppressed implements SuppressedState { + private final @Nullable String suppressionName; + + private boolean used = false; + + public Suppressed(@Nullable String suppressionName) { + this.suppressionName = suppressionName; + } + + public @Nullable String getSuppressionName() { + return suppressionName; + } + + public boolean isUsed() { + return used; + } + + public void setAsUsed() { + this.used = true; + } + + @Override + public boolean isSuppressed() { + return true; + } } } diff --git a/check_api/src/main/java/com/google/errorprone/VisitorState.java b/check_api/src/main/java/com/google/errorprone/VisitorState.java index 75e6d68040c..280eee8c550 100644 --- a/check_api/src/main/java/com/google/errorprone/VisitorState.java +++ b/check_api/src/main/java/com/google/errorprone/VisitorState.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.errorprone.SuppressionInfo.Unsuppressed.UNSUPPRESSED; import static com.google.errorprone.util.ASTHelpers.getStartPosition; import com.google.common.annotations.VisibleForTesting; @@ -92,7 +93,7 @@ public static VisitorState createForUtilityPurposes(Context context) { // Can't use this VisitorState to report results, so no-op collector. StatisticsCollector.createNoOpCollector(), null, - SuppressedState.UNSUPPRESSED); + UNSUPPRESSED); } /** @@ -108,7 +109,7 @@ public static VisitorState createForCustomFindingCollection( ErrorProneOptions.empty(), StatisticsCollector.createCollector(), null, - SuppressedState.UNSUPPRESSED); + UNSUPPRESSED); } /** @@ -126,7 +127,7 @@ public static VisitorState createConfiguredForCompilation( errorProneOptions, StatisticsCollector.createCollector(), null, - SuppressedState.UNSUPPRESSED); + UNSUPPRESSED); } /** @@ -145,7 +146,7 @@ public VisitorState(Context context) { // Can't use this VisitorState to report results, so no-op collector. StatisticsCollector.createNoOpCollector(), null, - SuppressedState.UNSUPPRESSED); + UNSUPPRESSED); } /** @@ -163,7 +164,7 @@ public VisitorState(Context context, DescriptionListener listener) { ErrorProneOptions.empty(), StatisticsCollector.createCollector(), null, - SuppressedState.UNSUPPRESSED); + UNSUPPRESSED); } /** @@ -184,7 +185,7 @@ public VisitorState( errorProneOptions, StatisticsCollector.createCollector(), null, - SuppressedState.UNSUPPRESSED); + UNSUPPRESSED); } /** @@ -293,6 +294,14 @@ public void reportMatch(Description description) { if (override != null) { description = description.applySeverityOverride(override); } + if (suppressedState.isSuppressed()) { + // we used it + ((SuppressionInfo.Suppressed) suppressedState).setAsUsed(); + if (errorProneOptions().isWarnOnUnneededSuppressions()) { + // For now, don't report suppressed findings if we're going to warn on unused suppressions. + return; + } + } sharedState.statisticsCollector.incrementCounter(statsKey(description.checkName + "-findings")); // TODO(glorioso): I believe it is correct to still emit regular findings since the @@ -303,7 +312,11 @@ public void reportMatch(Description description) { } private String statsKey(String key) { - return suppressedState == SuppressedState.SUPPRESSED ? key + "-suppressed" : key; + return suppressedState.isSuppressed() ? key + "-suppressed" : key; + } + + public SuppressedState getSuppressedState() { + return suppressedState; } /** 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 84fee1a7e64..8a89bcb18cf 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 @@ -320,9 +320,10 @@ public boolean isSuppressed(Symbol sym, VisitorState state) { && state.severityMap().get(canonicalName()) != SeverityLevel.ERROR; SuppressionInfo.SuppressedState suppressedState = SuppressionInfo.EMPTY - .withExtendedSuppressions(sym, state, customSuppressionAnnotationNames.get(state)) + .withExtendedSuppressions( + sym, state, customSuppressionAnnotationNames.get(state), allNames()) .suppressedState(BugChecker.this, suppressedInGeneratedCode, state); - return suppressedState == SuppressionInfo.SuppressedState.SUPPRESSED; + return suppressedState.isSuppressed(); } private final Supplier> customSuppressionAnnotationNames = @@ -349,6 +350,14 @@ public Void scan(Tree tree, Void unused) { return ImmutableRangeSet.copyOf(suppressedRegions); } + /** + * Returns true if this checker supports unneeded suppression warnings. False by default; + * supporting checkers should override. + */ + public boolean supportsUnneededSuppressionWarnings() { + return false; + } + public interface AnnotatedTypeTreeMatcher extends Suppressible { Description matchAnnotatedType(AnnotatedTypeTree tree, VisitorState state); } diff --git a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java index cc3b3ac9eab..53683c414be 100644 --- a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java +++ b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java @@ -22,6 +22,7 @@ import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.ErrorProneError; import com.google.errorprone.ErrorProneOptions; +import com.google.errorprone.SuppressionInfo; import com.google.errorprone.SuppressionInfo.SuppressedState; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -175,6 +176,12 @@ public class ErrorProneScanner extends Scanner { private final Map severities; private final ImmutableSet bugCheckers; + /** + * If the WarnOnUnneededSuppressions option is set, this set contains all the names of + * {@code @SuppressWarnings} annotations for which we should warn if they are unused. + */ + private final ImmutableSet warnOnUnneededSuppressWarningStrings; + /** * Create an error-prone scanner for the given checkers. * @@ -204,9 +211,13 @@ public ErrorProneScanner(Iterable checkers, Map> annotationClassesBuilder = ImmutableSet.builder(); + ImmutableSet.Builder warnOnUnneededSuppressWarningStringsBuilder = + ImmutableSet.builder(); for (BugChecker checker : this.bugCheckers) { - registerNodeTypes(checker, annotationClassesBuilder); + registerNodeTypes( + checker, annotationClassesBuilder, warnOnUnneededSuppressWarningStringsBuilder); } + this.warnOnUnneededSuppressWarningStrings = warnOnUnneededSuppressWarningStringsBuilder.build(); ImmutableSet> annotationClasses = annotationClassesBuilder.build(); this.customSuppressionAnnotations = VisitorState.memoize( @@ -299,9 +310,12 @@ protected Set getCustomSuppressionAnnotations(VisitorState state private void registerNodeTypes( BugChecker checker, - ImmutableSet.Builder> customSuppressionAnnotationClasses) { + ImmutableSet.Builder> customSuppressionAnnotationClasses, + ImmutableSet.Builder warnOnUnneededSuppressWarningStringsBuilder) { customSuppressionAnnotationClasses.addAll(checker.customSuppressionAnnotations()); - + if (checker.supportsUnneededSuppressionWarnings()) { + warnOnUnneededSuppressWarningStringsBuilder.addAll(checker.allNames()); + } if (checker instanceof AnnotatedTypeTreeMatcher annotatedTypeTreeMatcher) { annotatedTypeMatchers.add(annotatedTypeTreeMatcher); } @@ -500,14 +514,20 @@ private VisitorState processMatchers( for (M matcher : matchers) { SuppressedState suppressed = isSuppressed(matcher, errorProneOptions, newState); // If the ErrorProneOptions say to visit suppressed code, we still visit it - if (suppressed == SuppressedState.UNSUPPRESSED - || errorProneOptions.isIgnoreSuppressionAnnotations()) { + if (!suppressed.isSuppressed() + || errorProneOptions.isIgnoreSuppressionAnnotations() + || (errorProneOptions.isWarnOnUnneededSuppressions() + && warnOnUnneededSuppressWarningStrings.contains( + ((SuppressionInfo.Suppressed) suppressed).getSuppressionName()))) { try (AutoCloseable unused = oldState.timingSpan(matcher)) { // We create a new VisitorState with the suppression info specific to this matcher. VisitorState stateWithSuppressionInformation = newState.withSuppression(suppressed); reportMatch( processingFunction.process(matcher, tree, stateWithSuppressionInformation), stateWithSuppressionInformation); + if (suppressed.isSuppressed()) { + currentSuppressions.updatedUsedSuppressions((SuppressionInfo.Suppressed) suppressed); + } } catch (Exception | AssertionError t) { handleError(matcher, t); } @@ -1068,4 +1088,9 @@ public Map severityMap() { public ImmutableSet getBugCheckers() { return this.bugCheckers; } + + @Override + protected Set getWarnOnUnneededSuppressWarningStrings() { + return warnOnUnneededSuppressWarningStrings; + } } diff --git a/check_api/src/main/java/com/google/errorprone/scanner/Scanner.java b/check_api/src/main/java/com/google/errorprone/scanner/Scanner.java index f1a1780f8bd..fd9650a372a 100644 --- a/check_api/src/main/java/com/google/errorprone/scanner/Scanner.java +++ b/check_api/src/main/java/com/google/errorprone/scanner/Scanner.java @@ -48,7 +48,7 @@ @CheckReturnValue public class Scanner extends TreePathScanner { - private SuppressionInfo currentSuppressions = SuppressionInfo.EMPTY; + protected SuppressionInfo currentSuppressions = SuppressionInfo.EMPTY; /** Scan a tree from a position identified by a TreePath. */ @Override @@ -58,6 +58,10 @@ public Void scan(TreePath path, VisitorState state) { return super.scan(path, state); } finally { // Restore old suppression state. + if (state.errorProneOptions().isWarnOnUnneededSuppressions() + && currentSuppressions != prevSuppressionInfo) { + currentSuppressions.warnOnUnusedSuppressions(state); + } currentSuppressions = prevSuppressionInfo; } } @@ -74,6 +78,10 @@ public Void scan(Tree tree, VisitorState state) { return super.scan(tree, state); } finally { // Restore old suppression state. + if (state.errorProneOptions().isWarnOnUnneededSuppressions() + && currentSuppressions != prevSuppressionInfo) { + currentSuppressions.warnOnUnusedSuppressions(state); + } currentSuppressions = prevSuppressionInfo; } } @@ -91,7 +99,10 @@ private SuppressionInfo updateSuppressions(Tree tree, VisitorState state) { if (sym != null) { currentSuppressions = currentSuppressions.withExtendedSuppressions( - sym, state, getCustomSuppressionAnnotations(state)); + sym, + state, + getCustomSuppressionAnnotations(state), + getWarnOnUnneededSuppressWarningStrings()); } } return prevSuppressionInfo; @@ -121,6 +132,10 @@ protected Set getCustomSuppressionAnnotations(VisitorState state return ImmutableSet.of(); } + protected Set getWarnOnUnneededSuppressWarningStrings() { + return ImmutableSet.of(); + } + protected void reportMatch(Description description, VisitorState state) { checkNotNull(description, "Use Description.NO_MATCH to denote an absent finding."); state.reportMatch(description); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java b/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java index fb0d3aa4730..dd4c0079657 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JdkObsolete.java @@ -423,4 +423,9 @@ private static boolean implementingObsoleteMethod( } return true; } + + @Override + public boolean supportsUnneededSuppressionWarnings() { + return true; + } } diff --git a/core/src/main/java/com/google/errorprone/refaster/RefasterSuppressionHelper.java b/core/src/main/java/com/google/errorprone/refaster/RefasterSuppressionHelper.java index adbeb30096b..48fb5ed68c9 100644 --- a/core/src/main/java/com/google/errorprone/refaster/RefasterSuppressionHelper.java +++ b/core/src/main/java/com/google/errorprone/refaster/RefasterSuppressionHelper.java @@ -44,12 +44,15 @@ static boolean suppressed(RefasterRule rule, Tree tree, Context context) { if (sym == null) { return false; } + RefasterSuppressible suppressible = new RefasterSuppressible(rule); return SuppressionInfo.EMPTY .withExtendedSuppressions( - sym, state, /* customSuppressionAnnosToLookFor= */ ImmutableSet.of()) - .suppressedState( - new RefasterSuppressible(rule), /* suppressedInGeneratedCode= */ false, state) - .equals(SuppressionInfo.SuppressedState.SUPPRESSED); + sym, + state, + /* customSuppressionAnnosToLookFor= */ ImmutableSet.of(), + suppressible.allNames()) + .suppressedState(suppressible, /* suppressedInGeneratedCode= */ false, state) + .isSuppressed(); } /** Adapts a {@link RefasterRule} into a {@link Suppressible}. */ diff --git a/core/src/test/java/com/google/errorprone/suppress/WarnOnUnneededSuppressionsTest.java b/core/src/test/java/com/google/errorprone/suppress/WarnOnUnneededSuppressionsTest.java new file mode 100644 index 00000000000..6f35b4ae200 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/suppress/WarnOnUnneededSuppressionsTest.java @@ -0,0 +1,209 @@ +/* + * Copyright 2025 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.suppress; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.MethodInvocationTree; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class WarnOnUnneededSuppressionsTest { + + @BugPattern( + name = "NoCallsToFoo", + summary = "No calls to foo", + severity = BugPattern.SeverityLevel.ERROR) + static final class NoCallsToFoo extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher { + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + String methodSelect = tree.getMethodSelect().toString(); + if (methodSelect.contains("foo")) { + return buildDescription(tree).setMessage("No calls to foo").build(); + } + return Description.NO_MATCH; + } + + @Override + public boolean supportsUnneededSuppressionWarnings() { + return true; + } + } + + @BugPattern( + name = "NoCallsToFooUnsupported", + summary = "No calls to foo", + severity = BugPattern.SeverityLevel.ERROR) + static final class NoCallsToFooUnsupported extends BugChecker + implements BugChecker.MethodInvocationTreeMatcher { + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + String methodSelect = tree.getMethodSelect().toString(); + if (methodSelect.contains("foo")) { + return buildDescription(tree).setMessage("No calls to foo").build(); + } + return Description.NO_MATCH; + } + } + + private final CompilationTestHelper testHelperSupported = + CompilationTestHelper.newInstance(NoCallsToFoo.class, getClass()) + .setArgs("-XepWarnOnUnneededSuppressions") + .matchAllDiagnostics(); + + private final CompilationTestHelper testHelperUnsupported = + CompilationTestHelper.newInstance(NoCallsToFooUnsupported.class, getClass()) + .setArgs("-XepWarnOnUnneededSuppressions") + .matchAllDiagnostics(); + + @Test + public void classLevelSuppression() { + testHelperSupported + .addSourceLines( + "TestPositive.java", + """ + @SuppressWarnings("NoCallsToFoo") + // BUG: Diagnostic contains: Unnecessary + class TestPositive { + void test() { + bar(); + } + void bar() {} + } + """) + .addSourceLines( + "TestNegative.java", + """ + @SuppressWarnings("NoCallsToFoo") + class TestNegative { + void test() { + foo(); + } + void foo() {} + } + """) + .doTest(); + } + + @Test + public void suppressionUnsupportedButUsed() { + testHelperUnsupported + .addSourceLines( + "TestNegative.java", + """ + @SuppressWarnings("NoCallsToFooUnsupported") + class TestNegative { + void test() { + foo(); + } + void foo() {} + } + """) + .doTest(); + } + + @Test + public void methodLevelSuppression() { + testHelperSupported + .addSourceLines( + "TestPositive.java", + """ + class TestPositive { + @SuppressWarnings("NoCallsToFoo") + // BUG: Diagnostic contains: Unnecessary + void test() { + bar(); + } + void bar() {} + } + """) + .addSourceLines( + "TestNegative.java", + """ + class TestNegative { + @SuppressWarnings("NoCallsToFoo") + void test() { + foo(); + } + void foo() {} + } + """) + .doTest(); + } + + @Test + public void fieldLevelSuppression() { + testHelperSupported + .addSourceLines( + "TestPositive.java", + """ + class TestPositive { + @SuppressWarnings("NoCallsToFoo") + // BUG: Diagnostic contains: Unnecessary + Object x = bar(); + Object bar() { return null; } + } + """) + .addSourceLines( + "TestNegative.java", + """ + class TestNegative { + @SuppressWarnings("NoCallsToFoo") + Object x = foo(); + Object foo() { return null; } + } + """) + .doTest(); + } + + @Test + public void localVariableLevelSuppression() { + testHelperSupported + .addSourceLines( + "TestPositive.java", + """ + class TestPositive { + void test() { + @SuppressWarnings("NoCallsToFoo") + // BUG: Diagnostic contains: Unnecessary + Object x = bar(); + } + Object bar() { return null; } + } + """) + .addSourceLines( + "TestNegative.java", + """ + class TestNegative { + void test() { + @SuppressWarnings("NoCallsToFoo") + Object x = foo(); + } + Object foo() { return null; } + } + """) + .doTest(); + } +}