diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java index 422d1295b0..9ab0839d5a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -547,6 +547,8 @@ public TransferResult visitAssignment( } } + handler.onDataflowVisitAssignment(node, state, apContext, values(input), input.getRegularStore(), updates); + return updateRegularStore(value, input, updates); } @@ -741,6 +743,7 @@ public TransferResult 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 diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java index eb347e862b..77af556f66 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -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; } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java index 0a80ca1811..6ef4442e73 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -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. * diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java index 5f22594e95..9817b5b98b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -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); } @@ -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, @@ -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) { @@ -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); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/OptionalEmptinessTests.java b/nullaway/src/test/java/com/uber/nullaway/OptionalEmptinessTests.java index c8bc9a06e1..f37ffb4411 100644 --- a/nullaway/src/test/java/com/uber/nullaway/OptionalEmptinessTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/OptionalEmptinessTests.java @@ -724,4 +724,36 @@ private Optional f(Map map1, Map 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 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 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 o = Optional.ofNullable(nullableObj);", + " // BUG: Diagnostic contains: Invoking get() on possibly empty Optional o", + " o.get().hashCode();", + " }", + "}") + .doTest(); + } }