Skip to content

[draft] initial implementation of emitting warnings for unneeded suppressions #4828

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[draft] initial implementation of emitting warnings for unneeded suppressions #4828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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;
Expand All @@ -171,6 +177,7 @@ private ErrorProneOptions(
boolean suggestionsAsWarnings,
boolean enableAllChecksAsWarnings,
boolean disableAllChecks,
boolean warnOnUnneededSuppressions,
boolean isTestOnlyTarget,
boolean isPubliclyVisibleTarget,
ErrorProneFlags flags,
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -367,6 +380,7 @@ public ErrorProneOptions build(ImmutableList<String> remainingArgs) {
suggestionsAsWarnings,
enableAllChecksAsWarnings,
disableAllChecks,
warnOnUnneededSuppressions,
isTestOnlyTarget,
isPubliclyVisibleTarget,
flagsBuilder.build(),
Expand Down Expand Up @@ -419,6 +433,7 @@ public static ErrorProneOptions processArgs(Iterable<String> 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);
Expand Down
166 changes: 157 additions & 9 deletions check_api/src/main/java/com/google/errorprone/SuppressionInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<String> suppressWarningStringsForSymbol;
private final Set<String> usedSuppressWarningStrings = new HashSet<>();
private final ImmutableSet<Name> customSuppressionsForSymbol;
private final @Nullable SuppressionInfoForSymbol parent;

public SuppressionInfoForSymbol(
@Nullable Symbol symbol,
ImmutableSet<String> suppressWarningStringsForSymbol,
ImmutableSet<Name> 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<String> suppressWarningsStrings, Set<Name> customSuppressions, boolean inGeneratedCode) {
this(suppressWarningsStrings, customSuppressions, inGeneratedCode, null);
}

private SuppressionInfo(
Set<String> suppressWarningsStrings,
Set<Name> 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) {
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -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<? extends Name> customSuppressionAnnosToLookFor) {
Symbol sym,
VisitorState state,
Set<? extends Name> customSuppressionAnnosToLookFor,
Set<String> warnOnUnneededSuppressWarningsStrings) {
boolean newInGeneratedCode = inGeneratedCode || isGenerated(sym);
boolean anyModification = newInGeneratedCode != inGeneratedCode;

/* Handle custom suppression annotations. */
Set<Name> 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<Name> newlyPresent = ASTHelpers.annotationsAmong(sym, lookingFor, state);
Set<Name> newCustomSuppressions;
Expand All @@ -151,6 +250,7 @@ public SuppressionInfo withExtendedSuppressions(
Name suppressLint = ANDROID_SUPPRESS_LINT.get(state);
Name valueName = VALUE.get(state);
Set<String> newSuppressions = null;
Set<String> 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)
Expand All @@ -167,6 +267,9 @@ public SuppressionInfo withExtendedSuppressions(
newSuppressions = new HashSet<>(suppressWarningsStrings);
}
newSuppressions.add(suppressedWarning);
if (warnOnUnneededSuppressWarningsStrings.contains(suppressedWarning)) {
newWarnOnUnneededSuppressions.add(suppressedWarning);
}
}
}
} else {
Expand All @@ -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;
}
}
}
Loading