Skip to content

Commit

Permalink
refactor: streamline mayBeNullExpr flow
Browse files Browse the repository at this point in the history
the uniform flow is now:
- return early for obvious expression types
- determine initial exprMayBeNull through simple checks
- apply handler overrides in single place
- finally run dataflow if exprMayBeNull is true in single place
  • Loading branch information
XN137 committed Apr 1, 2023
1 parent e4dfeac commit 3127f16
Showing 1 changed file with 26 additions and 41 deletions.
67 changes: 26 additions & 41 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -2197,18 +2197,13 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) {
// obviously not null
return false;
}
if (expr.getKind() == Tree.Kind.NULL_LITERAL) {
// obviously null, so same as for above literals we return early without consulting handlers
return true;
}
// the logic here is to avoid doing dataflow analysis whenever possible
Symbol exprSymbol = ASTHelpers.getSymbol(expr);
boolean exprMayBeNull;
// return early for expressions that no handler overrides and will not need dataflow analysis
switch (expr.getKind()) {
case NULL_LITERAL:
// obviously null
return true;
case ARRAY_ACCESS:
// unsound! we cannot check for nullness of array contents yet
exprMayBeNull = false;
break;
case NEW_CLASS:
case NEW_ARRAY:
// for string concatenation, auto-boxing
Expand Down Expand Up @@ -2258,29 +2253,34 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) {
case RIGHT_SHIFT:
case UNSIGNED_RIGHT_SHIFT:
// clearly not null
exprMayBeNull = false;
return false;
default:
break;
}
// the logic here is to avoid doing dataflow analysis whenever possible
Symbol exprSymbol = ASTHelpers.getSymbol(expr);
boolean exprMayBeNull;
switch (expr.getKind()) {
case MEMBER_SELECT:
if (exprSymbol == null) {
throw new IllegalStateException(
"unexpected null symbol for dereference expression " + state.getSourceForNode(expr));
}
exprMayBeNull = mayBeNullFieldAccess(state, expr, exprSymbol);
exprMayBeNull =
NullabilityUtil.mayBeNullFieldFromType(exprSymbol, config, codeAnnotationInfo);
break;
case IDENTIFIER:
if (exprSymbol == null) {
throw new IllegalStateException(
"unexpected null symbol for identifier " + state.getSourceForNode(expr));
}
if (exprSymbol.getKind().equals(ElementKind.FIELD)) {
// Special case: mayBeNullFieldAccess runs handler.onOverrideMayBeNullExpr before
// dataflow.
return mayBeNullFieldAccess(state, expr, exprSymbol);
if (exprSymbol.getKind() == ElementKind.FIELD) {
exprMayBeNull =
NullabilityUtil.mayBeNullFieldFromType(exprSymbol, config, codeAnnotationInfo);
} else {
// Check handler.onOverrideMayBeNullExpr before dataflow.
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, true);
return exprMayBeNull ? nullnessFromDataflow(state, expr) : false;
exprMayBeNull = true;
}
break;
case METHOD_INVOCATION:
if (!(exprSymbol instanceof Symbol.MethodSymbol)) {
throw new IllegalStateException(
Expand All @@ -2289,36 +2289,30 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) {
+ " for method invocation "
+ state.getSourceForNode(expr));
}
// Special case: mayBeNullMethodCall runs handler.onOverrideMayBeNullExpr before dataflow.
return mayBeNullMethodCall(state, expr, (Symbol.MethodSymbol) exprSymbol);
exprMayBeNull = mayBeNullMethodCall((Symbol.MethodSymbol) exprSymbol);
break;
case CONDITIONAL_EXPRESSION:
case ASSIGNMENT:
exprMayBeNull = nullnessFromDataflow(state, expr);
exprMayBeNull = true;
break;
default:
// match switch expressions by comparing strings, so the code compiles on JDK versions < 12
if (expr.getKind().name().equals("SWITCH_EXPRESSION")) {
exprMayBeNull = nullnessFromDataflow(state, expr);
exprMayBeNull = true;
} else {
throw new RuntimeException(
"whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr));
}
}
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, exprMayBeNull);
return exprMayBeNull;
return exprMayBeNull && nullnessFromDataflow(state, expr);
}

private boolean mayBeNullMethodCall(
VisitorState state, ExpressionTree expr, Symbol.MethodSymbol exprSymbol) {
boolean exprMayBeNull = true;
private boolean mayBeNullMethodCall(Symbol.MethodSymbol exprSymbol) {
if (codeAnnotationInfo.isSymbolUnannotated(exprSymbol, config)) {
exprMayBeNull = false;
}
if (!Nullness.hasNullableAnnotation(exprSymbol, config)) {
exprMayBeNull = false;
return false;
}
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, exprMayBeNull);
return exprMayBeNull ? nullnessFromDataflow(state, expr) : false;
return Nullness.hasNullableAnnotation(exprSymbol, config);
}

public boolean nullnessFromDataflow(VisitorState state, ExpressionTree expr) {
Expand All @@ -2336,15 +2330,6 @@ public AccessPathNullnessAnalysis getNullnessAnalysis(VisitorState state) {
return AccessPathNullnessAnalysis.instance(state, nonAnnotatedMethod, config, this.handler);
}

private boolean mayBeNullFieldAccess(VisitorState state, ExpressionTree expr, Symbol exprSymbol) {
boolean exprMayBeNull = true;
if (!NullabilityUtil.mayBeNullFieldFromType(exprSymbol, config, codeAnnotationInfo)) {
exprMayBeNull = false;
}
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, exprMayBeNull);
return exprMayBeNull ? nullnessFromDataflow(state, expr) : false;
}

private Description matchDereference(
ExpressionTree baseExpression, ExpressionTree derefExpression, VisitorState state) {
Symbol baseExpressionSymbol = ASTHelpers.getSymbol(baseExpression);
Expand Down

0 comments on commit 3127f16

Please sign in to comment.