Skip to content

Fix library models check for expression-body lambdas (Fixes #910) #1603

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

Closed
dyrpsf wants to merge 1 commit into from
Closed

Fix library models check for expression-body lambdas (Fixes #910) #1603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,8 @@ public TransferResult<Nullness, NullnessStore> visitAssignment(
}
}

handler.onDataflowVisitAssignment(node, state, apContext, values(input), input.getRegularStore(), updates);

return updateRegularStore(value, input, updates);
}

Expand Down Expand Up @@ -741,6 +743,7 @@ public TransferResult<Nullness, NullnessStore> visitVariableDeclaration(
if (isCatchVariable(node)) {
updates.set(node, NONNULL);
}

/*
* We can return whatever we want here because a variable declaration is not an expression and
* thus no one can use its value directly. Any updates to the nullness of the variable are
Expand Down
14 changes: 8 additions & 6 deletions nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,13 +377,15 @@ public boolean shouldSkipFieldInitializationCheck(
}

@Override
public boolean isSingleArgNullImpliesFalseMethod(
Symbol.MethodSymbol methodSymbol, VisitorState state) {
public void onDataflowVisitAssignment(
org.checkerframework.nullaway.dataflow.cfg.node.AssignmentNode node,
com.google.errorprone.VisitorState state,
com.uber.nullaway.dataflow.AccessPath.AccessPathContext apContext,
com.uber.nullaway.dataflow.AccessPathNullnessPropagation.SubNodeValues inputs,
com.uber.nullaway.dataflow.NullnessStore store,
com.uber.nullaway.dataflow.AccessPathNullnessPropagation.Updates updates) {
for (Handler h : handlers) {
if (h.isSingleArgNullImpliesFalseMethod(methodSymbol, state)) {
return true;
}
h.onDataflowVisitAssignment(node, state, apContext, inputs, store, updates);
}
return false;
}
}
19 changes: 19 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,25 @@ default NullnessHint onDataflowVisitMethodInvocation(
return NullnessHint.UNKNOWN;
}

/**
* Called when NullAway visits an assignment node during dataflow analysis.
*
* @param node The assignment node being visited.
* @param state The current visitor state.
* @param apContext The current access path context.
* @param store The current nullness store.
* @param updates The updates to be applied to the store.
*/
default void onDataflowVisitAssignment(
org.checkerframework.nullaway.dataflow.cfg.node.AssignmentNode node,
com.google.errorprone.VisitorState state,
com.uber.nullaway.dataflow.AccessPath.AccessPathContext apContext,
com.uber.nullaway.dataflow.AccessPathNullnessPropagation.SubNodeValues inputs,
com.uber.nullaway.dataflow.NullnessStore store,
com.uber.nullaway.dataflow.AccessPathNullnessPropagation.Updates updates) {
// NoOp
}

/**
* Called when the Dataflow analysis visits each field access.
*
Expand Down
125 changes: 108 additions & 17 deletions nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,19 @@ public NullnessHint onDataflowVisitMethodInvocation(
} else if (optionalIsEmptyCall(symbol, types)) {
updateNonNullAPsForOptionalContent(
state.context, elseUpdates, node.getTarget().getReceiver(), apContext);
} else if (optionalOfCall(symbol, types)) {
// Optional.of(...) always yields a non-empty Optional
updateNonNullAPsForOptionalContent(
state.context, bothUpdates, node, apContext);
} else if (optionalOfNullableCall(symbol, types)) {
// Optional.ofNullable(...) yields a non-empty Optional ONLY if the argument is non-null
if (node.getArguments().size() == 1) {
Node arg = node.getArgument(0);
if (inputs.valueOfSubNode(arg) == Nullness.NONNULL) {
updateNonNullAPsForOptionalContent(
state.context, bothUpdates, node, apContext);
}
}
} else if (config.handleTestAssertionLibraries()) {
handleTestAssertions(state, apContext, bothUpdates, node, symbol);
}
Expand All @@ -151,6 +164,85 @@ && isOptionalContentNullable(state, baseExpr, analysis.getNullnessAnalysis(state
return Optional.empty();
}

@Override
public void onDataflowVisitAssignment(
org.checkerframework.nullaway.dataflow.cfg.node.AssignmentNode node,
com.google.errorprone.VisitorState state,
com.uber.nullaway.dataflow.AccessPath.AccessPathContext apContext,
com.uber.nullaway.dataflow.AccessPathNullnessPropagation.SubNodeValues inputs,
com.uber.nullaway.dataflow.NullnessStore store,
com.uber.nullaway.dataflow.AccessPathNullnessPropagation.Updates updates) {

org.checkerframework.nullaway.dataflow.cfg.node.Node rhs = node.getExpression();
org.checkerframework.nullaway.dataflow.cfg.node.Node lhs = node.getTarget();

// Peel back any type cast, null check, widening or narrowing conversion nodes using shaded paths
while (rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.TypeCastNode
|| rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.NullChkNode
|| rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.WideningConversionNode
|| rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.NarrowingConversionNode) {
if (rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.TypeCastNode) {
rhs = ((org.checkerframework.nullaway.dataflow.cfg.node.TypeCastNode) rhs).getOperand();
} else if (rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.NullChkNode) {
rhs = ((org.checkerframework.nullaway.dataflow.cfg.node.NullChkNode) rhs).getOperand();
} else if (rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.WideningConversionNode) {
rhs = ((org.checkerframework.nullaway.dataflow.cfg.node.WideningConversionNode) rhs).getOperand();
} else if (rhs instanceof org.checkerframework.nullaway.dataflow.cfg.node.NarrowingConversionNode) {
rhs = ((org.checkerframework.nullaway.dataflow.cfg.node.NarrowingConversionNode) rhs).getOperand();
}
}

// Case 1: The RHS is a factory method (e.g., Optional.of or Optional.ofNullable)
if (rhs instanceof MethodInvocationNode methodNode) {
Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(methodNode.getTree());
Types types = state.getTypes();
if (symbol != null) {
if (optionalOfCall(symbol, types)) {
updateNonNullAPsForOptionalContent(state.context, updates, lhs, apContext);
return;
} else if (optionalOfNullableCall(symbol, types)) {
if (methodNode.getArguments().size() == 1) {
org.checkerframework.nullaway.dataflow.cfg.node.Node arg = methodNode.getArgument(0);
Nullness argNullness = inputs.valueOfSubNode(arg);
if (argNullness != Nullness.NONNULL) {
AccessPath argAp = AccessPath.getAccessPathForNode(arg, state, apContext);
if (argAp != null) {
argNullness = store.getNullnessOfAccessPath(argAp);
}
}
if (argNullness == Nullness.NONNULL) {
updateNonNullAPsForOptionalContent(state.context, updates, lhs, apContext);
}
}
return;
}
}
}

// Case 2: Copying existing tracking facts across regular variables (e.g., o2 = o1)
AccessPath rhsAp = AccessPath.fromBaseAndElement(
rhs, OptionalContentVariableElement.instance(state.context), apContext);
if (rhsAp != null) {
Nullness rhsNullness = store.getNullnessOfAccessPath(rhsAp);
if (rhsNullness == Nullness.NONNULL) {
updateNonNullAPsForOptionalContent(state.context, updates, lhs, apContext);
}
}
}

private void updateNonNullAPsForOptionalContent(
Context context,
AccessPathNullnessPropagation.Updates updates,
Node base,
AccessPath.AccessPathContext apContext) {
AccessPath ap =
AccessPath.fromBaseAndElement(
base, OptionalContentVariableElement.instance(context), apContext);
if (ap != null) {
updates.set(ap, Nullness.NONNULL);
}
}

private boolean isOptionalContentNullable(
VisitorState state,
ExpressionTree baseExpr,
Expand Down Expand Up @@ -259,32 +351,31 @@ private MethodInvocationNode maybeUnwrapBooleanValueOf(MethodInvocationNode node
return node;
}

private void updateNonNullAPsForOptionalContent(
Context context,
AccessPathNullnessPropagation.Updates updates,
Node base,
AccessPath.AccessPathContext apContext) {
AccessPath ap =
AccessPath.fromBaseAndElement(
base, OptionalContentVariableElement.instance(context), apContext);
if (ap != null && base.getTree() != null) {
updates.set(ap, Nullness.NONNULL);
}
public static javax.lang.model.element.VariableElement getOptionalContentElement(com.sun.tools.javac.util.Context context) {
return OptionalContentVariableElement.instance(context);
}

private boolean optionalIsPresentCall(Symbol.MethodSymbol symbol, Types types) {
return isZeroArgOptionalMethod("isPresent", symbol, types);
return isOptionalMethod("isPresent", 0, symbol, types);
}

private boolean optionalIsEmptyCall(Symbol.MethodSymbol symbol, Types types) {
return isZeroArgOptionalMethod("isEmpty", symbol, types);
return isOptionalMethod("isEmpty", 0, symbol, types);
}

private boolean optionalOfCall(Symbol.MethodSymbol symbol, Types types) {
return isOptionalMethod("of", 1, symbol, types);
}

private boolean optionalOfNullableCall(Symbol.MethodSymbol symbol, Types types) {
return isOptionalMethod("ofNullable", 1, symbol, types);
}

private boolean isZeroArgOptionalMethod(
String methodName, Symbol.MethodSymbol symbol, Types types) {
private boolean isOptionalMethod(
String methodName, int paramCount, Symbol.MethodSymbol symbol, Types types) {
Preconditions.checkNotNull(optionalTypes);
if (!(symbol.getSimpleName().toString().equals(methodName)
&& symbol.getParameters().length() == 0)) {
&& symbol.getParameters().length() == paramCount)) {
return false;
}
for (Type optionalType : optionalTypes) {
Expand All @@ -296,7 +387,7 @@ private boolean isZeroArgOptionalMethod(
}

private boolean optionalIsGetCall(Symbol.MethodSymbol symbol, Types types) {
return isZeroArgOptionalMethod("get", symbol, types);
return isOptionalMethod("get", 0, symbol, types);
}

/**
Expand Down
32 changes: 32 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/OptionalEmptinessTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,4 +724,36 @@ private Optional<String> f(Map<String, String> map1, Map<String, String> map2) {
""")
.doTest();
}

@Test
public void testOptionalOfAndOfNullableInitialValues() {
makeTestHelperWithArgs(
java.util.List.of(
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:CheckOptionalEmptiness=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import java.util.Optional;",
"import javax.annotation.Nullable;", // <-- ADDED THIS IMPORT
"public class Test {",
" public void logOptionalOf() {",
" Optional<Object> o = Optional.of(new Object());",
" // Should be a true negative (no error) because of Optional.of()",
" o.get().hashCode();",
" }",
" public void logOptionalOfNullableWithNonNull() {",
" Object nonNullObj = new Object();",
" Optional<Object> o = Optional.ofNullable(nonNullObj);",
" // Should be a true negative because the argument is explicitly non-null",
" o.get().hashCode();",
" }",
" public void logOptionalOfNullableWithNullable(@Nullable Object nullableObj) {", // <-- ADDED @Nullable
" Optional<Object> o = Optional.ofNullable(nullableObj);",
" // BUG: Diagnostic contains: Invoking get() on possibly empty Optional o",
" o.get().hashCode();",
" }",
"}")
.doTest();
}
}