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.
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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);
}
}
}
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
continue;
}

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

Expand Down
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit 5d647de

Please sign in to comment.