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 LibraryModelsHandler.onOverrideMayBeNullExpr #754

Merged
merged 2 commits into from
Apr 5, 2023
Merged

refactor LibraryModelsHandler.onOverrideMayBeNullExpr #754

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e59cef7
refactor LibraryModelsHandler.onOverrideMayBeNullExpr
XN137 Mar 31, 2023
faf71d4
review: single return with comment
XN137 Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,29 +128,31 @@ public boolean onOverrideMayBeNullExpr(
@Nullable Symbol exprSymbol,
VisitorState state,
boolean exprMayBeNull) {
if (expr.getKind().equals(Tree.Kind.METHOD_INVOCATION)
&& exprSymbol instanceof Symbol.MethodSymbol) {
OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) exprSymbol;
// When looking up library models of annotated code, we match the exact method signature only;
// overriding methods in subclasses must be explicitly given their own library model.
// When dealing with unannotated code, we default to generality: a model applies to a method
// and any of its overriding implementations.
// see https://github.com/uber/NullAway/issues/445 for why this is needed.
boolean isMethodAnnotated =
!getCodeAnnotationInfo(state.context).isSymbolUnannotated(methodSymbol, this.config);
if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isMethodAnnotated)
|| !optLibraryModels.nullImpliesNullParameters(methodSymbol).isEmpty()) {
// These mean the method might be null, depending on dataflow and arguments. We force
// dataflow to run.
return analysis.nullnessFromDataflow(state, expr) || exprMayBeNull;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see the odd nullnessFromDataflow being run, even without consideration the current value of exprMayBeNull.

the comment above suggests this is intentional and important, however no test is failing without this dataflow call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, from the point of view of code calling this Handler extension point in NullAway.java, with this as the only relevant handler, the change is:

Before:

  1. We ran dataflow here, and return true only if dataflow said true or if exprMayBeNull == true
  2. If we get false out of the full handlers pipeline, we are done, the expression is non-null
  3. Otherwise we call dataflow again.

After:

  1. We return true from the handler unconditionally in this case (meaning if we have either of the above library models)
  2. We call dataflow after getting true from the full handlers pipeline.

Either way, the expression is only consider @Nullable if both the if condition logic in this handler matches and dataflow returns @Nullable and non-null otherwise. So they seem equivalent from the point of view of NullAway.java.

There is a small subtlety here, though, which is that other handlers in the chain will observe the result of LibraryModelsHandler before any call to dataflow on NullAway.java. They will observe different values before/after this change.

I don't think we are relying on dataflow being run here for the correctness of other handlers implementing onOverrideMayBeNullExpr, so this is probably fine? But it's worth pointing that small change in semantics.

One thing I do see possibly happening is that after this change, this handler will set exprMayBeNull == true a lot more often for handlers further down the chain, which could cause some handlers that are checking for reasons to transition nullable -> non-null to do extra work. At the same time, the double call to dataflow is not particularly expensive, because we cache the results of the dataflow analysis.

That said, taking a look at the handlers we currently have, the above concern is theoretical for now. All other handlers implementing this method (RestrictiveAnnotationHandler, InferredJARModelsHandler, and OptionalEmptinessHandler) do strictly less work when exprMayBeNull == true.

Long digression, but basically convinced that this change is a good idea. Worth internal/performance testing, but I'd expect it to be mostly the same, just clearer code! (for which: thank you!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this analysis is correct. Since the new code structure is significantly clearer and easier to understand, I think we would probably want benchmarks showing some measurable performance difference before (re-)introducing an early call to dataflow in a handler. As it stands, I don't expect this change to have a measurable impact

} else if (optLibraryModels.hasNonNullReturn(
methodSymbol, state.getTypes(), !isMethodAnnotated)) {
// This means the method can't be null, so we return false outright.
return false;
}
if (!(expr.getKind() == Tree.Kind.METHOD_INVOCATION
&& exprSymbol instanceof Symbol.MethodSymbol)) {
return exprMayBeNull;
}
OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) exprSymbol;
// When looking up library models of annotated code, we match the exact method signature only;
// overriding methods in subclasses must be explicitly given their own library model.
// When dealing with unannotated code, we default to generality: a model applies to a method
// and any of its overriding implementations.
// see https://github.com/uber/NullAway/issues/445 for why this is needed.
boolean isMethodUnannotated =
getCodeAnnotationInfo(state.context).isSymbolUnannotated(methodSymbol, this.config);
if (exprMayBeNull) {
// This is the only case in which we may switch the result from @Nullable to @NonNull:
return !optLibraryModels.hasNonNullReturn(
methodSymbol, state.getTypes(), isMethodUnannotated);
}
if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), isMethodUnannotated)) {
return true;
}
if (!optLibraryModels.nullImpliesNullParameters(methodSymbol).isEmpty()) {
return true;
}
return exprMayBeNull;
return false;
}

@Override
Expand Down