Run dataflow analysis on bodies of lambdas passed to generic methods#1375
Run dataflow analysis on bodies of lambdas passed to generic methods#1375
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1375 +/- ##
============================================
- Coverage 88.47% 88.35% -0.12%
- Complexity 2626 2642 +16
============================================
Files 97 97
Lines 8822 8888 +66
Branches 1762 1775 +13
============================================
+ Hits 7805 7853 +48
- Misses 509 518 +9
- Partials 508 517 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a public utility NullabilityUtil.stripParensAndCasts(ExpressionTree) and replaces local stripping logic in NullAway. Adds APIs to query the running dataflow state: DataFlow.resultBeforeWithAnalysisRunning(...), RunOnceForwardAnalysisImpl.getStoreBefore(Tree), and AccessPathNullnessAnalysis.getNullnessInfoBeforeNestedMethodWithAnalysisRunning(...), plus a helper to filter access paths for nested methods. Updates GenericsChecks to prepare environment mappings for lambdas and to use dataflow-derived nullness for refining expression types. Adds and reorganizes JSPECIFY generic-method lambda inference tests. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2815-2841: Public API: define null/compat contract + document javac-onlyNullchkstripping.Now that
stripParensAndCastsispublic, please harden/specify its contract:
- Add an explicit null contract (either
Objects.requireNonNull(expr, "...")or annotate/handle nullable input).- Update Javadoc to mention it may strip javac-internal
Nullchk(JCTree.JCUnarywithTree.Kind.OTHER), i.e., behavior depends on javac ASTs and may not apply to non-javacExpressionTreeimplementations.@@ - public static ExpressionTree stripParensAndCasts(ExpressionTree expr) { + public static ExpressionTree stripParensAndCasts(ExpressionTree expr) { + Objects.requireNonNull(expr, "expr"); boolean someChange = true; while (someChange) { someChange = false; @@ return expr; }nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java (1)
227-282: Clarify (or adjust) whetherhandlerPredicateshould also apply to “final-field-chain” access paths.
Right now, whenallAPNonRootElementsAreFinalFieldsis true, the decision ignoreshandlerPredicateentirely and only checks the root kind. If this is intentional (“always allow safe final-field captures”), a short comment would prevent future confusion; otherwise, consider combining with the handler predicate.If you want the handler to still be able to veto those paths:
- if (allAPNonRootElementsAreFinalFields) { + if (allAPNonRootElementsAreFinalFields) { Element e = ap.getRoot(); - return e == null + boolean rootOk = e == null || e.getKind().equals(ElementKind.PARAMETER) || e.getKind().equals(ElementKind.LOCAL_VARIABLE) || (e.getKind().equals(ElementKind.FIELD) && (e.getModifiers().contains(Modifier.FINAL) || hasMonotonicNonNullAnnotation(e))); + return rootOk && handlerPredicate.test(ap); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java(2 hunks)nullaway/src/main/java/com/uber/nullaway/dataflow/RunOnceForwardAnalysisImpl.java(2 hunks)nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(6 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.javanullaway/src/main/java/com/uber/nullaway/dataflow/RunOnceForwardAnalysisImpl.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/NullAway.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/dataflow/RunOnceForwardAnalysisImpl.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
🧬 Code graph analysis (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java (2)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
NullAwayTestsBase(9-51)nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java (1)
JSpecifyJavacConfig(16-59)
nullaway/src/main/java/com/uber/nullaway/dataflow/RunOnceForwardAnalysisImpl.java (1)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(67-700)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: benchmarking
🔇 Additional comments (11)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (1)
918-923: Improved diagnostic message reflects better dataflow analysis.The diagnostic message change from "passing @nullable parameter" to "dereferenced expression t is @nullable" is more precise and accurate. It correctly identifies that the issue is dereferencing the nullable result
t, rather than just flagging the argument passed toid(). This improvement aligns with the PR's objective to run dataflow analysis on lambda bodies passed to generic methods.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java (6)
1-10: Well-organized test suite for lambda argument scenarios.The new test file appropriately separates lambda-specific test cases from the general generic method tests. The class structure follows established patterns and includes all necessary imports. This improves test maintainability and organization.
63-88: LGTM! Comprehensive test for lambda parameter nullability.The test correctly verifies that lambda parameters respect generic type bounds and that dereferencing a
@Nullableparameter produces the expected diagnostic. The test cases cover both the error case (dereferencing nullable) and the safe cases (null check and non-null type parameter).
117-136: Key test case for issue #1373 fix.This test directly addresses the scenario described in issue #1373, where a lambda with a conditional expression
s -> s == null ? null : sis passed to a generic method. The test correctly expects thattis inferred as@Nullableand produces a diagnostic when dereferenced, validating that the dataflow analysis now runs on lambda bodies passed to generic methods.
138-157: LGTM! Validates dataflow tracking through lambda local variables.The test correctly verifies that dataflow analysis tracks nullability through local variable assignments within lambda bodies. This complements the conditional expression tests and ensures comprehensive coverage of lambda body analysis.
159-192: Excellent stress test with realistic nested lambda scenario.The test based on Spring's Flux code provides valuable coverage of complex nested lambda scenarios with multiple levels of generic type inference. This ensures the dataflow analysis handles real-world production patterns correctly.
194-200: LGTM! Helper method correctly configured for inference testing.The helper method properly configures the test environment with JSpecify mode and enables the
WarnOnGenericInferenceFailureflag, which is essential for validating the enhanced inference behavior introduced in this PR.nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java (1)
203-225: Good factoring + consistent API for “analysis running” store retrieval.
The newgetNullnessInfoBeforeNestedMethodWithAnalysisRunningmirrors the existing API cleanly, and the shared helper reduces duplication.nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (3)
857-874: LambdaPath threading for lambda-body constraint generation looks correct.
BuildingTreePath lambdaPath = new TreePath(path, lambda)and passing it intogenerateConstraintsForPseudoAssignmentis the right mechanism to enable lambda-body-aware dataflow/inference.
1035-1063:shouldRunDataflowForExpressiongate is a solid perf/robustness win; keep an eye on coverage.
Skipping primitives and null types, and restricting to “interesting” expression kinds, should reduce unnecessary analysis churn in inference. TheSWITCH_EXPRESSIONstring fallback is a pragmatic cross-JDK approach.
959-999: The concern about infinite recursion inupdateEnvironmentMappingForLambdais not valid.
NullabilityUtil.findEnclosingMethodOrLambdaOrInitializer(path)starts its search frompath.getParentPath()(line 3 of the implementation), not frompathitself. This means when called with a LambdaExpressionTree path, the method will search upward from the lambda's parent, never returning the same lambda. Consequently:
- In
refineArgumentTypeWithDataflow(line 977):methodEnclosingLambdawill be strictly enclosing relative toenclosingPath- In
updateEnvironmentMappingForLambda(line 1016):nextEnclosingwill be strictly enclosing relative toenclosingForLambdaRecursion progresses correctly in both cases. The proposed fix is unnecessary.
Likely an incorrect or invalid review comment.
| /** | ||
| * Like {@link #resultBefore(TreePath, Context, ForwardTransferFunction)} but for the case where | ||
| * the dataflow analysis is currently running | ||
| */ | ||
| public <A extends AbstractValue<A>, S extends Store<S>, T extends ForwardTransferFunction<A, S>> | ||
| @Nullable S resultBeforeWithAnalysisRunning(TreePath exprPath, Context context, T transfer) { | ||
| TreePath enclosingPath = findEnclosingMethodOrLambdaOrInitializer(exprPath); | ||
| if (enclosingPath == null) { | ||
| throw new RuntimeException("expression is not inside a method, lambda or initializer block!"); | ||
| } | ||
|
|
||
| Tree method = enclosingPath.getLeaf(); | ||
| if (!(method instanceof MethodTree) || ((MethodTree) method).getBody() != null) { | ||
| RunOnceForwardAnalysisImpl<A, S, T> analysis = | ||
| (RunOnceForwardAnalysisImpl<A, S, T>) | ||
| dataflow(enclosingPath, context, transfer, false).getAnalysis(); | ||
| Verify.verify(analysis.isRunning(), "Expected analysis to be running for %s", method); | ||
| return analysis.getStoreBefore(exprPath.getLeaf()); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Make the guard logic + exception type consistent with the rest of DataFlow API.
The behavior seems right, but the conditional if (!(method instanceof MethodTree) || ((MethodTree) method).getBody() != null) is hard to parse and the RuntimeException is inconsistent with nearby APIs that use IllegalArgumentException for bad inputs.
Suggested refactor:
public <A extends AbstractValue<A>, S extends Store<S>, T extends ForwardTransferFunction<A, S>>
@Nullable S resultBeforeWithAnalysisRunning(TreePath exprPath, Context context, T transfer) {
TreePath enclosingPath = findEnclosingMethodOrLambdaOrInitializer(exprPath);
if (enclosingPath == null) {
- throw new RuntimeException("expression is not inside a method, lambda or initializer block!");
+ throw new IllegalArgumentException(
+ "expression is not inside a method, lambda or initializer block!");
}
- Tree method = enclosingPath.getLeaf();
- if (!(method instanceof MethodTree) || ((MethodTree) method).getBody() != null) {
+ Tree enclosing = enclosingPath.getLeaf();
+ if (enclosing instanceof MethodTree && ((MethodTree) enclosing).getBody() == null) {
+ return null;
+ }
+ @SuppressWarnings("unchecked")
RunOnceForwardAnalysisImpl<A, S, T> analysis =
(RunOnceForwardAnalysisImpl<A, S, T>)
dataflow(enclosingPath, context, transfer, false).getAnalysis();
- Verify.verify(analysis.isRunning(), "Expected analysis to be running for %s", method);
+ Verify.verify(analysis.isRunning(), "Expected analysis to be running for %s", enclosing);
return analysis.getStoreBefore(exprPath.getLeaf());
- }
- return null;
}🤖 Prompt for AI Agents
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java around lines 277
to 297: the guard condition and exception are inconsistent with the rest of the
DataFlow API — change the conditional to explicitly require a MethodTree with a
non-null body (i.e., proceed only when method instanceof MethodTree &&
((MethodTree) method).getBody() != null) and otherwise throw an
IllegalArgumentException (with a clear message like "expression is not inside a
method with a body, lambda or initializer block") instead of RuntimeException so
the behavior and exception type match surrounding APIs.
nullaway/src/main/java/com/uber/nullaway/dataflow/RunOnceForwardAnalysisImpl.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java
Outdated
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java
Outdated
Show resolved
Hide resolved
|
Main Branch: With This PR: |
yuxincs
left a comment
There was a problem hiding this comment.
This seems quite involved so if @lazaroclapp can also take a look it would be nice!
| * @return the same expression without parentheses. | ||
| */ | ||
| private static ExpressionTree stripParensAndCasts(ExpressionTree expr) { | ||
| public static ExpressionTree stripParensAndCasts(ExpressionTree expr) { |
There was a problem hiding this comment.
Feels like a generic helper function that should be in a helper class etc?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-08-29T18:41:43.584Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (3)
nullaway/src/main/java/com/uber/nullaway/dataflow/EnclosingEnvironmentNullness.java (1)
EnclosingEnvironmentNullness(18-60)nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java (1)
NullnessStore(43-371)nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(71-732)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on ubuntu-latest
🔇 Additional comments (5)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (5)
50-51: LGTM!The new imports are necessary for the lambda dataflow refinement functionality introduced in this PR.
Also applies to: 67-67
864-880: LGTM!The lambda path construction properly sets up the TreePath with the lambda as the leaf, enabling dataflow analysis to run within the lambda body. The null check for
pathensures we have a valid TreePath to extend.
969-975: Acknowledge the TreePath construction limitation.The comment correctly notes that appending
exprtocurrentPathis "a bit sketchy" since it may not be a valid tree path. However, since the path is only used to discover the enclosing method/lambda/initializer (which should be correctly found), this approach is acceptable.
1018-1041: LGTM!The method correctly sets up environment mappings for lambdas, handling:
- Recursive setup for nested lambdas
- Different code paths depending on whether dataflow analysis is already running
The recursion is bounded by lambda nesting depth, which is unlikely to be problematic in practice.
1051-1071: LGTM with a question about NEW_CLASS.The method correctly gates dataflow execution to expression kinds where refinement is useful. The string comparison for
SWITCH_EXPRESSION(Line 1066) is an appropriate workaround for JDK version compatibility.Minor question: Should
NEW_CLASSbe included in the supported kinds? New object creation always returns non-null, so dataflow might not add value, but it's worth considering for completeness.
| // TODO if we did the dataflow analysis for a lambda, always remove the analysis result from the | ||
| // cache for safety |
There was a problem hiding this comment.
Follow up on the TODO for lambda invalidation in loops.
The TODO mentions that dataflow results for lambdas inside loops should be invalidated to avoid caching incomplete results. Without this invalidation, the analysis could cache dataflow facts from an incomplete loop iteration, leading to incorrect nullness conclusions on subsequent iterations.
This is a correctness concern that should be tracked carefully.
Do you want me to open a tracking issue for this TODO to ensure it's addressed in a follow-up PR?
🤖 Prompt for AI Agents
In nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java around
lines 1001-1002, the TODO notes that dataflow results for lambdas inside loops
must be invalidated to avoid caching incomplete iteration state; implement this
by detecting when a lambda AST node is located within a loop construct
(for/while/do/foreach) and, when performing dataflow analysis for that lambda,
either (a) mark the lambda's dataflow result as non-cacheable or (b) explicitly
remove/clear its entry from the analysis cache immediately after analysis
completes; add unit tests that exercise a lambda within a loop to ensure
subsequent iterations do not reuse stale cached facts and add a brief comment
referencing the tracking issue/PR for future reasoning.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the TODO about lambda invalidation in loops
gh issue create \
--repo uber/NullAway \
--title "Invalidate dataflow cache for lambdas inside loops" \
--assignee msridhar \
--body "## Context
Follow-up from PR #1375 (comment: https://github.com/uber/NullAway/pull/1375#discussion_r2632195569)
In \`nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java\` around lines 1001-1002, there is a TODO noting that dataflow results for lambdas inside loops should be invalidated to avoid caching incomplete results from earlier loop iterations.
## Problem
Without this invalidation, the analysis could cache dataflow facts from an incomplete loop iteration, leading to incorrect nullness conclusions on subsequent iterations. This is a correctness concern.
## Proposed Solution
Implement invalidation by:
1. Detecting when a lambda AST node is located within a loop construct (for/while/do/foreach)
2. When performing dataflow analysis for that lambda, either:
- (a) Mark the lambda's dataflow result as non-cacheable, OR
- (b) Explicitly remove/clear its entry from the analysis cache immediately after analysis completes
3. Add unit tests that exercise a lambda within a loop to ensure subsequent iterations do not reuse stale cached facts
4. Add a brief comment referencing this issue for future reasoning
## Related Code
The TODO is in the \`refineArgumentTypeWithDataflow\` method:
\`\`\`java
// TODO if we did the dataflow analysis for a lambda, always remove the analysis result from the
// cache for safety
\`\`\`"
Length of output: 44
@msridhar I've created issue #1381 to track the TODO for invalidating dataflow cache results for lambdas inside loops. The issue includes the technical context, proposed solution approach, and references back to this PR and discussion.
🧠 Learnings used
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
lazaroclapp
left a comment
There was a problem hiding this comment.
Currently traveling and wasn't really able to trace all the related code changes this PR depends on, but the high level of what this PR should be doing makes sense to me, as do the tests and the inner workings of the newly added methods, so I am approving. All code in this PR itself LGTM, happy to learn more about the architectural changes to dataflow in late January :)
| refinedNullness = nullnessAnalysis.getNullness(exprPath, state.context); | ||
| } | ||
| // TODO if we did the dataflow analysis for a lambda, always remove the analysis result from the | ||
| // cache for safety |
There was a problem hiding this comment.
Do we have a test case we can add that shows the checker failing in some way due to the lack of this invalidation? If easy to do, we should add the suppressed test, maybe with the loop mentioned in the description for the PR. Not sure how hard is to trigger the incorrect behavior here, though...
Note that I am not asking for a fix in this PR, just a code-level unit test documentation of what the issue looks like :)
There was a problem hiding this comment.
After further thought, I plan to remove this TODO, as I think this is not an issue. I was thinking we might have a case like this one, but for lambdas:
So the idea would be that the first time you analyze a lambda like () -> s, the dataflow tells you it returns @NonNull, but then once you process the rest of the loop and continue dataflow analysis of the enclosing method, you realize it could return @Nullable. But I realize now this can't happen, because the only access paths we keep in the "environment" when analyzing a lambda are for locations that are (effectively) final. So, e.g., we wouldn't try to propagate facts about s in the above example to the body of a lambda. Fundamentally the pattern above relies on locations having different nullability at different program points, which won't get propagated to lambda bodies, so I think we're actually fine and don't have to do this invalidation 😀
Thanks for making me think this through a bit more!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java
🧬 Code graph analysis (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java (2)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
NullAwayTestsBase(9-51)nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java (1)
JSpecifyJavacConfig(16-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on windows-latest
🔇 Additional comments (3)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java (3)
1-10: LGTM! Well-organized test class structure.The test class is properly structured with appropriate package placement, necessary imports, and clear naming that reflects its purpose of testing generic method handling with lambda arguments in JSpecify mode.
64-240: Excellent test coverage for diverse lambda scenarios.These tests comprehensively validate generic method inference across multiple scenarios:
- Lambda arguments with callbacks
- Supplier lambdas returning null
- Conditional expressions in lambda bodies
- Nullable local variables and enclosing locals
- Non-final field access (with clear explanation of why facts don't propagate)
- Complex nested lambdas from real-world frameworks
The test assertions correctly verify that NullAway infers
@Nullabletypes where appropriate and raises diagnostics on unsafe dereferences. This directly addresses the objectives of issue #1373.
242-248: LGTM! Proper test helper configuration.The helper method correctly configures JSpecify mode and enables generic inference failure warnings, which is essential for validating the enhanced lambda inference behavior introduced in this PR.
| @Test | ||
| public void lambdaReturnsGenericMethodCall() { | ||
| makeHelperWithInferenceFailureWarning() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import org.jspecify.annotations.*;", | ||
| "@NullMarked", | ||
| "class Test {", | ||
| " static interface Supplier<T extends @Nullable Object> {", | ||
| " T get();", | ||
| " }", | ||
| " static <R extends @Nullable Object> R invokeWithReturn(Supplier<R> supplier) {", | ||
| " return supplier.get();", | ||
| " }", | ||
| " static <U extends @Nullable Object> U genericMethod(U var){", | ||
| " return var;", | ||
| " }", | ||
| " static void test() {", | ||
| " Object x = invokeWithReturn(() -> { return genericMethod(\"value\");});", | ||
| " Object y = invokeWithReturn(() -> { return genericMethod(null);});", | ||
| " // legal, should infer x is a @NonNull String", | ||
| " x.hashCode();", | ||
| " // BUG: Diagnostic contains: dereferenced expression y is @Nullable", | ||
| " y.hashCode();", | ||
| " // Block-bodied with parenthesized return", | ||
| " Object x_block_paren = invokeWithReturn(() -> { return (genericMethod(\"value\"));});", | ||
| " Object y_block_paren = invokeWithReturn(() -> { return (genericMethod(null));});", | ||
| " // legal, should infer x_block_paren is a @NonNull String", | ||
| " x_block_paren.hashCode();", | ||
| " // BUG: Diagnostic contains: dereferenced expression y_block_paren is @Nullable", | ||
| " y_block_paren.hashCode();", | ||
| " // Expression-bodied", | ||
| " Object x_expr = invokeWithReturn(() -> genericMethod(\"value\"));", | ||
| " Object y_expr = invokeWithReturn(() -> genericMethod(null));", | ||
| " // legal, should infer x_expr is a @NonNull String", | ||
| " x_expr.hashCode();", | ||
| " // BUG: Diagnostic contains: dereferenced expression y_expr is @Nullable", | ||
| " y_expr.hashCode();", | ||
| " // Expression-bodied with parenthesized return", | ||
| " Object x_expr_paren = invokeWithReturn(() -> (genericMethod(\"value\")));", | ||
| " Object y_expr_paren = invokeWithReturn(() -> (genericMethod(null)));", | ||
| " // legal, should infer x_expr_paren is a @NonNull String", | ||
| " x_expr_paren.hashCode();", | ||
| " // BUG: Diagnostic contains: dereferenced expression y_expr_paren is @Nullable", | ||
| " y_expr_paren.hashCode();", | ||
| " Object x2 = invokeWithReturn(() ->{ Object y2 = null; return y2;});", | ||
| " // BUG: Diagnostic contains: dereferenced expression x2 is @Nullable", | ||
| " x2.hashCode();", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Comprehensive test coverage for lambda returns from generic methods.
This test thoroughly validates inference for various lambda forms (block-bodied, expression-bodied, with/without parentheses) and correctly asserts that nullable returns are inferred as @Nullable while non-null literals are inferred as @NonNull.
Minor spacing inconsistency in test input
Line 56 has () ->{ (no space before {) while other lambdas in this test use () -> { (with space). For consistency and readability, consider:
- " Object x2 = invokeWithReturn(() ->{ Object y2 = null; return y2;});",
+ " Object x2 = invokeWithReturn(() -> { Object y2 = null; return y2;});",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void lambdaReturnsGenericMethodCall() { | |
| makeHelperWithInferenceFailureWarning() | |
| .addSourceLines( | |
| "Test.java", | |
| "import org.jspecify.annotations.*;", | |
| "@NullMarked", | |
| "class Test {", | |
| " static interface Supplier<T extends @Nullable Object> {", | |
| " T get();", | |
| " }", | |
| " static <R extends @Nullable Object> R invokeWithReturn(Supplier<R> supplier) {", | |
| " return supplier.get();", | |
| " }", | |
| " static <U extends @Nullable Object> U genericMethod(U var){", | |
| " return var;", | |
| " }", | |
| " static void test() {", | |
| " Object x = invokeWithReturn(() -> { return genericMethod(\"value\");});", | |
| " Object y = invokeWithReturn(() -> { return genericMethod(null);});", | |
| " // legal, should infer x is a @NonNull String", | |
| " x.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y is @Nullable", | |
| " y.hashCode();", | |
| " // Block-bodied with parenthesized return", | |
| " Object x_block_paren = invokeWithReturn(() -> { return (genericMethod(\"value\"));});", | |
| " Object y_block_paren = invokeWithReturn(() -> { return (genericMethod(null));});", | |
| " // legal, should infer x_block_paren is a @NonNull String", | |
| " x_block_paren.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y_block_paren is @Nullable", | |
| " y_block_paren.hashCode();", | |
| " // Expression-bodied", | |
| " Object x_expr = invokeWithReturn(() -> genericMethod(\"value\"));", | |
| " Object y_expr = invokeWithReturn(() -> genericMethod(null));", | |
| " // legal, should infer x_expr is a @NonNull String", | |
| " x_expr.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y_expr is @Nullable", | |
| " y_expr.hashCode();", | |
| " // Expression-bodied with parenthesized return", | |
| " Object x_expr_paren = invokeWithReturn(() -> (genericMethod(\"value\")));", | |
| " Object y_expr_paren = invokeWithReturn(() -> (genericMethod(null)));", | |
| " // legal, should infer x_expr_paren is a @NonNull String", | |
| " x_expr_paren.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y_expr_paren is @Nullable", | |
| " y_expr_paren.hashCode();", | |
| " Object x2 = invokeWithReturn(() ->{ Object y2 = null; return y2;});", | |
| " // BUG: Diagnostic contains: dereferenced expression x2 is @Nullable", | |
| " x2.hashCode();", | |
| " }", | |
| "}") | |
| .doTest(); | |
| } | |
| @Test | |
| public void lambdaReturnsGenericMethodCall() { | |
| makeHelperWithInferenceFailureWarning() | |
| .addSourceLines( | |
| "Test.java", | |
| "import org.jspecify.annotations.*;", | |
| "@NullMarked", | |
| "class Test {", | |
| " static interface Supplier<T extends @Nullable Object> {", | |
| " T get();", | |
| " }", | |
| " static <R extends @Nullable Object> R invokeWithReturn(Supplier<R> supplier) {", | |
| " return supplier.get();", | |
| " }", | |
| " static <U extends @Nullable Object> U genericMethod(U var){", | |
| " return var;", | |
| " }", | |
| " static void test() {", | |
| " Object x = invokeWithReturn(() -> { return genericMethod(\"value\");});", | |
| " Object y = invokeWithReturn(() -> { return genericMethod(null);});", | |
| " // legal, should infer x is a @NonNull String", | |
| " x.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y is @Nullable", | |
| " y.hashCode();", | |
| " // Block-bodied with parenthesized return", | |
| " Object x_block_paren = invokeWithReturn(() -> { return (genericMethod(\"value\"));});", | |
| " Object y_block_paren = invokeWithReturn(() -> { return (genericMethod(null));});", | |
| " // legal, should infer x_block_paren is a @NonNull String", | |
| " x_block_paren.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y_block_paren is @Nullable", | |
| " y_block_paren.hashCode();", | |
| " // Expression-bodied", | |
| " Object x_expr = invokeWithReturn(() -> genericMethod(\"value\"));", | |
| " Object y_expr = invokeWithReturn(() -> genericMethod(null));", | |
| " // legal, should infer x_expr is a @NonNull String", | |
| " x_expr.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y_expr is @Nullable", | |
| " y_expr.hashCode();", | |
| " // Expression-bodied with parenthesized return", | |
| " Object x_expr_paren = invokeWithReturn(() -> (genericMethod(\"value\")));", | |
| " Object y_expr_paren = invokeWithReturn(() -> (genericMethod(null)));", | |
| " // legal, should infer x_expr_paren is a @NonNull String", | |
| " x_expr_paren.hashCode();", | |
| " // BUG: Diagnostic contains: dereferenced expression y_expr_paren is @Nullable", | |
| " y_expr_paren.hashCode();", | |
| " Object x2 = invokeWithReturn(() -> { Object y2 = null; return y2;});", | |
| " // BUG: Diagnostic contains: dereferenced expression x2 is @Nullable", | |
| " x2.hashCode();", | |
| " }", | |
| "}") | |
| .doTest(); | |
| } |
🤖 Prompt for AI Agents
In
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaArgTests.java
around lines 11 to 62, there's a minor spacing inconsistency: on line 56 the
lambda is written as "() ->{ Object y2 = null; return y2;});" (missing space
before the '{'); fix it by adding a space so the lambda reads "() -> { Object y2
= null; return y2;});" to match the other lambdas' formatting for consistency
and readability.
Fixes #1373
This is a follow up to #1337. We now use dataflow analysis to refine the returned type from a lambda when generating constraints for generic method inference. This ended up being a rather subtle change due to cyclic dependencies between the dataflow analysis and the generic inference. To run dataflow analysis on a lambda, dataflow analysis must be run on the containing method/lambda/initializer first, to determine the nullability of variables "captured" from the environment. But, that dataflow analysis on the containing member may depend on the generic inference result for the current call.
The key changes are:
GenericsChecks#updateEnvironmentMappingForLambdathat ensures the environment mapping for a lambda is present before running dataflow analysis on the lambda. This method detects if dataflow analysis is already running on the containing member, and if so, creates an environment mapping using the current state of that analysis using new APIs (seeAccessPathNullnessAnalysis#getNullnessInfoBeforeNestedMethodWithAnalysisRunning). The method also handles multiple levels of lambda nesting.TreePaths to contain the relevant lambda.GenericsChecks#shouldRunDataflowForExpressionto properly detect when dataflow analysis is useful for an expression, to avoid running the analysis unnecessarily.Also, we shift tests related to generic methods and lambdas to a new test class
GenericMethodLambdaArgTeststo make them easier to find.Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.