Re-introduce annotations elided by javac for certain cases#1473
Merged
Re-introduce annotations elided by javac for certain cases#1473
javac for certain cases#1473Conversation
Contributor
|
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 a private helper method restoreNestedNullabilityForTypeVarArguments to com.uber.nullaway.generics.GenericsChecks and calls it from substituteTypeArgsInGenericMethodType for generic method calls with no explicit type arguments. The helper attempts to reconstruct nested type-use nullability for top-level type-variable arguments using actual invocation argument types, producing a repaired Type.MethodType when consistent substitutions are found; if inconsistent or unrecoverable, the original call-site method type is preserved. A unit test issue1455 exercising JSPECIFY-style inference warnings was added. 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1615-1697: The method
restoreMissingNullabilityFromSingleTopLevelTypeVarArguments currently only
updates parameter types but can leave return and thrown types inconsistent when
a method type variable also appears there; after computing
repairedTopLevelSubstitutions, apply those substitutions to
methodTypeAtCallSite.getReturnType() and each type in
methodTypeAtCallSite.getThrownTypes() using
TypeSubstitutionUtils.subst(state.getTypes(), type, fromMap, toMap, config) (or
equivalent API) to produce repairedReturnType and repairedThrownTypes, set
changed if any differ, and use those repaired types when constructing the new
Type.MethodType so return/thrown nullability stays consistent with the repaired
parameters.
Comment on lines
+1615
to
+1697
| /** | ||
| * For some calls, javac drops nested type-use nullability annotations in inferred substitutions | ||
| * for method type variables. Recover these annotations from the corresponding actual argument | ||
| * types, while preserving one consistent top-level substitution per method type variable. | ||
| */ | ||
| @SuppressWarnings("ReferenceEquality") | ||
| private Type.MethodType restoreMissingNullabilityFromSingleTopLevelTypeVarArguments( | ||
| MethodInvocationTree invocationTree, | ||
| Type.MethodType origMethodType, | ||
| Type.MethodType methodTypeAtCallSite, | ||
| VisitorState state) { | ||
| Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(invocationTree); | ||
| if (methodSymbol == null || methodSymbol.isVarArgs()) { | ||
| // TODO handle varargs methods | ||
| return methodTypeAtCallSite; | ||
| } | ||
| com.sun.tools.javac.util.List<Type> origArgTypes = origMethodType.getParameterTypes(); | ||
| com.sun.tools.javac.util.List<Type> callSiteArgTypes = methodTypeAtCallSite.getParameterTypes(); | ||
| List<? extends ExpressionTree> callArgs = invocationTree.getArguments(); | ||
| if (origArgTypes.size() != callSiteArgTypes.size() || callArgs.size() != origArgTypes.size()) { | ||
| return methodTypeAtCallSite; | ||
| } | ||
|
|
||
| // use this map to store repaired substitutions for method type variables, to ensure we use the | ||
| // same repaired | ||
| // substitution for all occurrences of the same method type variable | ||
| Map<Symbol.TypeVariableSymbol, Type> repairedTopLevelSubstitutions = new HashMap<>(); | ||
| ListBuffer<Type> updatedArgTypes = new ListBuffer<>(); | ||
| boolean changed = false; | ||
| for (int i = 0; i < origArgTypes.size(); i++) { | ||
| Type updatedType = callSiteArgTypes.get(i); | ||
| Type origArgType = origArgTypes.get(i); | ||
| if (origArgType instanceof Type.TypeVar typeVar | ||
| && typeVar.tsym.owner == methodSymbol | ||
| && !(updatedType instanceof Type.TypeVar)) { | ||
| Symbol.TypeVariableSymbol typeVarSymbol = (Symbol.TypeVariableSymbol) typeVar.tsym; | ||
| Type repairedSubstitution = repairedTopLevelSubstitutions.get(typeVarSymbol); | ||
| if (repairedSubstitution != null) { | ||
| if (!state | ||
| .getTypes() | ||
| .isSameType( | ||
| state.getTypes().erasure(repairedSubstitution), | ||
| state.getTypes().erasure(updatedType))) { | ||
| // Inconsistent substitution for the same top-level type variable; bail out. | ||
| return methodTypeAtCallSite; | ||
| } | ||
| if (repairedSubstitution != updatedType) { | ||
| changed = true; | ||
| updatedType = repairedSubstitution; | ||
| } | ||
| } else { // need to compute the substitution | ||
| Type actualArgType = getTreeType(callArgs.get(i), state); | ||
| if (actualArgType != null | ||
| && !actualArgType.isRaw() | ||
| && state | ||
| .getTypes() | ||
| .isSameType( | ||
| state.getTypes().erasure(actualArgType), | ||
| state.getTypes().erasure(updatedType))) { | ||
| Type restoredType = | ||
| TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations( | ||
| actualArgType, updatedType, config, Collections.emptyMap()); | ||
| repairedTopLevelSubstitutions.put(typeVarSymbol, restoredType); | ||
| if (restoredType != updatedType) { | ||
| changed = true; | ||
| updatedType = restoredType; | ||
| } | ||
| } else { | ||
| repairedTopLevelSubstitutions.put(typeVarSymbol, updatedType); | ||
| } | ||
| } | ||
| } | ||
| updatedArgTypes.append(updatedType); | ||
| } | ||
| if (!changed) { | ||
| return methodTypeAtCallSite; | ||
| } | ||
| return new Type.MethodType( | ||
| updatedArgTypes.toList(), | ||
| methodTypeAtCallSite.getReturnType(), | ||
| methodTypeAtCallSite.getThrownTypes(), | ||
| methodTypeAtCallSite.tsym); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate existing usage patterns of Types.subst and TypeSubstitutionUtils.subst
rg -n "Types\.subst\(|TypeSubstitutionUtils\.subst" -S --type java | head -30Repository: uber/NullAway
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Check the broader context of the GenericsChecks.java file to understand the pattern
rg -n "TypeSubstitutionUtils|Types.*subst" nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 1390
🏁 Script executed:
#!/bin/bash
# Look for usages of getTypeParameters and method type variable handling in the file
rg -n "getTypeParameters|TypeVariableSymbol" nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java -A 2 -B 2Repository: uber/NullAway
Length of output: 1302
🏁 Script executed:
#!/bin/bash
# Check if there are existing tests for the restoreMissingNullabilityFromSingleTopLevelTypeVarArguments method
fd -t f "Test.*\.java" nullaway | xargs grep -l "restoreMissingNullabilityFromSingleTopLevelTypeVarArguments" 2>/dev/nullRepository: uber/NullAway
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the context around line 1611 to see how TypeSubstitutionUtils.subst is used
sed -n '1605,1620p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 784
🏁 Script executed:
#!/bin/bash
# Look at the TypeSubstitutionUtils class to understand available subst methods
grep -n "\.subst\|public.*subst" nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java | head -20Repository: uber/NullAway
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Examine the entire method being reviewed to check if return types are mentioned
sed -n '1615,1697p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 3886
🏁 Script executed:
#!/bin/bash
# Find where restoreMissingNullabilityFromSingleTopLevelTypeVarArguments is called
rg -n "restoreMissingNullabilityFromSingleTopLevelTypeVarArguments" nullaway/Repository: uber/NullAway
Length of output: 364
🏁 Script executed:
#!/bin/bash
# Look for tests related to this method or similar return type scenarios
fd -t f ".*Test.*\.java" nullaway | xargs grep -l "restoreMissing\|return.*type.*var" 2>/dev/null | head -5Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are any TODOs or comments about return type handling near this method
sed -n '1597,1700p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | grep -i "todo\|fixme\|return\|thrown"Repository: uber/NullAway
Length of output: 615
Consider return/thrown type repair as a follow-up enhancement.
The method currently repairs parameter substitutions but leaves return and thrown types untouched. While technically consistent within this method's narrow scope, if a method type variable appears in the return type (e.g., <T extends Supplier<?>> T acceptSup(T)), parameter repairs to Supplier<@Nullable OuterT> paired with an unrepaired return Supplier<OuterT> could create inconsistent nullability in inferred types.
The suggested approach—applying repaired substitutions to return/thrown types using TypeSubstitutionUtils.subst(state.getTypes(), type, from, to, config)—is sound. However, this extends the method's scope beyond single top-level type var arguments. Consider addressing this in a follow-up PR to keep the current change focused.
🤖 Prompt for AI Agents
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around
lines 1615 - 1697, The method
restoreMissingNullabilityFromSingleTopLevelTypeVarArguments currently only
updates parameter types but can leave return and thrown types inconsistent when
a method type variable also appears there; after computing
repairedTopLevelSubstitutions, apply those substitutions to
methodTypeAtCallSite.getReturnType() and each type in
methodTypeAtCallSite.getThrownTypes() using
TypeSubstitutionUtils.subst(state.getTypes(), type, fromMap, toMap, config) (or
equivalent API) to produce repairedReturnType and repairedThrownTypes, set
changed if any differ, and use those repaired types when constructing the new
Type.MethodType so return/thrown nullability stays consistent with the repaired
parameters.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1473 +/- ##
============================================
+ Coverage 88.40% 88.43% +0.02%
- Complexity 2799 2808 +9
============================================
Files 99 99
Lines 9360 9406 +46
Branches 1880 1890 +10
============================================
+ Hits 8275 8318 +43
Misses 529 529
- Partials 556 559 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
changed the title
javac for certain casesjavac for certain cases
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1819-1901: The method
restoreMissingNullabilityFromSingleTopLevelTypeVarArguments updates only
parameter types but leaves return and thrown types unmodified, so occurrences of
the same method type variables in return/throws remain unrepaired; update the
return type and each thrown type using the same repairedTopLevelSubstitutions
before constructing the new Type.MethodType. Specifically, for method
restoreMissingNullabilityFromSingleTopLevelTypeVarArguments compute
substitutedReturn = if returnType contains a method type variable present in
repairedTopLevelSubstitutions then use
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations or equivalent
substitution (ensuring erasure matches as you do for params) to apply the
repaired substitution; do the same for each element of
methodTypeAtCallSite.getThrownTypes(); then pass updatedArgTypes.toList(),
substitutedReturn, and the updated thrown types into the new Type.MethodType so
return/throws reflect the same repaired top-level substitutions.
yuxincs
approved these changes
Feb 21, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1978-1981: The null check on ASTHelpers.getSymbol(invocationTree)
is dead code; update the guard in GenericsChecks.java to only check for varargs
by replacing the condition "methodSymbol == null || methodSymbol.isVarArgs()"
with "methodSymbol.isVarArgs()", and remove the TODO/comment about handling
nulls; keep the early return of methodTypeAtCallSite when the method is varargs
so callers (methodSymbol, ASTHelpers.getSymbol(invocationTree),
methodTypeAtCallSite) remain consistent.
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 2044-2049: The returned Type.MethodType still uses the original
un-repaired return and thrown types (methodTypeAtCallSite.getReturnType() and
getThrownTypes()), causing inconsistent views; update this to repair the return
type and each thrown type before constructing the new Type.MethodType (e.g.,
call the same repair logic you used for parameters on
methodTypeAtCallSite.getReturnType() and map/repair each element of
methodTypeAtCallSite.getThrownTypes()), then pass the repairedReturnType and
repairedThrownTypes to the Type.MethodType constructor instead of the originals.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1978-1982: The null check against
ASTHelpers.getSymbol(invocationTree) is unreachable; simplify the guard in
GenericsChecks by removing the methodSymbol == null branch and only checking
methodSymbol.isVarArgs(), i.e., replace the current if-condition that tests both
null and varargs with a single methodSymbol.isVarArgs() check (you can also drop
the accompanying "TODO handle varargs methods" only-if you prefer to keep it but
ensure logic only depends on methodSymbol.isVarArgs()); reference symbols:
ASTHelpers.getSymbol(invocationTree), methodSymbol, invocationTree, and
methodTypeAtCallSite.
- Around line 2044-2048: The returned MethodType is not applying the
repairedTopLevelSubstitutions to the return and thrown types, so update the code
that constructs the Type.MethodType in GenericsChecks (where new Type.MethodType
is created from methodTypeAtCallSite) to mirror how argument types are repaired:
apply repairedTopLevelSubstitutions to methodTypeAtCallSite.getReturnType() and
to each type in methodTypeAtCallSite.getThrownTypes() (using the same
substitution utility used for updatedArgTypes) before passing them into the
Type.MethodType constructor so repaired/nullability annotations are preserved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1996-2040: The loop in GenericsChecks.java uses indexed get(i) on
com.sun.tools.javac.util.List (origArgTypes, callSiteArgTypes, callArgs) which
is O(n^2); replace it with parallel head/tail traversal: create three list
cursors (e.g., orig = origArgTypes, callSite = callSiteArgTypes, callArgTrees =
callArgs) and iterate while orig != null, using orig.head / orig.tail and
callSite.head / callSite.tail and callArgTrees.head / callArgTrees.tail to
obtain origArgType, updatedType, and call arg tree respectively, preserving the
existing logic that references repairedTopLevelSubstitutions,
getTreeType(callArgs.get(i), state),
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations, and returning
methodTypeAtCallSite when needed; append updatedType to updatedArgTypes as
before.
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 2044-2049: The new Type.MethodType currently uses
methodTypeAtCallSite.getReturnType() and getThrownTypes() unchanged, so
repairedTopLevelSubstitutions aren't applied to return/thrown types; update
updateMethodTypeWithInferredNullability to call
TypeSubstitutionUtils.subst(repairedTopLevelSubstitutions,
methodTypeAtCallSite.getReturnType()) and similarly subst each entry of
methodTypeAtCallSite.getThrownTypes() before constructing the Type.MethodType
(while still using updatedArgTypes.toList() for params and
methodTypeAtCallSite.tsym) so the repaired nullability is reflected in return
and thrown types.
Comment on lines
+1996
to
+2040
| for (int i = 0; i < origArgTypes.size(); i++) { | ||
| Type updatedType = callSiteArgTypes.get(i); | ||
| Type origArgType = origArgTypes.get(i); | ||
| if (origArgType instanceof Type.TypeVar typeVar | ||
| && typeVar.tsym.owner == methodSymbol | ||
| && !(updatedType instanceof Type.TypeVar)) { | ||
| Symbol.TypeVariableSymbol typeVarSymbol = (Symbol.TypeVariableSymbol) typeVar.tsym; | ||
| Type repairedSubstitution = repairedTopLevelSubstitutions.get(typeVarSymbol); | ||
| if (repairedSubstitution != null) { | ||
| if (!state | ||
| .getTypes() | ||
| .isSameType( | ||
| state.getTypes().erasure(repairedSubstitution), | ||
| state.getTypes().erasure(updatedType))) { | ||
| // Inconsistent substitution for the same top-level type variable; bail out. | ||
| return methodTypeAtCallSite; | ||
| } | ||
| if (repairedSubstitution != updatedType) { | ||
| changed = true; | ||
| updatedType = repairedSubstitution; | ||
| } | ||
| } else { // need to compute the substitution | ||
| Type actualArgType = getTreeType(callArgs.get(i), state); | ||
| if (actualArgType != null | ||
| && !actualArgType.isRaw() | ||
| && state | ||
| .getTypes() | ||
| .isSameType( | ||
| state.getTypes().erasure(actualArgType), | ||
| state.getTypes().erasure(updatedType))) { | ||
| Type restoredType = | ||
| TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations( | ||
| actualArgType, updatedType, config, Collections.emptyMap()); | ||
| repairedTopLevelSubstitutions.put(typeVarSymbol, restoredType); | ||
| if (restoredType != updatedType) { | ||
| changed = true; | ||
| updatedType = restoredType; | ||
| } | ||
| } else { | ||
| repairedTopLevelSubstitutions.put(typeVarSymbol, updatedType); | ||
| } | ||
| } | ||
| } | ||
| updatedArgTypes.append(updatedType); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Replace indexed access with iterator-based traversal for com.sun.tools.javac.util.List.
origArgTypes.get(i) and callSiteArgTypes.get(i) are each O(i) on the linked-list com.sun.tools.javac.util.List, making the loop O(n²). In practice this is negligible, but the idiomatic javac pattern (used in visitTypeLists in RestoreNullnessAnnotationsVisitor) uses parallel l.tail/l.head traversal:
♻️ Proposed refactor (O(n) traversal)
- for (int i = 0; i < origArgTypes.size(); i++) {
- Type updatedType = callSiteArgTypes.get(i);
- Type origArgType = origArgTypes.get(i);
+ int i = 0;
+ for (
+ com.sun.tools.javac.util.List<Type> origL = origArgTypes,
+ callSiteL = callSiteArgTypes;
+ origL.nonEmpty();
+ origL = origL.tail, callSiteL = callSiteL.tail, i++) {
+ Type updatedType = callSiteL.head;
+ Type origArgType = origL.head;
...
Type actualArgType = getTreeType(callArgs.get(i), state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around
lines 1996 - 2040, The loop in GenericsChecks.java uses indexed get(i) on
com.sun.tools.javac.util.List (origArgTypes, callSiteArgTypes, callArgs) which
is O(n^2); replace it with parallel head/tail traversal: create three list
cursors (e.g., orig = origArgTypes, callSite = callSiteArgTypes, callArgTrees =
callArgs) and iterate while orig != null, using orig.head / orig.tail and
callSite.head / callSite.tail and callArgTrees.head / callArgTrees.tail to
obtain origArgType, updatedType, and call arg tree respectively, preserving the
existing logic that references repairedTopLevelSubstitutions,
getTreeType(callArgs.get(i), state),
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations, and returning
methodTypeAtCallSite when needed; append updatedType to updatedArgTypes as
before.
lazaroclapp
approved these changes
Mar 25, 2026
Collaborator
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
This LGTM as far as it goes, not sure what the full scope of the javac bug is, but it does seem to fix the issue without breaking existing tests. Not sure in which cases it could diverge from "expected" javac inference 🤔
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1455
For reference here is the test case:
What goes wrong here is at the call
acceptSup(sup),javacinfers the type argument to beSupplier<OuterT>, dropping the@Nullableannotation onOuterTseen in the declaration ofsup. I cannot figure out exactly when annotations get dropped; e.g., it does not seem to happen ifsupis a formal parameter rather than a field. For the case above, our inference correctly infers thatTitself should be@NonNull, but then we still report an error sinceSupplier<@Nullable OuterT>(the type ofsup) is incompatible withSupplier<OuterT>(what we think is the formal parameter type).This started happening in 0.13.0 due to #1348, because we started computing the proper method types at call sites to report better error messages. Unfortunately, this is leading to unforeseen interactions with
javac's type inference like the issue above; but I'd like to see if we can work around the corner cases since it's important to have good error messages.For this case, we add a method
restoreMissingNullabilityFromSingleTopLevelTypeVarArgumentsthat repairs the method type by re-introducing explicit nullability annotations nested in actual parameter types onto the formal parameter types. We try to tailor the fix narrowly for this case and to not introduce unsoundness by re-introducing annotations across parameter positions inconsistently.Summary by CodeRabbit
Bug Fixes
Tests