Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: streamline mayBeNullExpr flow #753

Merged
merged 2 commits into from
Apr 4, 2023
Merged

refactor: streamline mayBeNullExpr flow #753

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3127f16
refactor: streamline mayBeNullExpr flow
XN137 Mar 12, 2023
51c8b8a
review: add comment
XN137 Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 27 additions & 41 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
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,35 @@ 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;
// rely on dataflow analysis for local variables
exprMayBeNull = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here saying we rely on inference / dataflow analysis for local variables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, please check if it is as you had mind.

also i tried putting answers to the questions in the PR description, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will take another look later in the day and then likely land this

}
break;
case METHOD_INVOCATION:
if (!(exprSymbol instanceof Symbol.MethodSymbol)) {
throw new IllegalStateException(
Expand All @@ -2289,36 +2290,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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that here and above, we used to first run dataflow and only then called the handler overrides

while now we would be doing it the other way round.
running dataflow last seems like the most uniform way?
also there are no tests failing so maybe the old order did not matter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed, running dataflow last uniformly is what we want.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, under some circumstances, want handlers to override the results from the dataflow analysis, but I think this should in all cases be done by using one of the handler methods that run as part of the dataflow analysis itself, not the AST-node taking methods... (Edit: to clarify, I am agreeing with the uniform ordering here)

} 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

together with the LibraryModelsHandler refactoring, this would be the only place where nullnessFromDataflow would be getting called still

}

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 +2331,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