Invoke generic method inference in more places #1286
Conversation
|
Warning Rate limit exceeded@msridhar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds path-aware, per-invocation generic-method type-argument inference and caching in GenericsChecks, changes getGenericReturnNullnessAtInvocation to accept an optional TreePath and updates callers accordingly, and expands tests to cover dataflow-triggered inference and inference-failure warnings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Checker as Checker/Dataflow
participant GC as GenericsChecks
participant IF as InferenceEngine
participant Cache as InvocationCache
Checker->>GC: getGenericReturnNullnessAtInvocation(method, tree, path?, state)
alt return not a type variable
GC-->>Checker: declared/nullness result
else
GC->>Cache: lookup(invocation + path)
alt cache hit
Cache-->>GC: cached inference
else
GC->>IF: runInferenceForCall(invocation, path?, state)
IF->>IF: select invocation & assignment context (path-aware)
IF->>IF: generate constraints & solve
IF->>Cache: store inference result for related invocations
Cache-->>GC: inference result
end
GC-->>Checker: substituted return nullness (possibly NULLABLE)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1286 +/- ##
============================================
- Coverage 88.43% 88.34% -0.09%
- Complexity 2450 2465 +15
============================================
Files 93 93
Lines 8116 8188 +72
Branches 1600 1614 +14
============================================
+ Hits 7177 7234 +57
- Misses 475 480 +5
- Partials 464 474 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
1270-1276: Handle parenthesized arguments when locating the formal parameter type.Comparison should ignore parentheses; otherwise, inference misses the assignment context for calls like f((id(x))).
Apply:
- (arg, pos, formalParamType, unused) -> { - if (arg == invocation) { + (arg, pos, formalParamType, unused) -> { + if (ASTHelpers.stripParentheses(arg) == invocation) { formalParamTypeRef.set(formalParamType); } });
1244-1255: Inference for returns inside lambdas is unhandled.ReturnTree branch TODO: when the invocation is the returned expr of a lambda, we can derive the assignment context from the functional interface method return type.
I can send a follow-up patch to resolve the return type from NullabilityUtil.getFunctionalInterfaceMethod and use its return type here.
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2714-2720: Null path in getGenericReturnNullnessAtInvocation is acceptable; opportunistically pass a TreePath when available.Verified call sites: nullaway/src/main/java/com/uber/nullaway/NullAway.java:2719 (passes null for the TreePath), nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java:1157; method defined at nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:1103. Not blocking — consider constructing/passing a TreePath (e.g., new TreePath(state.getPath(), invocationTree) when state.getPath() reliably exists for the invocation) to improve inference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(17 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#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
PR: uber/NullAway#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/AccessPathNullnessPropagation.javanullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#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/GenericMethodTests.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(66-665)
⏰ 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 ubuntu-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on windows-latest
🔇 Additional comments (4)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
928-933: Scope control on NewClassTree substitution looks intentional; LGTM.Keeping compareGenericTypeParameterNullabilityForCall limited to MethodInvocationTree matches the stated preference to handle NewClassTree support separately.
If desired later, enabling NewClassTree here should be coordinated with broader support to avoid partial behavior.
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java (1)
1157-1159: Propagating TreePath into generic return nullness check: good integration.Passing node.getTreePath() enables path-aware inference during dataflow, improving precision.
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java (2)
742-774: Enabling inference-failure warnings and adding dataflow-triggered checks: nice coverage.Switching to makeHelperWithInferenceFailureWarning and asserting deref diagnostics exercises the new inference paths.
1144-1195: New inferenceFromDataflow test validates nested/paren/receiver/param contexts.Solid end-to-end coverage of path-aware inference via dataflow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (4)
1156-1181: Path-aware substitution should re-infer on prior failure when path/context existsSame failure-caching pitfall: if result was a failure, don’t skip re-inference when we now have an invocation/context from path.
Apply:
- if (result == null) { - // have not yet attempted inference for this call - MethodInvocationTree invocationTree = (MethodInvocationTree) tree; - InvocationAndContext invocationAndType = - path == null ? null : getInvocationAndContextForInference(path, state); - result = - invocationAndType == null - ? runInferenceForCall(state, invocationTree, null, false) - : runInferenceForCall( - state, - invocationAndType.invocation, - invocationAndType.typeFromAssignmentContext, - invocationAndType.assignedToLocal); - } + MethodInvocationTree invocationTree = (MethodInvocationTree) tree; + InvocationAndContext invocationAndType = + path == null ? null : getInvocationAndContextForInference(path, state); + boolean shouldReinfer = + result == null + || (result instanceof InferenceFailure && invocationAndType != null); + if (shouldReinfer) { + result = + invocationAndType == null + ? runInferenceForCall(state, invocationTree, null, false) + : runInferenceForCall( + state, + invocationAndType.invocation, + invocationAndType.typeFromAssignmentContext, + invocationAndType.assignedToLocal); + }
1282-1287: Avoid throwing when invocation isn’t a direct receiver; fall back gracefullyThrowing here can abort analysis for valid trees. Return null and continue without assignment context.
Apply:
- } else { - throw new RuntimeException( - "did not find invocation " - + state.getSourceForNode(invocation) - + " as receiver expression of " - + state.getSourceForNode(parentInvocation)); - } + } else { + // Not a direct receiver; fall back to inference without assignment context. + return null; + }
541-544: Re-run inference when a cached failure meets new assignment contextCurrently, a cached InferenceFailure blocks re-inference even when typeFromAssignmentContext becomes available. Re-run in that case.
Apply:
- MethodInferenceResult result = inferredTypeVarNullabilityForGenericCalls.get(invocationTree); - if (result == null) { // have not yet attempted inference for this call - result = - runInferenceForCall(state, invocationTree, typeFromAssignmentContext, assignedToLocal); - } + MethodInferenceResult result = inferredTypeVarNullabilityForGenericCalls.get(invocationTree); + if (result == null + || (result instanceof InferenceFailure && typeFromAssignmentContext != null)) { + result = + runInferenceForCall(state, invocationTree, typeFromAssignmentContext, assignedToLocal); + }
606-623: Don’t cache failures; allow re-try with richer contextCaching InferenceFailure for allInvocations freezes a context-less result and prevents later success.
Apply:
- InferenceFailure failureResult = new InferenceFailure(e.getMessage()); - for (MethodInvocationTree invTree : allInvocations) { - inferredTypeVarNullabilityForGenericCalls.put(invTree, failureResult); - } - return failureResult; + // Do not cache failures; allow re-run with better context later. + return new InferenceFailure(e.getMessage());
🧹 Nitpick comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
1139-1142: Minor: guard against null expression.type when converting explicit argsIn rare error states, JCExpression.type can be null. Safe-guard to avoid NPE.
Apply:
- types.add(expression.type); // Retrieve the Type + if (expression.type != null) { + types.add(expression.type); // Retrieve the Type + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(17 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#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
PR: uber/NullAway#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
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(66-665)nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java (1)
ErrorMessage(26-73)
⏰ 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). (5)
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on macos-latest
🔇 Additional comments (5)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (5)
472-481: Good: correctly tracking local-variable assignments for inference contextUsing assignedToLocal and the helper isAssignmentToLocalVariable() makes constraints more precise. LGTM.
Also applies to: 503-515
592-605: Success caching across nested invocations is sensiblePropagating InferenceSuccess to allInvocations avoids duplicate work. LGTM.
665-677: Good: only constrain return when assignment context existsGuarding addSubtypeConstraint with a null check prevents spurious constraints. LGTM.
1570-1572: Good: reduce API exposure of prettyTypeForErrorPrivate static is appropriate here. LGTM.
1104-1112: Confirmed: only TYPEVAR returns can be top-level nullable; callers handle explicit @nullable.Both call sites enforce explicit @nullable handling: NullAway.mayBeNullMethodCall checks Nullness.hasNullableAnnotation before calling, and AccessPathNullnessPropagation.genericReturnIsNullable only invokes the generics check when hasNullableAnnotation is false. No other callers found.
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
Show resolved
Hide resolved
lazaroclapp
left a comment
There was a problem hiding this comment.
Some nits and thoughts, but this LGTM :)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
Show resolved
Hide resolved
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Outdated
Show resolved
Hide resolved
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Outdated
Show resolved
Hide resolved
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
618-623: Avoid caching inference failures across all invocations; allow re-run with richer context.Caching an
InferenceFailurefor everyinvTreeinallInvocationscan permanently block later successful, path-aware inference (e.g., when dataflow provides an assignment/return context). Prefer not caching failures, or at least don’t broadcast them to nested calls.Apply:
- InferenceFailure failureResult = new InferenceFailure(e.getMessage()); - for (MethodInvocationTree invTree : allInvocations) { - inferredTypeVarNullabilityForGenericCalls.put(invTree, failureResult); - } - return failureResult; + // Do not cache failures; allow re-run if/when better context (e.g., TreePath) appears. + return new InferenceFailure(e.getMessage());
🧹 Nitpick comments (7)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (7)
503-515: Treat try-with-resources variables as “local” too.
isAssignmentToLocalVariable(...)missesElementKind.RESOURCE_VARIABLE, which are effectively local variables. Include it to avoid skewingassignedToLocalduring inference when initializing resources with generic calls.Apply:
- return treeSymbol != null && treeSymbol.getKind().equals(ElementKind.LOCAL_VARIABLE); + return treeSymbol != null + && (treeSymbol.getKind() == ElementKind.LOCAL_VARIABLE + || treeSymbol.getKind() == ElementKind.RESOURCE_VARIABLE);Also applies to: 472-481
1120-1124: Use enum identity comparison instead ofObjects.equals.Simpler and avoids unnecessary boxing.
Apply:
- if (substitutedReturnType != null - && Objects.equals(getTypeNullness(substitutedReturnType), Nullness.NULLABLE)) { + if (substitutedReturnType != null + && getTypeNullness(substitutedReturnType) == Nullness.NULLABLE) {
1351-1354: Same enum comparison nit here.Apply:
- if (substitutedParamTypes != null - && Objects.equals( - getParameterTypeNullness(substitutedParamTypes.get(paramIndex), isVarargsParam), - Nullness.NULLABLE)) { + if (substitutedParamTypes != null + && getParameterTypeNullness(substitutedParamTypes.get(paramIndex), isVarargsParam) + == Nullness.NULLABLE) {
1147-1154: Javadoc missingpathparameter.Document the new
patharg so readers understand when/why it’s used.Apply:
- * @param state the visitor state + * @param path optional TreePath to the invocation; when non-null, used to reconstruct + * assignment context for path-aware inference + * @param state the visitor state
1210-1226: Javadoc return contract out of sync with implementation.The text says “or null,” but the method always returns a non-null
InvocationAndContext(with a nullabletypeFromAssignmentContext). Update the doc to avoid confusion.Apply:
- * @return the correct invocation on which to perform inference, along with the relevant - * assignment context information, or {@code null} if no good assignment context can be found + * @return the invocation on which to perform inference, along with any assignment-context info + * (its {@code typeFromAssignmentContext} may be {@code null} when no context is available)
1238-1244: Track TODO for lambda handling.There’s a TODO to support lambdas here; worth opening an issue so it doesn’t get lost.
I can open a follow-up issue with a minimal repro and proposed approach. Want me to file it?
566-624: Optional: avoid broadcasting successes to unrelated nested calls.Success caching to every
invTreeinallInvocationsworks when all constraints are solved together, but it can make provenance opaque and complicate future per-site invalidation. Consider caching only for the root call and letting nested calls be re-solved when they are encountered independently.If you want, I can sketch a lightweight provenance map keyed by root invocation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(17 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#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
PR: uber/NullAway#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
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(66-665)nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java (1)
ErrorMessage(26-73)
🔇 Additional comments (4)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (4)
665-677: Good use of assignment context in constraint generation.Feeding the return-type-to-context constraint only when
typeFromAssignmentContext != nulland propagatingassignedToLocalto the solver looks correct and aligns with the objectives.
1104-1112: Early exit for non-typevar returns is fine.The explicit-nullability path is expected to be handled by the caller; this method focusing on type-variable-driven nullness is OK.
Can you confirm all callers check explicit
@Nullableon return types before calling this method?
1570-1572: MakingprettyTypeForErrorprivate is a good tightening.Removes unnecessary surface area; no concerns.
1103-1113: ```shell
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- search: keywords related to ignored/unused returns ----"
rg -n --hidden -S --glob 'nullaway/' '(ignored return|unused result|ignore result|@CanIgnoreReturnValue|CanIgnoreReturnValue|CheckReturnValue|discard|discarded|unused)' || true
echo "---- search: void method definitions in nullaway tests ----"
rg -n --hidden -S --glob 'nullaway/' '\bvoid\s+\w+\s*(' || true
echo "---- search: getGenericReturnNullnessAtInvocation references ----"
rg -n --hidden -S 'getGenericReturnNullnessAtInvocation' || true
echo "---- list test files under nullaway (if any) ----"
rg --files -g 'nullaway//test/' || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (3)
541-544: Re-run inference when a cached failure exists but a richer assignment context is now provided.Otherwise a prior failure (e.g., inferred without context) permanently blocks success when context appears.
Apply:
- if (result == null) { // have not yet attempted inference for this call + if (result == null + || (result instanceof InferenceFailure && typeFromAssignmentContext != null)) { result = runInferenceForCall(state, invocationTree, typeFromAssignmentContext, assignedToLocal); }
618-623: Don’t cache failures for all nested invocations.Caching an InferenceFailure for every
invTreecan freeze a context-less failure and prevent future successful re-inference.Apply:
- InferenceFailure failureResult = new InferenceFailure(e.getMessage()); - for (MethodInvocationTree invTree : allInvocations) { - inferredTypeVarNullabilityForGenericCalls.put(invTree, failureResult); - } - return failureResult; + // Do not cache failures; allow re-run later when a better context is available. + return new InferenceFailure(e.getMessage());
1165-1181: Allow re-inference on prior failure when a TreePath (context) is now available.Prevents a no-context failure from blocking path-aware success.
Apply:
- if (result == null) { + InvocationAndContext invocationAndType = + path == null + ? new InvocationAndContext(invocationTree, null, false) + : getInvocationAndContextForInference(path, state); + boolean shouldReinfer = + result == null + || (result instanceof InferenceFailure && invocationAndType.typeFromAssignmentContext != null); + if (shouldReinfer) { - // have not yet attempted inference for this call - MethodInvocationTree invocationTree = (MethodInvocationTree) tree; - InvocationAndContext invocationAndType = - path == null - ? new InvocationAndContext(invocationTree, null, false) - : getInvocationAndContextForInference(path, state); result = runInferenceForCall( state, invocationAndType.invocation, invocationAndType.typeFromAssignmentContext, invocationAndType.assignedToLocal); }
🧹 Nitpick comments (5)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (5)
1121-1124: Tiny cleanup: drop redundant null-check and use enum comparison.
getReturnType()won’t be null here; prefer==for enums.- if (substitutedReturnType != null - && Objects.equals(getTypeNullness(substitutedReturnType), Nullness.NULLABLE)) { + if (getTypeNullness(substitutedReturnType) == Nullness.NULLABLE) { return Nullness.NULLABLE; }
1155-1159: Javadoc parameter list out of date.Add the
@param pathdescription and fix ordering to match the signature./** * Substitutes the type arguments from a generic method invocation into the method's type. * * @param tree the method invocation tree * @param forAllType the generic method type - * @param state the visitor state + * @param path the TreePath to the invocation (used to reconstruct assignment context), or null + * @param state the visitor state * @return the substituted method type for the generic method */
1211-1226: Javadoc return contract doesn’t match implementation.Method never returns null; it falls back to
InvocationAndContext(invocation, null, false). Update docs.- * @return the correct invocation on which to perform inference, along with the relevant - * assignment context information, or {@code null} if no good assignment context can be found + * @return the correct invocation on which to perform inference, along with the relevant + * assignment context information; falls back to a context with {@code typeFromAssignmentContext = null}
1251-1254: Fix misleading inline comment.The code only uses the parent expression’s type when the parent is a MethodInvocation; otherwise it falls back to “no assignment context”.
- // could be a parameter to another method call, or part of a conditional expression, etc. - // in any case, just return the type of the parent expression + // Could be a parameter to another method call, or part of a larger expression. + // If the parent is a method invocation, use its formal parameter/receiver type; otherwise, + // fall back to no assignment context.
1354-1355: Optional: consider threadingTreePathhere too for symmetry/future cases.
getGenericParameterNullnessAtInvocationcould accept an optionalTreePathlike the return-path API, even if unused today.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(17 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#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
PR: uber/NullAway#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
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(66-665)nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java (1)
ErrorMessage(26-73)
⏰ 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). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on macos-latest
🔇 Additional comments (8)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (8)
7-7: LGTM on added imports.Preconditions, ReturnTree, NullabilityUtil, and AtomicReference uses are appropriate.
Also applies to: 25-25, 45-45, 57-57
472-481: Good: track whether RHS is assigned to a local.Plumbing
assignedToLocalinto inference improves precision for dataflow-triggered inference.
503-514: Helper to detect local-variable assignments looks correct.Covers both VariableTree and AssignmentTree; safely falls back when the symbol is null (e.g., array element writes).
665-676: LGTM: add return-type constraint from assignment context.Captures
assignedToLocaland only emits constraint when context exists.
1104-1112: Path-aware return nullness check is a solid addition.Early exit for non-typevars avoids unnecessary work; passing
pathenables assignment-context reconstruction.
1191-1208: LGTM: compact carrier for invocation + assignment context.Keeps inference plumbing tidy.
1303-1310: LGTM: assignment-site info helper.Precondition + reuse of
isAssignmentToLocalVariableare clean.
1577-1577: Visibility reduction to private is good.Helper isn’t part of the public API surface anymore.
Fixes #1262, fixes #1263
We now invoke inference for generic method calls in more places, including when it gets queried from dataflow analysis of a full method. Where possible, we provide a
TreePathto the relevant call. With thisTreePath, we can reconstruct the assignment context of the call and use that information when performing inference (see the newgetInvocationAndContextForInferencemethod). In some cases, we passnullas theTreePathsince one is not readily available. I believe that in most / all of these cases, inference should have already been performed for the call, so we should be able to use the cached result. If / when we find cases where that didn't happen, and it is impacting the inference result negatively, we can fix in follow ups.Summary by CodeRabbit