Improve handling of var-declared local variables#1573
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1573 +/- ##
============================================
- Coverage 88.42% 88.37% -0.06%
- Complexity 2885 2910 +25
============================================
Files 103 103
Lines 9650 9709 +59
Branches 1942 1959 +17
============================================
+ Hits 8533 8580 +47
- Misses 535 538 +3
- Partials 582 591 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
force-pushed
the
handle-var-better
branch
2 times, most recently
from
51d4506 to
6958035
Compare
msridhar
force-pushed
the
nested-receiver-inference
branch
from
ca80da3 to
d293b16
Compare
msridhar
force-pushed
the
handle-var-better
branch
from
3699dd8 to
0510d8e
Compare
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
There seem to be changes here other than adding the cache, also, the description doesn't fully motivate the cache. Is this a performance-only optimization or do we expect the semantics to be different with the cache (given all the tests, I assume the second, but don't see it spelled out in the description 😅 )
| pathToRhs, | ||
| lhsType, | ||
| // if a local is declared using `var`, don't use its javac-inferred type as part of | ||
| // inference |
There was a problem hiding this comment.
Why is this the case?
There was a problem hiding this comment.
It's because javac doesn't properly infer nullability of type arguments. So, if you have:
<T extends @Nullable Object> Foo<T> make(T t) { ... }
void baz(@Nullable String s) {
var t = make(s); ...
}javac may well compute the type of t as Foo, but with nullability, it should be Foo<@Nullable String>. So, for var-declared variables assigned the result of a generic method call, we run our own inference on the call ignoring the javac-inferred type of the variable, and then remember our own inference result as the variable's type. I will update the code comment.
| ElementKind kind = symbol.getKind(); | ||
| return (kind.equals(ElementKind.LOCAL_VARIABLE) || kind.equals(ElementKind.RESOURCE_VARIABLE)) | ||
| && symbol.owner != null | ||
| ? new VarLocalKey(symbol.owner, symbol.name) |
There was a problem hiding this comment.
Just to double-check here: Is there any shadowing of note here that might cause us to see two local variables with the same owner (presumably a method or lambda?) and same name?
While Java seems fairly strict about re-declarations of locals, I could, for example, get this to compile:
class Main {
public static void main(String[] args) {
int i = 0;
for(var foo = "Hello"; i == 0; ++i) {
System.out.println(foo);
}
i = 0;
for(var foo = 5; i == 0; ++i) {
System.out.println(5);
}
}
}
I suspect we could translate that into a test case about nullability where we can cause a key collision here, no?
There was a problem hiding this comment.
You're right, this is an issue. Writing a test for it exposed a subtle bug. Working on it.
There was a problem hiding this comment.
Went down a bit of a rabbit hole on this. It's fixed, along with other issues. This PR probably needs a from-scratch review.
msridhar
force-pushed
the
handle-var-better
branch
from
0510d8e to
9402f1a
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds context-aware type inference and caching for local variables declared with Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 121-122: The current varLocalDeclarations map only stores the
JCVariableDecl, so getInferredVarLocalType rebuilds pathToInitializer from the
use-site path and loses the declaration's parent context; fix by preserving the
declaration TreePath at registration time (or eagerly computing and caching the
inferred type when registering the var). Update the registration code that
writes to varLocalDeclarations to also store the declaration TreePath (e.g., add
a secondary map or change the stored value to include the TreePath) or compute
and cache the inferred type for that JCVariableDecl immediately, then change
getInferredVarLocalType to read the preserved TreePath / cached type instead of
recomputing from the use-site path; reference varLocalDeclarations,
getInferredVarLocalType, and pathToInitializer when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e32bd5b-4f61-4736-b9f1-33e69b6269d4
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/VarDeclaredLocalTests.java
There was a problem hiding this comment.
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/generics/GenericsChecks.java (2)
1032-1055:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
calledFromDataflowthrough constraint generation.
runInferenceForCall(..., true)still drops the flag before enteringgenerateConstraintsForCall(). From there, pseudo-assignment typing falls back to the normalgetTreeType(...)flow, so a dataflow-originated inference can still populate caches from non-fixed-point facts while analyzing arguments or nested receivers. Thread the flag through the constraint-generation helpers and use the 3-arggetTreeType(...)consistently.Suggested fix
private void generateConstraintsForCall( VisitorState state, `@Nullable` TreePath path, `@Nullable` Type typeFromAssignmentContext, boolean assignedToLocal, + boolean calledFromDataflow, ConstraintSolver solver, Symbol.MethodSymbol methodSymbol, MethodInvocationTree methodInvocationTree, Set<MethodInvocationTree> allInvocations) throws UnsatisfiableConstraintsException { @@ generateConstraintsForPseudoAssignment( state.withPath(pathToArgument), solver, allInvocations, argument, - formalParamType); + formalParamType, + calledFromDataflow); } private void generateConstraintsForPseudoAssignment( VisitorState state, ConstraintSolver solver, Set<MethodInvocationTree> allInvocations, ExpressionTree rhsExpr, - Type lhsType) { + Type lhsType, + boolean calledFromDataflow) { @@ - Type argumentType = getTreeType(rhsExpr, state); + Type argumentType = getTreeType(rhsExpr, state, calledFromDataflow);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 1032 - 1055, runInferenceForCall currently drops the calledFromDataflow flag before calling generateConstraintsForCall; propagate this boolean into generateConstraintsForCall and all downstream constraint-generation helpers so they know the call originated from dataflow, and update those helpers to pass it along. Also ensure you call the 3-arg getTreeType(...) (including the calledFromDataflow flag) everywhere inside the constraint generation path (e.g., in methods that analyze arguments, receivers, and nested invocations) so pseudo-assignment typing uses dataflow-aware lookup consistently; update signatures and call sites for generateConstraintsForCall and any helper methods to accept and forward calledFromDataflow.
1065-1081:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip poly-expression cache writes during dataflow inference.
inferredPolyExpressionTypesis populated even whencalledFromDataflowis true. Those entries are derived from the same inference result this method deliberately avoids caching elsewhere, so later non-dataflow visits can read stale lambda/member-reference target types from the cache.Suggested fix
- // Store inferred types for lambda arguments - new InvocationArguments(invocationTree, methodSymbol.type.asMethodType()) - .forEach( - (argument, argPos, formalParamType, unused) -> { - if (argument instanceof LambdaExpressionTree - || argument instanceof MemberReferenceTree) { - Type polyExprTreeType = ASTHelpers.getType(argument); - if (polyExprTreeType != null) { - Type typeWithInferredNullability = - TypeSubstitutionUtils.updateTypeWithInferredNullability( - polyExprTreeType, formalParamType, typeVarNullability, state, config); - inferredPolyExpressionTypes.put(argument, typeWithInferredNullability); - } - } - }); + if (!calledFromDataflow) { + new InvocationArguments(invocationTree, methodSymbol.type.asMethodType()) + .forEach( + (argument, argPos, formalParamType, unused) -> { + if (argument instanceof LambdaExpressionTree + || argument instanceof MemberReferenceTree) { + Type polyExprTreeType = ASTHelpers.getType(argument); + if (polyExprTreeType != null) { + Type typeWithInferredNullability = + TypeSubstitutionUtils.updateTypeWithInferredNullability( + polyExprTreeType, formalParamType, typeVarNullability, state, config); + inferredPolyExpressionTypes.put(argument, typeWithInferredNullability); + } + } + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 1065 - 1081, The code is writing inferred lambda/member-reference types into inferredPolyExpressionTypes even when calledFromDataflow is true; to avoid caching dataflow-only inference results, skip adding entries when calledFromDataflow is true. Modify the lambda passed to InvocationArguments.forEach so that before calling inferredPolyExpressionTypes.put(argument, typeWithInferredNullability) you check if (!calledFromDataflow) (or return early when calledFromDataflow) and only then perform the put; keep the existing computation of typeWithInferredNullability if you still need it for local logic, but do not mutate inferredPolyExpressionTypes when calledFromDataflow is true (references: inferredPolyExpressionTypes, calledFromDataflow, InvocationArguments.forEach, TypeSubstitutionUtils.updateTypeWithInferredNullability, InferenceSuccess).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1032-1055: runInferenceForCall currently drops the
calledFromDataflow flag before calling generateConstraintsForCall; propagate
this boolean into generateConstraintsForCall and all downstream
constraint-generation helpers so they know the call originated from dataflow,
and update those helpers to pass it along. Also ensure you call the 3-arg
getTreeType(...) (including the calledFromDataflow flag) everywhere inside the
constraint generation path (e.g., in methods that analyze arguments, receivers,
and nested invocations) so pseudo-assignment typing uses dataflow-aware lookup
consistently; update signatures and call sites for generateConstraintsForCall
and any helper methods to accept and forward calledFromDataflow.
- Around line 1065-1081: The code is writing inferred lambda/member-reference
types into inferredPolyExpressionTypes even when calledFromDataflow is true; to
avoid caching dataflow-only inference results, skip adding entries when
calledFromDataflow is true. Modify the lambda passed to
InvocationArguments.forEach so that before calling
inferredPolyExpressionTypes.put(argument, typeWithInferredNullability) you check
if (!calledFromDataflow) (or return early when calledFromDataflow) and only then
perform the put; keep the existing computation of typeWithInferredNullability if
you still need it for local logic, but do not mutate inferredPolyExpressionTypes
when calledFromDataflow is true (references: inferredPolyExpressionTypes,
calledFromDataflow, InvocationArguments.forEach,
TypeSubstitutionUtils.updateTypeWithInferredNullability, InferenceSuccess).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6dfbda00-7589-492f-bde0-ce220b6e9091
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.javanullaway/src/main/java/com/uber/nullaway/dataflow/RunOnceForwardAnalysisImpl.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/VarDeclaredLocalTests.java
| for (MethodInvocationTree invTree : allInvocations) { | ||
| inferredTypeVarNullabilityForGenericCalls.put(invTree, successResult); | ||
| } | ||
| // Store inferred types for lambda or method reference arguments |
There was a problem hiding this comment.
This change is not strictly related to the PR, but it was spotted by coderabbit and relates to avoiding caching results when called from dataflow. It's a relatively simple fix so I decided to include it without doing a separate PR, but could separate it out if desired.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
495-565:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the cached
vartype for assignment targets too.This only fixes
IdentifierTreelookups. A laterfoo = ...still gets its LHS type fromlhsSymbol.type, so avarlocal whose declaration was repaired toFoo<@nullableString>is re-checked/re-inferred as javac'sFoo<String>. That leaves reassignments and RHS generic-call inference incorrect for the same local.Suggested fix
@@ } else if (tree instanceof AssignmentTree assignmentTree) { // type on the tree itself can be missing nested annotations for arrays; get the type from // the symbol for the assigned location instead, if available Symbol lhsSymbol = ASTHelpers.getSymbol(assignmentTree.getVariable()); if (lhsSymbol != null) { - result = lhsSymbol.type; + Type inferredVarLocalType = + getInferredVarLocalType(lhsSymbol, state, calledFromDataflow); + result = inferredVarLocalType != null ? inferredVarLocalType : lhsSymbol.type; } else { result = ASTHelpers.getType(assignmentTree); }Also applies to: 2356-2372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around lines 495 - 565, The IdentifierTree branch in getTreeType reuses getInferredVarLocalType but assignment targets still read lhsSymbol.type causing var locals with repaired generic annotations to lose inferred annotations on reassignments; update the AssignmentTree handling to call getInferredVarLocalType(symbol, state, calledFromDataflow) (same as used in the IdentifierTree branch) and if it returns non-null use that Type instead of lhsSymbol.type. Locate the assignment/compound-assignment handling in getTreeType and replace the direct use of lhsSymbol.type with the inferredVarLocalType when available; ensure to reference getInferredVarLocalType, AssignmentTree, and lhsSymbol.type so the change mirrors the IdentifierTree fix and also apply the same change to the other occurrence referenced (lines ~2356-2372).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 495-565: The IdentifierTree branch in getTreeType reuses
getInferredVarLocalType but assignment targets still read lhsSymbol.type causing
var locals with repaired generic annotations to lose inferred annotations on
reassignments; update the AssignmentTree handling to call
getInferredVarLocalType(symbol, state, calledFromDataflow) (same as used in the
IdentifierTree branch) and if it returns non-null use that Type instead of
lhsSymbol.type. Locate the assignment/compound-assignment handling in
getTreeType and replace the direct use of lhsSymbol.type with the
inferredVarLocalType when available; ensure to reference
getInferredVarLocalType, AssignmentTree, and lhsSymbol.type so the change
mirrors the IdentifierTree fix and also apply the same change to the other
occurrence referenced (lines ~2356-2372).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba75d815-1f42-4159-b3af-d62ac00c5925
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
msridhar
force-pushed
the
handle-var-better
branch
from
97205e2 to
5fed645
Compare
msridhar
force-pushed
the
handle-var-better
branch
from
5fed645 to
89aaeca
Compare
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
Minor, perhaps dumb, question, but otherwise this LGTM
| void testPositive2(Foo<@Nullable Object> f1, Foo<Object> f2) { | ||
| var foo = makeNested(f1); | ||
| // BUG: Diagnostic contains: inference failure | ||
| foo = makeNested(f2); |
There was a problem hiding this comment.
Why is this one a true positive and the one below a true negative, again?
We have: Foo<Object> :<: Foo<@Nullable Object> but not
Foo<Foo<Object>> :<: Foo<Foo<@Nullable Object>>?
There was a problem hiding this comment.
For testNegative(), at the var foo = make(null) assignment we infer the type of foo as Foo<@Nullable Object>. Then, for the statement foo = make(new Object()), we can again infer the type returned by make to be Foo<@Nullable Object> to make type checking succeed (since you can pass new Object() to a parameter of type @Nullable Object. I will add a comment to the test to explain this, it's subtle.
2 participants
Fix #1572
Infer the nullability of type arguments for locals declared with
var, based on the initializer at the variable's declaration. This change is a bit complex for two reasons. First, there is no efficient way withjavacto get the declaration of a local from itsSymbol. In the course of running the normal NullAway visitor, we should always visit the declaration of a variable before its uses, so as long as we cache the inferred type from the declaration, it should be available.But, the second issue is that dataflow analysis may require the type of a
var-declared local before the main NullAway visitor reaches the declaration. To handle this case, we cache declarations ofvar-declared locals as they are reached by the dataflow analysis, and use that cache of declarations to infer types on demand at uses. This is a bit subtle / fragile, but we want to minimize overhead, and the alternative would be to run a full visitor to find these declarations, which could be expensive.We also need to thread the
calledFromDataflowflag through more places, since we should not cache the inferred type of such a local when called from dataflow (since the inferred type may rely on incomplete dataflow results; see testvarGenericInferenceFromDataflowInLoop()). Many of the changes in this PR are just passingcalledFromDataflowthrough more methods.Integration test failures are expected.
Summary by CodeRabbit
New Features
Bug Fixes
Tests