Skip to content

Commit

Permalink
Allow library models of the form null param -> null return (#407)
Browse files Browse the repository at this point in the history 
This is equivalent to what `@Contract(!null -> !null)` already
allows for first party code.

In particular, we use this to implement the nullability contract
of `Optional.orElse(...)` and resolve #406.
  • Loading branch information
@lazaroclapp
lazaroclapp authored Jul 13, 2020
1 parent 3e2233b commit 4375cf0
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 10 deletions.
13 changes: 13 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/LibraryModels.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ public interface LibraryModels {
*/
ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters();

/**
* Get (method, parameter) pairs that cause the method to return <code>null</code> when passed
* <code>null</code> on that parameter.
*
* <p>This is equivalent to annotating a method with a contract like:
*
* <pre><code>@Contract("!null -&gt; !null") @Nullable</code></pre>
*
* @return map from the names of null-in-implies-null out methods to the indexes of the arguments
* that determine nullness of the return.
*/
ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters();

/**
* Get the set of library methods that may return null.
*
Expand Down
67 changes: 57 additions & 10 deletions nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,16 +100,18 @@ public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositio
@Override
public boolean onOverrideMayBeNullExpr(
NullAway analysis, ExpressionTree expr, VisitorState state, boolean exprMayBeNull) {
if (expr.getKind() == Tree.Kind.METHOD_INVOCATION
&& getOptLibraryModels(state.context)
.hasNullableReturn(
(Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) {
return analysis.nullnessFromDataflow(state, expr) || exprMayBeNull;
}
if (expr.getKind() == Tree.Kind.METHOD_INVOCATION
&& getOptLibraryModels(state.context)
.hasNonNullReturn((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) {
return false;
if (expr.getKind() == Tree.Kind.METHOD_INVOCATION) {
OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context);
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) ASTHelpers.getSymbol(expr);
if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes())
|| !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;
} else if (optLibraryModels.hasNonNullReturn(methodSymbol, state.getTypes())) {
// This means the method can't be null, so we return false outright.
return false;
}
}
return exprMayBeNull;
}
Expand All @@ -127,6 +129,21 @@ public NullnessHint onDataflowVisitMethodInvocation(
Preconditions.checkNotNull(callee);
setUnconditionalArgumentNullness(bothUpdates, node.getArguments(), callee, context);
setConditionalArgumentNullness(thenUpdates, elseUpdates, node.getArguments(), callee, context);
ImmutableSet<Integer> nullImpliesNullIndexes =
getOptLibraryModels(context).nullImpliesNullParameters(callee);
if (!nullImpliesNullIndexes.isEmpty()) {
// If the method is marked as having argument dependent nullability and any of the
// corresponding arguments is null, then the return is nullable. If the method is
// marked as having argument dependent nullability but NONE of the corresponding
// arguments is null, then the return should be non-null.
boolean anyNull = false;
for (int idx : nullImpliesNullIndexes) {
if (!inputs.valueOfSubNode(node.getArgument(idx)).equals(NONNULL)) {
anyNull = true;
}
}
return anyNull ? NullnessHint.HINT_NULLABLE : NullnessHint.FORCE_NONNULL;
}
if (getOptLibraryModels(context).hasNonNullReturn(callee, types)) {
return NullnessHint.FORCE_NONNULL;
} else if (getOptLibraryModels(context).hasNullableReturn(callee, types)) {
Expand Down Expand Up @@ -459,6 +476,11 @@ private static class DefaultLibraryModels implements LibraryModels {
.put(methodRef("java.util.Objects", "nonNull(java.lang.Object)"), 0)
.build();

private static final ImmutableSetMultimap<MethodRef, Integer> NULL_IMPLIES_NULL_PARAMETERS =
new ImmutableSetMultimap.Builder<MethodRef, Integer>()
.put(methodRef("java.util.Optional", "orElse(T)"), 0)
.build();

private static final ImmutableSet<MethodRef> NULLABLE_RETURNS =
new ImmutableSet.Builder<MethodRef>()
.add(methodRef("java.lang.ref.Reference", "get()"))
Expand Down Expand Up @@ -553,6 +575,11 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters() {
return NULL_IMPLIES_FALSE_PARAMETERS;
}

@Override
public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {
return NULL_IMPLIES_NULL_PARAMETERS;
}

@Override
public ImmutableSet<MethodRef> nullableReturns() {
return NULLABLE_RETURNS;
Expand All @@ -576,6 +603,8 @@ private static class CombinedLibraryModels implements LibraryModels {

private final ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters;

private final ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters;

private final ImmutableSet<MethodRef> nullableReturns;

private final ImmutableSet<MethodRef> nonNullReturns;
Expand All @@ -591,6 +620,8 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
new ImmutableSetMultimap.Builder<>();
ImmutableSetMultimap.Builder<MethodRef, Integer> nullImpliesFalseParametersBuilder =
new ImmutableSetMultimap.Builder<>();
ImmutableSetMultimap.Builder<MethodRef, Integer> nullImpliesNullParametersBuilder =
new ImmutableSetMultimap.Builder<>();
ImmutableSet.Builder<MethodRef> nullableReturnsBuilder = new ImmutableSet.Builder<>();
ImmutableSet.Builder<MethodRef> nonNullReturnsBuilder = new ImmutableSet.Builder<>();
for (LibraryModels libraryModels : models) {
Expand All @@ -612,6 +643,10 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
libraryModels.nullImpliesFalseParameters().entries()) {
nullImpliesFalseParametersBuilder.put(entry);
}
for (Map.Entry<MethodRef, Integer> entry :
libraryModels.nullImpliesNullParameters().entries()) {
nullImpliesNullParametersBuilder.put(entry);
}
for (MethodRef name : libraryModels.nullableReturns()) {
nullableReturnsBuilder.add(name);
}
Expand All @@ -624,6 +659,7 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
nonNullParameters = nonNullParametersBuilder.build();
nullImpliesTrueParameters = nullImpliesTrueParametersBuilder.build();
nullImpliesFalseParameters = nullImpliesFalseParametersBuilder.build();
nullImpliesNullParameters = nullImpliesNullParametersBuilder.build();
nullableReturns = nullableReturnsBuilder.build();
nonNullReturns = nonNullReturnsBuilder.build();
}
Expand Down Expand Up @@ -653,6 +689,11 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters() {
return nullImpliesFalseParameters;
}

@Override
public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {
return nullImpliesNullParameters;
}

@Override
public ImmutableSet<MethodRef> nullableReturns() {
return nullableReturns;
Expand Down Expand Up @@ -705,6 +746,7 @@ public boolean nameNotPresent(Symbol.MethodSymbol symbol) {
private final NameIndexedMap<ImmutableSet<Integer>> nonNullParams;
private final NameIndexedMap<ImmutableSet<Integer>> nullImpliesTrueParams;
private final NameIndexedMap<ImmutableSet<Integer>> nullImpliesFalseParams;
private final NameIndexedMap<ImmutableSet<Integer>> nullImpliesNullParams;
private final NameIndexedMap<Boolean> nullableRet;
private final NameIndexedMap<Boolean> nonNullRet;

Expand All @@ -717,6 +759,7 @@ public OptimizedLibraryModels(LibraryModels models, Context context) {
nullImpliesTrueParams = makeOptimizedIntSetLookup(names, models.nullImpliesTrueParameters());
nullImpliesFalseParams =
makeOptimizedIntSetLookup(names, models.nullImpliesFalseParameters());
nullImpliesNullParams = makeOptimizedIntSetLookup(names, models.nullImpliesNullParameters());
nullableRet = makeOptimizedBoolLookup(names, models.nullableReturns());
nonNullRet = makeOptimizedBoolLookup(names, models.nonNullReturns());
}
Expand Down Expand Up @@ -749,6 +792,10 @@ ImmutableSet<Integer> nullImpliesFalseParameters(Symbol.MethodSymbol symbol) {
return lookupImmutableSet(symbol, nullImpliesFalseParams);
}

ImmutableSet<Integer> nullImpliesNullParameters(Symbol.MethodSymbol symbol) {
return lookupImmutableSet(symbol, nullImpliesNullParams);
}

private ImmutableSet<Integer> lookupImmutableSet(
Symbol.MethodSymbol symbol, NameIndexedMap<ImmutableSet<Integer>> lookup) {
ImmutableSet<Integer> result = lookup.get(symbol);
Expand Down
39 changes: 39 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,10 @@ public void impliesNonNullContractAnnotation() {
" String test2(Object o2) {",
" return NullnessChecker.noOp(o2).toString();",
" }",
" Object test3(@Nullable Object o1) {",
" // BUG: Diagnostic contains: returning @Nullable expression",
" return NullnessChecker.noOp(o1);",
" }",
"}")
.doTest();
}
Expand Down Expand Up @@ -2524,4 +2528,39 @@ public void checkForNullSupport() {
"}")
.doTest();
}

@Test
public void orElseLibraryModelSupport() {
// Checks both Optional.orElse(...) support itself and the general nullImpliesNullParameters
// Library Models mechanism for encoding @Contract(!null -> !null) as a library model.
compilationHelper
.addSourceLines(
"TestOptionalOrElseNegative.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"import java.util.Optional;",
"class TestOptionalOrElseNegative {",
" public Object foo(Optional<Object> o) {",
" return o.orElse(\"Something\");",
" }",
" public @Nullable Object bar(Optional<Object> o) {",
" return o.orElse(null);",
" }",
"}")
.addSourceLines(
"TestOptionalOrElsePositive.java",
"package com.uber;",
"import java.util.Optional;",
"class TestOptionalOrElsePositive {",
" public Object foo(Optional<Object> o) {",
" // BUG: Diagnostic contains: returning @Nullable expression",
" return o.orElse(null);",
" }",
" public void bar(Optional<Object> o) {",
" // BUG: Diagnostic contains: dereferenced expression o.orElse(null) is @Nullable",
" System.out.println(o.orElse(null).toString());",
" }",
"}")
.doTest();
}
}
5 changes: 5 additions & 0 deletions sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters() {
return ImmutableSetMultimap.of();
}

@Override
public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {
return ImmutableSetMultimap.of();
}

@Override
public ImmutableSet<MethodRef> nullableReturns() {
return ImmutableSet.of();
Expand Down
5 changes: 5 additions & 0 deletions test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public ImmutableSetMultimap<MethodRef, Integer> nullImpliesFalseParameters() {
return ImmutableSetMultimap.of();
}

@Override
public ImmutableSetMultimap<MethodRef, Integer> nullImpliesNullParameters() {
return ImmutableSetMultimap.of();
}

@Override
public ImmutableSet<MethodRef> nullableReturns() {
return ImmutableSet.of();
Expand Down

0 comments on commit 4375cf0

Please sign in to comment.