Skip to content

Commit

Permalink
Reconcile BugChecker#isSuppressed with suppression handling in `Err…
Browse files Browse the repository at this point in the history
…orProneScanner`

This makes the full, dizzying complexity of `ErrorProneScanner`'s suppression
handling available in `BugChecker#isSuppressed`, including handling of custom
suppression annotations and `-XepDisableWarningsInGeneratedCode`.

#3094

PiperOrigin-RevId: 441580794
  • Loading branch information
cushon authored and Error Prone Team committed Apr 13, 2022
1 parent 47c2b05 commit 5d647de
Show file tree
Hide file tree
Showing 46 changed files with 141 additions and 64 deletions.
Expand Up @@ -262,6 +262,10 @@ public ErrorProneOptions errorProneOptions() {
return sharedState.errorProneOptions;
}

public Map<String, SeverityLevel> severityMap() {
return sharedState.severityMap;
}

public void reportMatch(Description description) {
checkNotNull(description, "Use Description.NO_MATCH to denote an absent finding.");
if (description == Description.NO_MATCH) {
Expand Down
Expand Up @@ -19,12 +19,15 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
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;
import com.google.common.collect.Range;
import com.google.errorprone.BugCheckerInfo;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.ErrorProneOptions;
import com.google.errorprone.SuppressionInfo;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.CheckReturnValue;
import com.google.errorprone.fixes.Fix;
Expand Down Expand Up @@ -248,17 +251,17 @@ public boolean suppressedByAnyOf(Set<Name> annotations, VisitorState s) {
}

/**
* Returns true if the given tree is annotated with a {@code @SuppressWarnings} that disables this
* bug checker.
* @deprecated use {@link #isSuppressed(Tree, VisitorState)} instead
*/
@Deprecated
public boolean isSuppressed(Tree tree) {
return isSuppressed(ASTHelpers.getAnnotation(tree, SuppressWarnings.class));
}

/**
* Returns true if the given symbol is annotated with a {@code @SuppressWarnings} that disables
* this bug checker.
* @deprecated use {@link #isSuppressed(Symbol, VisitorState)} instead
*/
@Deprecated
public boolean isSuppressed(Symbol symbol) {
return isSuppressed(ASTHelpers.getAnnotation(symbol, SuppressWarnings.class));
}
Expand All @@ -268,13 +271,45 @@ private boolean isSuppressed(SuppressWarnings suppression) {
&& !Collections.disjoint(Arrays.asList(suppression.value()), allNames());
}

/**
* Returns true if the given tree is annotated with a {@code @SuppressWarnings} that disables this
* bug checker.
*/
public boolean isSuppressed(Tree tree, VisitorState state) {
Symbol sym = getSymbol(tree);
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.
*/
public boolean isSuppressed(Symbol sym, VisitorState state) {
ErrorProneOptions errorProneOptions = state.errorProneOptions();
boolean suppressedInGeneratedCode =
errorProneOptions.disableWarningsInGeneratedCode()
&& state.severityMap().get(canonicalName()) != SeverityLevel.ERROR;
SuppressionInfo.SuppressedState suppressedState =
SuppressionInfo.EMPTY
.withExtendedSuppressions(sym, state, customSuppressionAnnotationNames.get(state))
.suppressedState(BugChecker.this, suppressedInGeneratedCode, state);
return suppressedState == SuppressionInfo.SuppressedState.SUPPRESSED;
}

private final Supplier<? extends Set<? extends Name>> customSuppressionAnnotationNames =
VisitorState.memoize(
state ->
customSuppressionAnnotations().stream()
.map(a -> state.getName(a.getName()))
.collect(toImmutableSet()));

/** Computes a RangeSet of code regions which are suppressed by this bug checker. */
public ImmutableRangeSet<Integer> suppressedRegions(VisitorState state) {
ImmutableRangeSet.Builder<Integer> suppressedRegions = ImmutableRangeSet.builder();
new TreeScanner<Void, Void>() {
@Override
public Void scan(Tree tree, Void unused) {
if (getModifiers(tree) != null && isSuppressed(tree)) {
if (getModifiers(tree) != null && isSuppressed(tree, state)) {
suppressedRegions.add(Range.closed(getStartPosition(tree), state.getEndPosition(tree)));
} else {
super.scan(tree, null);
Expand Down Expand Up @@ -517,11 +552,34 @@ public int hashCode() {

/** A {@link TreePathScanner} which skips trees which are suppressed for this check. */
protected class SuppressibleTreePathScanner<A, B> extends TreePathScanner<A, B> {

// TODO(cushon): make this protected once it is required; currently it would shadow
// other variables named state and break checks that pass the deprecated constructor
private final VisitorState state;

public SuppressibleTreePathScanner(VisitorState state) {
this.state = state;
}

/**
* @deprecated use {@link #SuppressibleTreePathScanner(VisitorState)} instead
*/
@Deprecated
public SuppressibleTreePathScanner() {
this(null);
}

@Override
public A scan(Tree tree, B b) {
boolean isSuppressible =
tree instanceof ClassTree || tree instanceof MethodTree || tree instanceof VariableTree;
return isSuppressible && isSuppressed(tree) ? null : super.scan(tree, b);
if (isSuppressible) {
boolean suppressed = state != null ? isSuppressed(tree, state) : isSuppressed(tree);
if (suppressed) {
return null;
}
}
return super.scan(tree, b);
}
}
}
Expand Up @@ -80,6 +80,7 @@ private final class IfScanner extends SuppressibleTreePathScanner<Void, Void> {
private final VisitorState state;

private IfScanner(VisitorState state) {
super(state);
this.state = state;
}

Expand Down
Expand Up @@ -67,7 +67,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
continue;
}
MethodSymbol msym = getSymbol((MethodTree) member);
if (isSuppressed(msym)) {
if (isSuppressed(msym, state)) {
continue;
}
List<MethodSymbol> clash = methods.remove(methodReferenceDescriptor(types, msym));
Expand Down
Expand Up @@ -131,7 +131,7 @@ private static Matcher<MethodTree> returning(String type) {
public Description matchClass(ClassTree tree, VisitorState state) {
if (ASTHelpers.hasAnnotation(tree, "com.google.auto.value.AutoValue", state)) {
for (Tree memberTree : tree.getMembers()) {
if (memberTree instanceof MethodTree && !isSuppressed(memberTree)) {
if (memberTree instanceof MethodTree && !isSuppressed(memberTree, state)) {
MethodTree methodTree = (MethodTree) memberTree;
if (ABSTRACT_MATCHER.matches(methodTree, state)) {
for (Map.Entry<String, Matcher<MethodTree>> entry : REPLACEMENT_TO_MATCHERS.entries()) {
Expand Down
Expand Up @@ -66,7 +66,7 @@ private void scanAndReportAutoValueReferences(
CompilationUnitTree tree,
ImmutableSet<Type> autoValueClassesFromThisFile,
VisitorState state) {
new SuppressibleTreePathScanner<Void, Void>() {
new SuppressibleTreePathScanner<Void, Void>(state) {

@Override
public Void visitClass(ClassTree classTree, Void unused) {
Expand Down
Expand Up @@ -171,7 +171,7 @@ private Description buildDescription(
CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();
TreePath path = TreePath.getPath(compilationUnit, compilationUnit);
IdentifierTree firstFound =
new SuppressibleTreePathScanner<IdentifierTree, Void>() {
new SuppressibleTreePathScanner<IdentifierTree, Void>(state) {
@Override
public IdentifierTree reduce(IdentifierTree r1, IdentifierTree r2) {
return (r2 != null) ? r2 : r1;
Expand All @@ -180,7 +180,7 @@ public IdentifierTree reduce(IdentifierTree r1, IdentifierTree r2) {
@Override
public IdentifierTree visitIdentifier(IdentifierTree node, Void unused) {
Symbol nodeSymbol = getSymbol(node);
if (symbols.contains(nodeSymbol) && !isSuppressed(node)) {
if (symbols.contains(nodeSymbol) && !isSuppressed(node, state)) {
if (getCurrentPath().getParentPath().getLeaf().getKind() != Kind.CASE) {
builder.prefixWith(node, enclosingReplacement);
moveTypeAnnotations(node);
Expand Down
Expand Up @@ -52,7 +52,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
for (Tree member : tree.getTypeDecls()) {
if (member instanceof ClassTree) {
ClassTree classMember = (ClassTree) member;
if (isSuppressed(classMember)) {
if (isSuppressed(classMember, state)) {
// If any top-level classes have @SuppressWarnings("ClassName"), ignore
// this compilation unit. We can't rely on the normal suppression
// mechanism because the only enclosing element is the package declaration,
Expand Down
Expand Up @@ -39,7 +39,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
if (state.errorProneOptions().isTestOnlyTarget()) {
return Description.NO_MATCH;
}
if (tree.getTypeDecls().stream().anyMatch(s -> isSuppressed(s))) {
if (tree.getTypeDecls().stream().anyMatch(s -> isSuppressed(s, state))) {
return Description.NO_MATCH;
}
if (tree.getTypeDecls().stream()
Expand Down
Expand Up @@ -185,7 +185,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
ImmutableListMultimap<VarSymbol, Type> assignedTypes = getAssignedTypes(state);

new SuppressibleTreePathScanner<Void, Void>() {
new SuppressibleTreePathScanner<Void, Void>(state) {
@Override
public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
handleTree(tree, getSymbol(tree));
Expand Down
Expand Up @@ -59,7 +59,7 @@ public class EqualsHashCode extends BugChecker implements ClassTreeMatcher {
@Override
public Description matchClass(ClassTree classTree, VisitorState state) {
MethodTree methodTree = checkMethodPresence(classTree, state/* expectedNoArgMethod= */ );
if (methodTree == null || isSuppressed(methodTree)) {
if (methodTree == null || isSuppressed(methodTree, state)) {
return NO_MATCH;
}
return describeMatch(methodTree);
Expand Down
Expand Up @@ -257,7 +257,7 @@ private FinalScanner(VariableAssignmentRecords writes, VisitorState compilationS
@Override
public Void visitVariable(VariableTree node, InitializationContext init) {
VarSymbol sym = ASTHelpers.getSymbol(node);
if (sym.getKind() == ElementKind.FIELD && !isSuppressed(node)) {
if (sym.getKind() == ElementKind.FIELD && !isSuppressed(node, compilationState)) {
writes.recordDeclaration(sym, node);
}
return super.visitVariable(node, InitializationContext.NONE);
Expand Down Expand Up @@ -317,7 +317,7 @@ private boolean isThisAccess(Tree tree) {
public Void visitClass(ClassTree node, InitializationContext init) {
VisitorState state = compilationState.withPath(getCurrentPath());

if (isSuppressed(node)) {
if (isSuppressed(node, state)) {
return null;
}

Expand Down
Expand Up @@ -79,7 +79,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
SetMultimap<VarSymbol, Tree> uses =
MultimapBuilder.linkedHashKeys().linkedHashSetValues().build();

new SuppressibleTreePathScanner<Void, Void>() {
new SuppressibleTreePathScanner<Void, Void>(state) {
@Override
public Void visitVariable(VariableTree variableTree, Void unused) {
VarSymbol symbol = getSymbol(variableTree);
Expand Down Expand Up @@ -118,7 +118,7 @@ private boolean canBeUsedOnLocalVariable(AnnotationTree annotationTree) {

@Override
public Void visitClass(ClassTree classTree, Void unused) {
if (isSuppressed(classTree)) {
if (isSuppressed(classTree, state)) {
return null;
}
inMethod = false;
Expand Down
Expand Up @@ -112,7 +112,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
continue;
}

if (isSuppressed(member)) {
if (isSuppressed(member, state)) {
continue;
}

Expand Down
Expand Up @@ -58,7 +58,11 @@ public Description matchClass(ClassTree classTree, VisitorState visitorState) {
classTree.getMembers().stream()
.filter(mem -> mem instanceof VariableTree)
.map(mem -> (VariableTree) mem)
.filter(mem -> !isSuppressed(getSymbol(mem)) && !isIgnoredType(mem) && !isStatic(mem))
.filter(
mem ->
!isSuppressed(getSymbol(mem), visitorState)
&& !isIgnoredType(mem)
&& !isStatic(mem))
.collect(toCollection(ArrayList::new));

ClassSymbol classSymbol = getSymbol(classTree);
Expand Down
Expand Up @@ -126,7 +126,7 @@ public Description matchClass(ClassTree classTree, VisitorState state) {
classTree.getMembers().stream()
.filter(member -> PRIVATE_FINAL_VAR_MATCHER.matches(member, state))
.filter(member -> !EXCLUSIONS.matches(member, state))
.filter(member -> !isSuppressed(member))
.filter(member -> !isSuppressed(member, state))
.map(VariableTree.class::cast)
.flatMap(varTree -> stream(isReplaceable(varTree, state)))
.collect(toImmutableMap(ReplaceableVar::symbol, var -> var));
Expand Down
Expand Up @@ -119,7 +119,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
SuggestedFix.Builder fix = SuggestedFix.builder();
Optional<VariableTree> firstReplacement = Optional.empty();
for (VariableTree var : immutableListVar) {
if (isSuppressed(var)) {
if (isSuppressed(var, state)) {
continue;
}
if (!usageScanner.disallowedVarUsages.get(getSymbol(var))) {
Expand Down
Expand Up @@ -165,7 +165,7 @@ public Description matchTry(TryTree tree, VisitorState state) {
&& !blockChecksForInterruptedException(catchTree.getBlock(), state)
&& !(exceptionIsRethrown(catchTree, catchTree.getParameter(), state)
&& methodDeclaresInterruptedException(state.findEnclosing(MethodTree.class), state))
&& !isSuppressed(catchTree.getParameter())) {
&& !isSuppressed(catchTree.getParameter(), state)) {
return describeMatch(catchTree, createFix(catchTree));
}
}
Expand Down
Expand Up @@ -93,7 +93,7 @@ public final class JUnit3TestNotRun extends BugChecker implements CompilationUni
@Override
public Description matchCompilationUnit(CompilationUnitTree unused, VisitorState state) {
ImmutableSet<MethodSymbol> calledMethods = calledMethods(state);
new SuppressibleTreePathScanner<Void, Void>() {
new SuppressibleTreePathScanner<Void, Void>(state) {
@Override
public Void visitMethod(MethodTree tree, Void unused) {
checkMethod(tree, calledMethods, state.withPath(getCurrentPath()))
Expand Down
Expand Up @@ -103,11 +103,11 @@ public Description matchClass(ClassTree tree, VisitorState state) {
}
Map<MethodSymbol, MethodTree> suspiciousMethods = new HashMap<>();
for (Tree member : tree.getMembers()) {
if (!(member instanceof MethodTree) || isSuppressed(member)) {
if (!(member instanceof MethodTree) || isSuppressed(member, state)) {
continue;
}
MethodTree methodTree = (MethodTree) member;
if (possibleTestMethod.matches(methodTree, state) && !isSuppressed(tree)) {
if (possibleTestMethod.matches(methodTree, state) && !isSuppressed(tree, state)) {
suspiciousMethods.put(getSymbol(methodTree), methodTree);
}
}
Expand Down
Expand Up @@ -77,7 +77,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s

@Override
public Void visitClass(ClassTree classTree, Void unused) {
if (isSuppressed(classTree)) {
if (isSuppressed(classTree, state)) {
suppressions++;
super.visitClass(classTree, null);
suppressions--;
Expand All @@ -89,7 +89,7 @@ public Void visitClass(ClassTree classTree, Void unused) {

@Override
public Void visitMethod(MethodTree tree, Void unused) {
if (isSuppressed(tree)) {
if (isSuppressed(tree, state)) {
suppressions++;
matchMethod(tree);
super.visitMethod(tree, null);
Expand All @@ -103,7 +103,7 @@ public Void visitMethod(MethodTree tree, Void unused) {

@Override
public Void visitVariable(VariableTree variableTree, Void unused) {
if (isSuppressed(variableTree)) {
if (isSuppressed(variableTree, state)) {
suppressions++;
super.visitVariable(variableTree, null);
suppressions--;
Expand Down
Expand Up @@ -193,6 +193,7 @@ private final class ReturnTypesScanner extends SuppressibleTreePathScanner<Void,

private ReturnTypesScanner(
VisitorState state, Set<VarSymbol> immutable, Set<VarSymbol> mutable) {
super(state);
this.state = state;
this.immutable = immutable;
this.mutable = mutable;
Expand Down
Expand Up @@ -60,7 +60,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
case INTERFACE:
case ANNOTATION_TYPE:
case ENUM:
if (isSuppressed(classMember)) {
if (isSuppressed(classMember, state)) {
// If any top-level classes have @SuppressWarnings("TopLevel"), ignore
// this compilation unit. We can't rely on the normal suppression
// mechanism because the only enclosing element is the package declaration,
Expand Down
Expand Up @@ -77,6 +77,7 @@ private final class IfScanner extends SuppressibleTreePathScanner<Void, Void> {
private final VisitorState state;

private IfScanner(VisitorState state) {
super(state);
this.state = state;
}

Expand Down
Expand Up @@ -181,7 +181,7 @@ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {

private ImmutableMap<Symbol, Tree> getFixableTypes(VisitorState state) {
ImmutableMap.Builder<Symbol, Tree> fixableTypes = ImmutableMap.builder();
new SuppressibleTreePathScanner<Void, Void>() {
new SuppressibleTreePathScanner<Void, Void>(state) {
@Override
public Void visitVariable(VariableTree tree, Void unused) {
VarSymbol symbol = getSymbol(tree);
Expand Down
Expand Up @@ -70,7 +70,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
.filter(m -> HAS_PROTECTED.matches(m, state))
.filter(
m -> !(m instanceof MethodTree) || methodHasNoParentMethod((MethodTree) m, state))
.filter(m -> !isSuppressed(m))
.filter(m -> !isSuppressed(m, state))
.collect(toImmutableList());
if (relevantMembers.isEmpty()) {
return NO_MATCH;
Expand Down

0 comments on commit 5d647de

Please sign in to comment.