Skip to content

Commit

Permalink
Refactor Scanner's suppression code a bit to use an immutable suppres…
Browse files Browse the repository at this point in the history 
…sion info

object as opposed to holding onto different bits and pieces of the suppression
individually, as well as resolve some TODO's about avoiding garbage.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=226030707
  • Loading branch information
@nick-someone @ronshapiro
nick-someone authored and ronshapiro committed Dec 24, 2018
1 parent dee5774 commit b56cdf3
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 82 deletions.
95 changes: 55 additions & 40 deletions check_api/src/main/java/com/google/errorprone/SuppressionHelper.java
View file
Expand Up @@ -18,6 +18,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.annotations.Immutable;
import com.google.errorprone.matchers.Suppressible;
import com.google.errorprone.util.ASTHelpers;
import com.sun.tools.javac.code.Attribute;
Expand Down Expand Up @@ -59,21 +60,22 @@ public SuppressionHelper(Set<Class<? extends Annotation>> customSuppressionAnnot
this.customSuppressionAnnotations = customSuppressionAnnotations;
}

/**
* Container for information about suppressions. Either reference field may be null, which
* indicates that the suppression sets are unchanged.
*/
/** Immutable container for information about currently-known-about suppressions. */
@Immutable
public static class SuppressionInfo {
public Set<String> suppressWarningsStrings;
public Set<Class<? extends Annotation>> customSuppressions;
public boolean inGeneratedCode;
public static final SuppressionInfo EMPTY =
new SuppressionInfo(ImmutableSet.of(), ImmutableSet.of(), false);

public final ImmutableSet<String> suppressWarningsStrings;
public final ImmutableSet<Class<? extends Annotation>> customSuppressions;
public final boolean inGeneratedCode;

public SuppressionInfo(
private SuppressionInfo(
Set<String> suppressWarningsStrings,
Set<Class<? extends Annotation>> customSuppressions,
boolean inGeneratedCode) {
this.suppressWarningsStrings = suppressWarningsStrings;
this.customSuppressions = customSuppressions;
this.suppressWarningsStrings = ImmutableSet.copyOf(suppressWarningsStrings);
this.customSuppressions = ImmutableSet.copyOf(customSuppressions);
this.inGeneratedCode = inGeneratedCode;
}
}
Expand All @@ -91,87 +93,100 @@ public SuppressionInfo(
* @param sym The {@code Symbol} for the AST node currently being scanned
* @param suppressWarningsType The {@code Type} for {@code @SuppressWarnings}, as given by javac's
* symbol table
* @param suppressionsOnCurrentPath The set of strings in all {@code @SuppressWarnings}
* annotations on the current path through the AST
* @param customSuppressionsOnCurrentPath The set of all custom suppression annotations
* @param toExtend The suppression info to extend from.
* @param state VisitorState for checking the current tree
*/
public SuppressionInfo extendSuppressionSets(
Symbol sym,
Type suppressWarningsType,
Set<String> suppressionsOnCurrentPath,
Set<Class<? extends Annotation>> customSuppressionsOnCurrentPath,
boolean inGeneratedCode,
VisitorState state) {
Symbol sym, Type suppressWarningsType, SuppressionInfo toExtend, VisitorState state) {
boolean newInGeneratedCode = toExtend.inGeneratedCode || isGenerated(sym, state);
boolean anyModification = newInGeneratedCode != toExtend.inGeneratedCode;

boolean newInGeneratedCode = inGeneratedCode || isGenerated(sym, state);

/** Handle custom suppression annotations. */
/* Handle custom suppression annotations. */
Set<Class<? extends Annotation>> newCustomSuppressions = null;
for (Class<? extends Annotation> annotationType : customSuppressionAnnotations) {
// Don't need to check already-suppressed annos
if (toExtend.customSuppressions.contains(annotationType)) {
continue;
}
if (ASTHelpers.hasAnnotation(sym, annotationType, state)) {
anyModification = true;
if (newCustomSuppressions == null) {
newCustomSuppressions = new HashSet<>(customSuppressionsOnCurrentPath);
newCustomSuppressions = new HashSet<>(toExtend.customSuppressions);
}
newCustomSuppressions.add(annotationType);
}
}

/** Handle {@code @SuppressWarnings} and {@code @SuppressLint}. */
/* Handle {@code @SuppressWarnings} and {@code @SuppressLint}. */
Set<String> newSuppressions = null;
// Iterate over annotations on this symbol, looking for SuppressWarnings
for (Attribute.Compound attr : sym.getAnnotationMirrors()) {
if ((attr.type.tsym == suppressWarningsType.tsym)
|| attr.type.tsym.getQualifiedName().contentEquals("android.annotation.SuppressLint")) {
for (Pair<MethodSymbol, Attribute> value : attr.values) {
if (value.fst.name.contentEquals("value"))
if (value.fst.name.contentEquals("value")) {
if (value.snd
instanceof Attribute.Array) { // SuppressWarnings/SuppressLint take an array
for (Attribute suppress : ((Attribute.Array) value.snd).values) {
if (newSuppressions == null) {
newSuppressions = new HashSet<>(suppressionsOnCurrentPath);
String suppressedWarning = (String) suppress.getValue();
if (!toExtend.suppressWarningsStrings.contains(suppressedWarning)) {
anyModification = true;
if (newSuppressions == null) {
newSuppressions = new HashSet<>(toExtend.suppressWarningsStrings);
}
newSuppressions.add(suppressedWarning);
}
// TODO(eaftan): check return value to see if this was a new warning?
newSuppressions.add((String) suppress.getValue());
}
} else {
throw new RuntimeException(
"Expected SuppressWarnings/SuppressLint annotation to take array type");
}
}
}
}
}

// Since this is invoked every time we descend into a new node, let's save some garbage
// by returning the same instance if there were no changes.
if (!anyModification) {
return toExtend;
}

if (newCustomSuppressions == null) {
newCustomSuppressions = toExtend.customSuppressions;
}
if (newSuppressions == null) {
newSuppressions = toExtend.suppressWarningsStrings;
}
return new SuppressionInfo(newSuppressions, newCustomSuppressions, newInGeneratedCode);
}

/**
* Returns true if this checker should be suppressed on the current tree path.
*
* @param suppressible Holds information about the suppressibilty of a checker
* @param suppressionsOnCurrentPath The set of strings in all {@code @SuppressWarnings}
* annotations on the current path through the AST
* @param customSuppressionsOnCurrentPath The set of all custom suppression annotations on the
* current path through the AST
* @param severityLevel of the check to be suppressed
* @param inGeneratedCode true if the current code is generated
* @param suppressionInfo The current set of suppressions.
* @param disableWarningsInGeneratedCode true if warnings in generated code should be suppressed
*/
public static boolean isSuppressed(
Suppressible suppressible,
Set<String> suppressionsOnCurrentPath,
Set<Class<? extends Annotation>> customSuppressionsOnCurrentPath,
SeverityLevel severityLevel,
boolean inGeneratedCode,
SuppressionInfo suppressionInfo,
boolean disableWarningsInGeneratedCode) {
if (inGeneratedCode && disableWarningsInGeneratedCode && severityLevel != SeverityLevel.ERROR) {

if (suppressionInfo.inGeneratedCode
&& disableWarningsInGeneratedCode
&& severityLevel != SeverityLevel.ERROR) {
return true;
}
if (suppressible.supportsSuppressWarnings()
&& !Collections.disjoint(suppressible.allNames(), suppressionsOnCurrentPath)) {
&& !Collections.disjoint(
suppressible.allNames(), suppressionInfo.suppressWarningsStrings)) {
return true;
}
return !Collections.disjoint(
suppressible.customSuppressionAnnotations(), customSuppressionsOnCurrentPath);
suppressible.customSuppressionAnnotations(), suppressionInfo.customSuppressions);
}

private static boolean isGenerated(Symbol sym, VisitorState state) {
Expand Down
57 changes: 15 additions & 42 deletions check_api/src/main/java/com/google/errorprone/scanner/Scanner.java
View file
Expand Up @@ -19,6 +19,7 @@
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.ErrorProneOptions;
import com.google.errorprone.SuppressionHelper;
import com.google.errorprone.SuppressionHelper.SuppressionInfo;
import com.google.errorprone.VisitorState;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Suppressible;
Expand All @@ -29,7 +30,6 @@
import com.sun.tools.javac.code.Symbol;
import java.lang.annotation.Annotation;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

Expand All @@ -42,32 +42,27 @@
*/
public class Scanner extends TreePathScanner<Void, VisitorState> {

private Set<String> suppressions = new HashSet<>();
private Set<Class<? extends Annotation>> customSuppressions = new HashSet<>();
private boolean inGeneratedCode = false;
private SuppressionInfo currentSuppressions = SuppressionInfo.EMPTY;
// This must be lazily initialized, because the list of custom suppression annotations will
// not be available until after the subclass's constructor has run.
private SuppressionHelper suppressionHelper;

private void initSuppressionHelper() {
private SuppressionHelper suppressionHelper() {
if (suppressionHelper == null) {
suppressionHelper = new SuppressionHelper(getCustomSuppressionAnnotations());
}
return suppressionHelper;
}

/** Scan a tree from a position identified by a TreePath. */
@Override
public Void scan(TreePath path, VisitorState state) {
SuppressionHelper.SuppressionInfo prevSuppressionInfo =
updateSuppressions(path.getLeaf(), state);

SuppressionInfo prevSuppressionInfo = updateSuppressions(path.getLeaf(), state);
try {
return super.scan(path, state);
} finally {
// Restore old suppression state.
suppressions = prevSuppressionInfo.suppressWarningsStrings;
customSuppressions = prevSuppressionInfo.customSuppressions;
inGeneratedCode = prevSuppressionInfo.inGeneratedCode;
currentSuppressions = prevSuppressionInfo;
}
}

Expand All @@ -78,46 +73,28 @@ public Void scan(Tree tree, VisitorState state) {
return null;
}

SuppressionHelper.SuppressionInfo prevSuppressionInfo = updateSuppressions(tree, state);
SuppressionInfo prevSuppressionInfo = updateSuppressions(tree, state);
try {
return super.scan(tree, state);
} finally {
// Restore old suppression state.
suppressions = prevSuppressionInfo.suppressWarningsStrings;
customSuppressions = prevSuppressionInfo.customSuppressions;
inGeneratedCode = prevSuppressionInfo.inGeneratedCode;
currentSuppressions = prevSuppressionInfo;
}
}

/**
* Updates current suppression state with information for the given {@code tree}. Returns the
* previous suppression state so that it can be restored when going up the tree.
*/
private SuppressionHelper.SuppressionInfo updateSuppressions(Tree tree, VisitorState state) {
SuppressionHelper.SuppressionInfo prevSuppressionInfo =
new SuppressionHelper.SuppressionInfo(suppressions, customSuppressions, inGeneratedCode);

initSuppressionHelper();

private SuppressionInfo updateSuppressions(Tree tree, VisitorState state) {
SuppressionInfo prevSuppressionInfo = currentSuppressions;
Symbol sym = ASTHelpers.getDeclaredSymbol(tree);
if (sym != null) {
SuppressionHelper.SuppressionInfo newSuppressions =
suppressionHelper.extendSuppressionSets(
sym,
state.getSymtab().suppressWarningsType,
suppressions,
customSuppressions,
inGeneratedCode,
state);
if (newSuppressions.suppressWarningsStrings != null) {
suppressions = newSuppressions.suppressWarningsStrings;
}
if (newSuppressions.customSuppressions != null) {
customSuppressions = newSuppressions.customSuppressions;
}
inGeneratedCode = newSuppressions.inGeneratedCode;
currentSuppressions =
suppressionHelper()
.extendSuppressionSets(
sym, state.getSymtab().suppressWarningsType, currentSuppressions, state);
}

return prevSuppressionInfo;
}

Expand All @@ -127,14 +104,10 @@ private SuppressionHelper.SuppressionInfo updateSuppressions(Tree tree, VisitorS
* @param suppressible holds information about the suppressibility of a checker
*/
protected boolean isSuppressed(Suppressible suppressible, ErrorProneOptions errorProneOptions) {
initSuppressionHelper();

return SuppressionHelper.isSuppressed(
suppressible,
suppressions,
customSuppressions,
severityMap().get(suppressible.canonicalName()),
inGeneratedCode,
currentSuppressions,
errorProneOptions.disableWarningsInGeneratedCode());
}

Expand Down

0 comments on commit b56cdf3

Please sign in to comment.