Skip to content

Commit

Permalink
Merge ddce326 into 62f343a
Browse files Browse the repository at this point in the history
  • Loading branch information
@lazaroclapp
lazaroclapp committed Jul 23, 2020
2 parents 62f343a + ddce326 commit e7b0c59
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 56 deletions.
40 changes: 37 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
View file
Expand Up @@ -25,11 +25,14 @@
import static com.uber.nullaway.ErrorMessage.MessageTypes.FIELD_NO_INIT;
import static com.uber.nullaway.ErrorMessage.MessageTypes.GET_ON_EMPTY_OPTIONAL;
import static com.uber.nullaway.ErrorMessage.MessageTypes.METHOD_NO_INIT;
import static com.uber.nullaway.ErrorMessage.MessageTypes.NONNULL_FIELD_READ_BEFORE_INIT;
import static com.uber.nullaway.NullAway.CORE_CHECK_NAME;
import static com.uber.nullaway.NullAway.INITIALIZATION_CHECK_NAME;
import static com.uber.nullaway.NullAway.OPTIONAL_CHECK_NAME;
import static com.uber.nullaway.NullAway.getTreesInstance;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.errorprone.VisitorState;
Expand Down Expand Up @@ -102,8 +105,18 @@ public Description createErrorDescription(
Description.Builder descriptionBuilder,
VisitorState state) {
Description.Builder builder = descriptionBuilder.setMessage(errorMessage.message);
if (errorMessage.messageType.equals(GET_ON_EMPTY_OPTIONAL)
&& hasPathSuppression(state.getPath(), OPTIONAL_CHECK_NAME)) {
String checkName = CORE_CHECK_NAME;
if (errorMessage.messageType.equals(GET_ON_EMPTY_OPTIONAL)) {
checkName = OPTIONAL_CHECK_NAME;
} else if (errorMessage.messageType.equals(FIELD_NO_INIT)
|| errorMessage.messageType.equals(METHOD_NO_INIT)
|| errorMessage.messageType.equals(NONNULL_FIELD_READ_BEFORE_INIT)) {
checkName = INITIALIZATION_CHECK_NAME;
}

// Mildly expensive state.getPath() traversal, occurs only once per potentially
// reported error.
if (hasPathSuppression(state.getPath(), checkName)) {
return Description.NO_MATCH;
}

Expand Down Expand Up @@ -136,7 +149,10 @@ private boolean hasPathSuppression(TreePath treePath, String subcheckerName) {
.filter(ErrorBuilder::canHaveSuppressWarningsAnnotation)
.map(tree -> ASTHelpers.getSymbol(tree))
.filter(symbol -> symbol != null)
.anyMatch(symbol -> symbolHasSuppressWarningsAnnotation(symbol, subcheckerName));
.anyMatch(
symbol ->
symbolHasSuppressWarningsAnnotation(symbol, subcheckerName)
|| symbolIsExcludedClassSymbol(symbol));
}

private Description.Builder addSuggestedSuppression(
Expand Down Expand Up @@ -300,6 +316,9 @@ void reportInitializerError(
String message,
VisitorState state,
Description.Builder descriptionBuilder) {
// Check needed here, despite check in hasPathSuppression because initialization
// checking happens at the class-level (meaning state.getPath() might not include the
// method itself).
if (symbolHasSuppressWarningsAnnotation(methodSymbol, INITIALIZATION_CHECK_NAME)) {
return;
}
Expand All @@ -323,6 +342,18 @@ boolean symbolHasSuppressWarningsAnnotation(Symbol symbol, String suppression) {
return false;
}

private boolean symbolIsExcludedClassSymbol(Symbol symbol) {
if (symbol instanceof Symbol.ClassSymbol) {
ImmutableSet<String> excludedClassAnnotations = config.getExcludedClassAnnotations();
return ((Symbol.ClassSymbol) symbol)
.getAnnotationMirrors()
.stream()
.map(anno -> anno.getAnnotationType().toString())
.anyMatch(excludedClassAnnotations::contains);
}
return false;
}

static int getLineNumForElement(Element uninitField, VisitorState state) {
Tree tree = getTreesInstance(state).getTree(uninitField);
if (tree == null)
Expand Down Expand Up @@ -374,6 +405,9 @@ static String errMsgForInitializer(Set<Element> uninitFields, VisitorState state
}

void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Builder builder) {
// Check needed here, despite check in hasPathSuppression because initialization
// checking happens at the class-level (meaning state.getPath() might not include the
// field itself).
if (symbolHasSuppressWarningsAnnotation(symbol, INITIALIZATION_CHECK_NAME)) {
return;
}
Expand Down
87 changes: 35 additions & 52 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Expand Up @@ -92,7 +92,6 @@
import com.uber.nullaway.dataflow.EnclosingEnvironmentNullness;
import com.uber.nullaway.handlers.Handler;
import com.uber.nullaway.handlers.Handlers;
import java.lang.annotation.Annotation;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -173,13 +172,15 @@ public class NullAway extends BugChecker

static final String INITIALIZATION_CHECK_NAME = "NullAway.Init";
static final String OPTIONAL_CHECK_NAME = "NullAway.Optional";
// Unmatched, used for when we only want full checker suppressions to work
static final String CORE_CHECK_NAME = "NullAway.<core>";

private static final Matcher<ExpressionTree> THIS_MATCHER = NullAway::isThisIdentifierMatcher;

private final Predicate<MethodInvocationNode> nonAnnotatedMethod;

/** should we match within the current class? */
private boolean matchWithinClass = true;
/** should we match within the current top level class? */
private boolean matchWithinTopLevelClass = true;

private final Config config;

Expand Down Expand Up @@ -220,8 +221,6 @@ public class NullAway extends BugChecker
*/
private final Map<ExpressionTree, Nullness> computedNullnessMap = new LinkedHashMap<>();

private final ImmutableSet<Class<? extends Annotation>> customSuppressionAnnotations;

/**
* Error Prone requires us to have an empty constructor for each Plugin, in addition to the
* constructor taking an ErrorProneFlags object. This constructor should not be used anywhere
Expand All @@ -232,35 +231,19 @@ public NullAway() {
config = new DummyOptionsConfig();
handler = Handlers.buildEmpty();
nonAnnotatedMethod = this::isMethodUnannotated;
customSuppressionAnnotations = ImmutableSet.of();
errorBuilder = new ErrorBuilder(config, "", ImmutableSet.of());
}

public NullAway(ErrorProneFlags flags) {
config = new ErrorProneCLIFlagsConfig(flags);
handler = Handlers.buildDefault(config);
nonAnnotatedMethod = this::isMethodUnannotated;
customSuppressionAnnotations = initCustomSuppressions();
errorBuilder = new ErrorBuilder(config, canonicalName(), allNames());
// workaround for Checker Framework static state bug;
// See https://github.com/typetools/checker-framework/issues/1482
AnnotationUtils.clear();
}

private ImmutableSet<Class<? extends Annotation>> initCustomSuppressions() {
ImmutableSet.Builder<Class<? extends Annotation>> builder = ImmutableSet.builder();
builder.addAll(super.customSuppressionAnnotations());
for (String annotName : config.getExcludedClassAnnotations()) {
try {
builder.add(Class.forName(annotName).asSubclass(Annotation.class));
} catch (ClassNotFoundException e) {
// in this case, the annotation may be a source file currently being compiled,
// in which case we won't be able to resolve the class
}
}
return builder.build();
}

private boolean isMethodUnannotated(MethodInvocationNode invocationNode) {
return invocationNode == null
|| NullabilityUtil.isUnannotated(ASTHelpers.getSymbol(invocationNode.getTree()), config);
Expand All @@ -272,18 +255,13 @@ public String linkUrl() {
return config.getErrorURL() + " ";
}

@Override
public Set<Class<? extends Annotation>> customSuppressionAnnotations() {
return customSuppressionAnnotations;
}

/**
* We are trying to see if (1) we are in a method guaranteed to return something non-null, and (2)
* this return statement can return something null.
*/
@Override
public Description matchReturn(ReturnTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
handler.onMatchReturn(this, tree, state);
Expand Down Expand Up @@ -319,7 +297,7 @@ public Description matchReturn(ReturnTree tree, VisitorState state) {

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
final Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree);
Expand All @@ -334,7 +312,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

@Override
public Description matchNewClass(NewClassTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree);
Expand Down Expand Up @@ -392,7 +370,7 @@ private Symbol.MethodSymbol getSymbolOfSuperConstructor(

@Override
public Description matchAssignment(AssignmentTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Type lhsType = ASTHelpers.getType(tree.getVariable());
Expand Down Expand Up @@ -423,7 +401,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {

@Override
public Description matchCompoundAssignment(CompoundAssignmentTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Type lhsType = ASTHelpers.getType(tree.getVariable());
Expand All @@ -437,7 +415,7 @@ public Description matchCompoundAssignment(CompoundAssignmentTree tree, VisitorS

@Override
public Description matchArrayAccess(ArrayAccessTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Description description = matchDereference(tree.getExpression(), tree, state);
Expand All @@ -450,7 +428,7 @@ public Description matchArrayAccess(ArrayAccessTree tree, VisitorState state) {

@Override
public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Symbol symbol = ASTHelpers.getSymbol(tree);
Expand All @@ -474,7 +452,7 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state)

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
// if the method is overriding some other method,
Expand All @@ -497,7 +475,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {

@Override
public Description matchSwitch(SwitchTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}

Expand Down Expand Up @@ -665,7 +643,7 @@ private Description checkReturnExpression(

@Override
public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Symbol.MethodSymbol funcInterfaceMethod =
Expand Down Expand Up @@ -703,7 +681,7 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
*/
@Override
public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
Symbol.MethodSymbol referencedMethod = ASTHelpers.getSymbol(tree);
Expand Down Expand Up @@ -777,7 +755,7 @@ && getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE)) {

@Override
public Description matchIdentifier(IdentifierTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
return checkForReadBeforeInit(tree, state);
Expand Down Expand Up @@ -1114,7 +1092,7 @@ private boolean okToReadBeforeInitialized(TreePath path) {

@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
VarSymbol symbol = ASTHelpers.getSymbol(tree);
Expand Down Expand Up @@ -1142,19 +1120,24 @@ public Description matchVariable(VariableTree tree, VisitorState state) {

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
// check if the class is excluded according to the filter
// Check if the class is excluded according to the filter
// if so, set the flag to match within the class to false
// NOTE: for this mechanism to work, we rely on the enclosing ClassTree
// always being visited before code within that class. We also
// assume that a single checker object is not being
// used from multiple threads
// We don't want to update the flag for nested classes.
// Ideally we would keep a stack of flags to handle nested types,
// but this is not easy within the Error Prone APIs.
// Instead, we use this flag as an optimization, skipping work if the
// top-level class is to be skipped. If a nested class should be
// skipped, we instead rely on last-minute suppression of the
// error message, using the mechanism in
// ErrorBuilder.hasPathSuppression(...)
Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(tree);
// we don't want to update the flag for nested classes.
// ideally we would keep a stack of flags to handle nested types,
// but this is not easy within the Error Prone APIs
NestingKind nestingKind = classSymbol.getNestingKind();
if (!nestingKind.isNested()) {
matchWithinClass = !isExcludedClass(classSymbol);
matchWithinTopLevelClass = !isExcludedClass(classSymbol);
// since we are processing a new top-level class, invalidate any cached
// results for previous classes
handler.onMatchTopLevelClass(this, tree, state, classSymbol);
Expand All @@ -1165,7 +1148,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
computedNullnessMap.clear();
EnclosingEnvironmentNullness.instance(state.context).clear();
}
if (matchWithinClass) {
if (matchWithinTopLevelClass) {
// we need to update the environment before checking field initialization, as the latter
// may run dataflow analysis
if (nestingKind.equals(NestingKind.LOCAL) || nestingKind.equals(NestingKind.ANONYMOUS)) {
Expand All @@ -1180,7 +1163,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {

@Override
public Description matchBinary(BinaryTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
ExpressionTree leftOperand = tree.getLeftOperand();
Expand All @@ -1201,7 +1184,7 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {

@Override
public Description matchUnary(UnaryTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
return doUnboxingCheck(state, tree.getExpression());
Expand All @@ -1210,31 +1193,31 @@ public Description matchUnary(UnaryTree tree, VisitorState state) {
@Override
public Description matchConditionalExpression(
ConditionalExpressionTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
return doUnboxingCheck(state, tree.getCondition());
}

@Override
public Description matchIf(IfTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
return doUnboxingCheck(state, tree.getCondition());
}

@Override
public Description matchWhileLoop(WhileLoopTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
return doUnboxingCheck(state, tree.getCondition());
}

@Override
public Description matchForLoop(ForLoopTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
if (tree.getCondition() != null) {
Expand All @@ -1245,7 +1228,7 @@ public Description matchForLoop(ForLoopTree tree, VisitorState state) {

@Override
public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState state) {
if (!matchWithinClass) {
if (!matchWithinTopLevelClass) {
return Description.NO_MATCH;
}
ExpressionTree expr = tree.getExpression();
Expand Down

0 comments on commit e7b0c59

Please sign in to comment.