Initial inference for wildcards#1549
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1549 +/- ##
============================================
- Coverage 88.52% 88.45% -0.07%
- Complexity 2862 2869 +7
============================================
Files 103 103
Lines 9522 9586 +64
Branches 1911 1930 +19
============================================
+ Hits 8429 8479 +50
- Misses 528 532 +4
- Partials 565 575 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
force-pushed
the
wildcard-opposite-bounds
branch
from
627c9d2 to
ef71fef
Compare
msridhar
force-pushed
the
wildcard-inference-experiment
branch
from
8a72414 to
a1d6652
Compare
msridhar
force-pushed
the
wildcard-inference-experiment
branch
from
aa9dcd2 to
493301a
Compare
msridhar
force-pushed
the
wildcard-inference-experiment
branch
from
4771d4c to
46e2b25
Compare
msridhar
changed the title
WalkthroughRefactors wildcard upper-bound extraction into new GenericsUtils helpers and removes the visitor-local helper. Updates CheckIdenticalNullabilityVisitor to use those helpers and to normalize RHS wildcards when comparing extends bounds. Extends ConstraintSolverImpl to generate containment-specific nullability constraints for type arguments (including wildcard↔wildcard and wildcard↔concrete relations) and adds a visitWildcardType path. Broadens TypeSubstitutionUtils to restore nullability from both EXTENDS and SUPER wildcard bounds. Adds and re-enables several JSpecify wildcard-related tests. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 3
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/ConstraintSolverImpl.java (1)
386-395:⚠️ Potential issue | 🟡 MinorVerify that capture symbols in the inferred nullability map won't cause inefficiencies.
Removing the
CapturedTypeexclusion means captured wildcards now create inference variables and their synthetic capture symbols end up as keys in the solution map. While downstream consumers likeTypeSubstitutionUtils.substituteInferredNullabilityForTypeVariablessafely handle these via symbol matching (capture symbols won't match any declared type parameters), having entries for symbols that never substitute anything adds unnecessary overhead. Consider whether filtering the result map to exclude non-type-parameter symbols would improve efficiency.🤖 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/ConstraintSolverImpl.java` around lines 386 - 395, The inferred-nullability map currently retains entries for synthetic capture symbols (CapturedType) which never correspond to declared type parameters; after building the map in ConstraintSolverImpl (the place that previously relied on isTypeVariable), filter the map to only keep keys that are actual declared type-parameter symbols (e.g., check symbol.getKind() == ElementKind.TYPE_PARAMETER or otherwise confirm the Symbol represents a declared type parameter) before returning/using it so downstream code like TypeSubstitutionUtils.substituteInferredNullabilityForTypeVariables doesn't carry useless capture-symbol entries; update the map-construction/return site to drop any symbol that is clearly a capture/non-type-parameter.
🤖 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/ConstraintSolverImpl.java`:
- Around line 162-168: The visitWildcardType method currently always calls
constrainWildcardToSupertype(subtype, supertype) without checking the
localVariableType flag like visitType does; mirror visitType’s behavior by
adding the same Verify.verify(!localVariableType, "...") guard at the start of
visitWildcardType (or, if intended to allow wildcards for localVariableType, add
a clear comment and an explicit branch explaining why) so that entering
visitWildcardType via addSubtypeConstraint(..., true) is caught or intentionally
handled; reference visitWildcardType, visitType, localVariableType,
constrainWildcardToSupertype, and addSubtypeConstraint when making the change.
- Around line 209-228: In constrainContainedByWildcard, when supertypeWildcard
is SUPER and the actual subtypeTypeArg is a wildcard that is not BoundKind.SUPER
the code intentionally performs no constraint (the branch falls through) but
this silent no-op should be documented; add an inline comment inside the SUPER
case (near variables supertypeLowerBound, subtypeWildcard and the conditional
checking subtypeWildcard.kind == BoundKind.SUPER) explaining that containment
for formals of the shape '? super S' with actuals '? extends X' or unbound is
illegal in source Java, is reachable during inference, and is intentionally
elided here (mirroring the comment pattern in
CheckIdenticalNullabilityVisitor.superWildcardContains) so future inference work
will revisit it.
- Around line 261-267: The current constrainWildcardToSupertype method
over-constrains in the SUPER branch by calling
castToNonNull(subtypeWildcard.getSuperBound()).accept(this, supertype), which
effectively forces S <: T for "? super S <: T" (an unsound constraint w.r.t.
lower-bound extraction); change the SUPER branch to avoid deriving a constraint
on the super-bound (i.e., do not call castToNonNull(...).accept(...)) — either
make it a no-op or only add a conservative comment explaining the choice and
refer to issue `#1551` for follow-up; keep the existing non-SUPER behavior that
uses GenericsUtils.wildcardUpperBound(...).accept(this, supertype) and preserve
function names constrainWildcardToSupertype, castToNonNull, and
GenericsUtils.wildcardUpperBound in the diff for easy location.
---
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java`:
- Around line 386-395: The inferred-nullability map currently retains entries
for synthetic capture symbols (CapturedType) which never correspond to declared
type parameters; after building the map in ConstraintSolverImpl (the place that
previously relied on isTypeVariable), filter the map to only keep keys that are
actual declared type-parameter symbols (e.g., check symbol.getKind() ==
ElementKind.TYPE_PARAMETER or otherwise confirm the Symbol represents a declared
type parameter) before returning/using it so downstream code like
TypeSubstitutionUtils.substituteInferredNullabilityForTypeVariables doesn't
carry useless capture-symbol entries; update the map-construction/return site to
drop any symbol that is clearly a capture/non-type-parameter.
🪄 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: a96d964c-0e65-4b6b-8fc2-341ef6a0ade8
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ConstraintSolverImpl.java`:
- Around line 162-168: The override visitWildcardType currently returns early
when config.handleWildcardGenerics() is false, which drops the previous behavior
of delegating to the generic visitor and breaks equateTypeArguments for wildcard
subtype arguments; change visitWildcardType in ConstraintSolverImpl so that when
handleWildcardGenerics() is true it performs the existing Verify and
constrainWildcardToSupertype(...) logic, and otherwise it delegates to the
superclass/generic path by calling visitType(subtype, supertype) (preserving
localVariableType handling by not bypassing it), ensuring equateTypeArguments
behavior is unchanged when the flag is off.
- Around line 182-189: The code in ConstraintSolverImpl currently calls
GenericsUtils.asWildcard on subtype/supertype type args which unwraps
CapturedType back to its source wildcard and loses the captured variable
identity; change the logic to detect captured types (after isTypeVariable
expanded) and handle them in a dedicated branch instead of calling
GenericsUtils.asWildcard: when you see a CapturedType preserve the capture
variable and generate constraints using that captured variable (i.e., add
constraints involving the capture variable directly rather than delegating to
constrainContainedByWildcard or constrainWildcardContainedByConcrete), only
falling back to asWildcard for true wildcards.
🪄 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: 25858af5-6e3f-42e4-a641-d366ab4923b1
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
msridhar
force-pushed
the
wildcard-inference-experiment
branch
from
8a2f2aa to
2ebfce6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1)
163-168:⚠️ Potential issue | 🟠 MajorPreserve the pre-flag path in
visitWildcardType().Line 168 returns
nullwhenhandleWildcardGenerics()is disabled, so wildcard subtype arguments stop delegating to the generic visitor path. That changesequateTypeArguments(...)behavior even though the feature is off by default. Please fall back tovisitType(subtype, supertype)in the disabled branch.Suggested fix
`@Override` public `@Nullable` Void visitWildcardType(WildcardType subtype, Type supertype) { - if (config.handleWildcardGenerics()) { - Verify.verify(!localVariableType, "A wildcard type cannot be assigned to a local variable"); - constrainWildcardToSupertype(subtype, supertype); - } - return null; + if (!config.handleWildcardGenerics()) { + return visitType(subtype, supertype); + } + Verify.verify(!localVariableType, "A wildcard type cannot be assigned to a local variable"); + constrainWildcardToSupertype(subtype, supertype); + return null; }🤖 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/ConstraintSolverImpl.java` around lines 163 - 168, In visitWildcardType(WildcardType subtype, Type supertype) ensure the disabled-flag path preserves the original delegation: when config.handleWildcardGenerics() is false, call visitType(subtype, supertype) instead of simply returning null so wildcard subtype arguments continue to follow the generic visitor path (preserving behavior used by equateTypeArguments); keep the existing branch that constrains wildcards when the flag is true (constrainWildcardToSupertype) and maintain the Verify on localVariableType.
🤖 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/ConstraintSolverImpl.java`:
- Around line 163-168: In visitWildcardType(WildcardType subtype, Type
supertype) ensure the disabled-flag path preserves the original delegation: when
config.handleWildcardGenerics() is false, call visitType(subtype, supertype)
instead of simply returning null so wildcard subtype arguments continue to
follow the generic visitor path (preserving behavior used by
equateTypeArguments); keep the existing branch that constrains wildcards when
the flag is true (constrainWildcardToSupertype) and maintain the Verify on
localVariableType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea3386c2-b00e-4ab5-bfd8-dce0ce7b9a64
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
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/ConstraintSolverImpl.java`:
- Around line 392-397: The change admits captured TypeVar instances into the
solver but they lack wildcard-aware nullability handling; revert to excluding
captures until handled properly by restoring the !tv.isCaptured() guard in
isTypeVariable(Type) OR, if you prefer to accept captures now, derive their
nullability from the originating wildcard using GenericsUtils.asWildcard(...)
when computing upperBoundIsNullable and in constrainTypeArgumentContainment()
(and ensure behavior respects handleWildcardGenerics flag) so that captures from
? super `@Nullable` T are treated nullable rather than using the raw upper bound;
update references to tv.isCaptured(), isTypeVariable, upperBoundIsNullable, and
constrainTypeArgumentContainment accordingly.
🪄 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: 57def0bb-f64d-4872-afb1-6ce58d31babe
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1)
390-395:⚠️ Potential issue | 🟠 MajorCaptured
? super@nullable...variables are still modeled as non-nullable solver vars.Removing the capture filter in
isTypeVariable()admits captured wildcards into the solver, butnullableAllowedis still derived from the capture's raw upper bound. For a capture of? super@nullableFoo, that upper bound is typically non-null, so a valid inference path can now fail withUnsatisfiableConstraintsExceptionwhen nullability propagates onto the capture. This regresses behavior even withhandleWildcardGenerics()off.Either restore the old
!tv.isCaptured()guard for now, or derive capture nullability from the source wildcard (for example viaGenericsUtils.asWildcard(...)) instead of only from the raw upper bound.Also applies to: 411-439
🤖 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/ConstraintSolverImpl.java` around lines 390 - 395, The bug is that captured wildcards are being admitted as solver type variables without preserving their source nullability, causing e.g. ? super `@Nullable` Foo to be treated non-nullable and leading to UnsatisfiableConstraintsException; fix by either restoring the previous capture guard in isTypeVariable (i.e., short-circuit with tv.isCaptured()) or, if you want captures handled, derive the capture's nullableAllowed from the original wildcard (use GenericsUtils.asWildcard(...) or similar to inspect the source wildcard annotations) rather than the raw upper bound; ensure logic in isTypeVariable and any code computing nullableAllowed (referencing tv, isTypeVariable, nullableAllowed, handleWildcardGenerics, and UnsatisfiableConstraintsException) consistently respects capture source nullability.
🤖 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/ConstraintSolverImpl.java`:
- Around line 390-395: The bug is that captured wildcards are being admitted as
solver type variables without preserving their source nullability, causing e.g.
? super `@Nullable` Foo to be treated non-nullable and leading to
UnsatisfiableConstraintsException; fix by either restoring the previous capture
guard in isTypeVariable (i.e., short-circuit with tv.isCaptured()) or, if you
want captures handled, derive the capture's nullableAllowed from the original
wildcard (use GenericsUtils.asWildcard(...) or similar to inspect the source
wildcard annotations) rather than the raw upper bound; ensure logic in
isTypeVariable and any code computing nullableAllowed (referencing tv,
isTypeVariable, nullableAllowed, handleWildcardGenerics, and
UnsatisfiableConstraintsException) consistently respects capture source
nullability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be0067ae-94fe-4d36-8d59-0f00789e17de
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
msridhar
force-pushed
the
wildcard-inference-experiment
branch
from
ea94de5 to
2f392ee
Compare
This reverts commit 2dcd522.
msridhar
force-pushed
the
wildcard-inference-experiment
branch
from
2f392ee to
adb858c
Compare
|
This change is part of the following stack: Change managed by git-spice. |
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
Mostly looks good to me, but there is some code duplication, I think
| upperBound = | ||
| wildcardType.bound == null | ||
| ? Symtab.instance(state.context).objectType | ||
| : wildcardType.bound.getUpperBound(); |
There was a problem hiding this comment.
Does this API not work with wildcardType.kind == BoundKind.EXTENDS bounded wildcards too? I thought upper bound was defined to include the type on the RHS of the extends, but not super certain about the API here.
There was a problem hiding this comment.
There are some docs from the previous version in CheckIdenticalNullabilityVisitor.java that seem lost here, but even those don't explain if wildcardType.bound.getUpperBound() would produce something different for ? extends ....
There was a problem hiding this comment.
I've added some comments to explain here. The confusing thing is that bound holds the type variable to which the wildcard is being passed, not a bound on the wildcard type...
| * {@code S <: supertypeTypeArg}. For {@code ? super S}, use the lower bound and require {@code | ||
| * S <: supertypeTypeArg}. | ||
| */ | ||
| private void constrainWildcardContainedByConcrete( |
There was a problem hiding this comment.
This method and constrainWildcardToSupertype are identical, modulo the renaming of their second argument. If we don't expect this to change dramatically in the near future, they should probably be the same method?
There was a problem hiding this comment.
Good catch! I kept constrainWildcardToSupertype
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
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/TypeSubstitutionUtils.java`:
- Around line 296-304: The current handling of Type.WildcardType in
TypeSubstitutionUtils ignores BoundKind.UNBOUND (plain '?'); update the wildcard
branch to explicitly handle BoundKind.UNBOUND by extracting the formal type
parameter's upper bound (as GenericsUtils.wildcardUpperBound(wt) or
wt.bound.getUpperBound()) and then call visit(t, thatUpperBound) just like the
EXTENDS/SUPER cases; modify the code around the Type.WildcardType branch to add
an else-if for wt.kind == BoundKind.UNBOUND that delegates to visit(t,
<upperBound>) so UNBOUND wildcards restore nullability annotations consistently.
🪄 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: 5a428160-4866-457c-859a-1367261bace8
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
| if (other instanceof Type.WildcardType wt) { | ||
| // When the other type is a wildcard, restore nullability annotations from the bound that | ||
| // determines the functional interface type. | ||
| if (wt.kind == BoundKind.EXTENDS) { | ||
| return visit(t, wt.getExtendsBound()); | ||
| } else if (wt.kind == BoundKind.SUPER) { | ||
| return visit(t, wt.getSuperBound()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are test cases covering UNBOUND wildcards flowing through this visitor
rg -n --type=java 'visitClassType.*Wildcard|UNBOUND' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java
rg -n --type=java '\<\?\>' nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java | head -20Repository: uber/NullAway
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the TypeSubstitutionUtils.java file to see the full context around lines 296-304
wc -l nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Read the relevant section of TypeSubstitutionUtils.java
sed -n '290,315p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 1129
🏁 Script executed:
#!/bin/bash
# Find GenericsUtils.java and check the wildcardUpperBound method
fd -n GenericsUtils.java nullaway/src/main/javaRepository: uber/NullAway
Length of output: 288
🏁 Script executed:
#!/bin/bash
# Search for UNBOUND keyword in the codebase
rg -n "BoundKind.UNBOUND" nullaway/src/main/java/Repository: uber/NullAway
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Search for test cases related to wildcards
fd -n "WildcardTests.java" nullaway/src/test/javaRepository: uber/NullAway
Length of output: 288
🏁 Script executed:
#!/bin/bash
# Find GenericsUtils.java
find nullaway/src/main/java -name "GenericsUtils.java"Repository: uber/NullAway
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Find WildcardTests.java
find nullaway/src/test/java -name "WildcardTests.java"Repository: uber/NullAway
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Search for wildcardUpperBound method
rg -n "wildcardUpperBound" nullaway/src/main/java/Repository: uber/NullAway
Length of output: 1483
🏁 Script executed:
#!/bin/bash
# Search for how UNBOUND wildcards are handled in generics code
rg -n "BoundKind\|UNBOUND" nullaway/src/main/java/com/uber/nullaway/generics/ -A 2 -B 2Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Read the GenericsUtils.java file to understand wildcardUpperBound
cat -n nullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.java | head -80Repository: uber/NullAway
Length of output: 3711
🏁 Script executed:
#!/bin/bash
# Search for UNBOUND handling patterns
rg -n "UNBOUND" nullaway/src/main/java/com/uber/nullaway/generics/Repository: uber/NullAway
Length of output: 425
🏁 Script executed:
#!/bin/bash
# Check WildcardTests.java for test coverage of UNBOUND wildcards
cat -n nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java | head -100Repository: uber/NullAway
Length of output: 4631
🏁 Script executed:
#!/bin/bash
# Search for similar wildcard handling patterns in other visitors
rg -n "instanceof.*WildcardType" nullaway/src/main/java/com/uber/nullaway/generics/ -A 5 -B 1Repository: uber/NullAway
Length of output: 5547
🏁 Script executed:
#!/bin/bash
# Check if visitClassType is called with UNBOUND wildcards in practice
rg -n "visitClassType" nullaway/src/main/java/com/uber/nullaway/generics/ -B 2 -A 2Repository: uber/NullAway
Length of output: 3135
🏁 Script executed:
#!/bin/bash
# Read more of TypeSubstitutionUtils.java to understand the context and purpose
sed -n '260,320p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 2698
🏁 Script executed:
#!/bin/bash
# Search for test cases with unbounded wildcards (just "?")
rg -n '\?\>' nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.javaRepository: uber/NullAway
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check if there are any tests with super wildcards
rg -n '\? super' nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.javaRepository: uber/NullAway
Length of output: 2881
🏁 Script executed:
#!/bin/bash
# Search for any existing UNBOUND handling or comments about it
rg -n "UNBOUND" nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if bound field can be null for WildcardType
rg -n "wildcardType.bound|wt.bound" nullaway/src/main/java/com/uber/nullaway/generics/ -B 2 -A 2Repository: uber/NullAway
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Look at the visitWildcardType method in TypeSubstitutionUtils to see how it handles wildcards
sed -n '321,345p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 830
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about why UNBOUND is not handled
sed -n '290,310p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java | cat -nRepository: uber/NullAway
Length of output: 1046
🏁 Script executed:
#!/bin/bash
# Verify if wt.bound can be null and check the null handling in GenericsUtils pattern
rg -n "bound == null|bound != null" nullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.javaRepository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are any other visitors that handle UNBOUND explicitly
rg -n "BoundKind.UNBOUND" nullaway/src/main/java/com/uber/nullaway/generics/ -B 2 -A 2Repository: uber/NullAway
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Check if there are tests for unbounded wildcards by looking for patterns like Foo<?>
rg -n 'Foo<.*>' nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java | grep -v extends | grep -v super | head -20Repository: uber/NullAway
Length of output: 1458
🏁 Script executed:
#!/bin/bash
# Check if the code actually processes UNBOUND wildcards and what happens
rg -n "RestoreNullnessAnnotationsVisitor\|visitClassType" nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java -B 5 -A 15 | head -60Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Verify what updateDirectNullabilityAnnotationsForType does with WildcardType
rg -n "updateDirectNullabilityAnnotationsForType" nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java -A 5 | head -40Repository: uber/NullAway
Length of output: 1206
🏁 Script executed:
#!/bin/bash
# Read the updateDirectNullabilityAnnotationsForType method fully
sed -n '362,390p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 1261
🏁 Script executed:
#!/bin/bash
# Check the git history or comments around this code
git log --oneline -20 -- nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Look for any test that specifically checks UNBOUND wildcard behavior
rg -n "unboundedFoo\|Foo<\?>" nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java -B 3 -A 8Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check what the actual test expectations are for unbounded wildcards
sed -n '270,320p' nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.javaRepository: uber/NullAway
Length of output: 2626
🏁 Script executed:
#!/bin/bash
# Look at the context around line 275 test with unbounded foo
rg -n "unboundedFoo" nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java -B 5 -A 15Repository: uber/NullAway
Length of output: 1671
🏁 Script executed:
#!/bin/bash
# Check if bound field is populated for UNBOUND wildcards
rg -n "\.bound\s*=" nullaway/src/main/java/com/uber/nullaway/generics/ -B 2 -A 2Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the commit context for the RestoreNullnessAnnotationsVisitor code
git show 272c447 --statRepository: uber/NullAway
Length of output: 27203
🏁 Script executed:
#!/bin/bash
# Search for usages of RestoreNullnessAnnotationsVisitor to understand when it's invoked
rg -n "RestoreNullnessAnnotationsVisitor\|new RestoreNullnessAnnotationsVisitor" nullaway/src/main/java/ -B 2 -A 2Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there's a way for UNBOUND to be processed through the formal type parameter bound
rg -n "wt.bound\|wildcardType.bound" nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look at the broader context of how RestoreNullnessAnnotationsVisitor is used
rg -n "new RestoreNullnessAnnotationsVisitor\|RestoreNullnessAnnotationsVisitor(" nullaway/src/main/java/com/uber/nullaway/generics/ -B 3 -A 3Repository: uber/NullAway
Length of output: 254
🏁 Script executed:
#!/bin/bash
# Look at the broader context of how RestoreNullnessAnnotationsVisitor is used
rg -n "RestoreNullnessAnnotationsVisitor" nullaway/src/main/java/com/uber/nullaway/generics/ -B 3 -A 3Repository: uber/NullAway
Length of output: 2508
🏁 Script executed:
#!/bin/bash
# Check what happens in the UNBOUND case - does it fall through intentionally or by omission?
# Look for any comments or documentation about wildcard handling
sed -n '290,340p' nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javaRepository: uber/NullAway
Length of output: 1782
Handle UNBOUND wildcards by extracting the formal type parameter's upper bound.
The code explicitly extracts bounds for BoundKind.EXTENDS and BoundKind.SUPER, but BoundKind.UNBOUND (i.e., ?) falls through without extraction. This is inconsistent with GenericsUtils.wildcardUpperBound, which uses wt.bound.getUpperBound() for both SUPER and UNBOUND wildcards to access the formal type parameter's upper bound.
For UNBOUND wildcards, consider handling the case explicitly:
Suggested approach
if (other instanceof Type.WildcardType wt) {
// When the other type is a wildcard, restore nullability annotations from the bound that
// determines the functional interface type.
if (wt.kind == BoundKind.EXTENDS) {
return visit(t, wt.getExtendsBound());
} else if (wt.kind == BoundKind.SUPER) {
return visit(t, wt.getSuperBound());
+ } else if (wt.kind == BoundKind.UNBOUND && wt.bound != null) {
+ return visit(t, wt.bound.getUpperBound());
+ }
}🤖 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/TypeSubstitutionUtils.java`
around lines 296 - 304, The current handling of Type.WildcardType in
TypeSubstitutionUtils ignores BoundKind.UNBOUND (plain '?'); update the wildcard
branch to explicitly handle BoundKind.UNBOUND by extracting the formal type
parameter's upper bound (as GenericsUtils.wildcardUpperBound(wt) or
wt.bound.getUpperBound()) and then call visit(t, thatUpperBound) just like the
EXTENDS/SUPER cases; modify the code around the Type.WildcardType branch to add
an else-if for wt.kind == BoundKind.UNBOUND that delegates to visit(t,
<upperBound>) so UNBOUND wildcards restore nullability annotations consistently.
There was a problem hiding this comment.
There cannot be explicit nullability annotations for the unbound case
There was a problem hiding this comment.
@msridhar, thanks for the clarification! That makes sense — an unbound wildcard ? carries no explicit nullability annotation, so there's nothing to restore for that case, and the fall-through to updateDirectNullabilityAnnotationsForType is intentionally correct. I was wrong to suggest handling BoundKind.UNBOUND there.
✏️ Learnings added
Learnt from: msridhar
Repo: uber/NullAway PR: 1549
File: nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java:296-304
Timestamp: 2026-05-03T17:32:46.673Z
Learning: In uber/NullAway, UNBOUND wildcards (`?`) cannot carry explicit nullability annotations (you cannot write `? Nullable`). Therefore, in `RestoreNullnessAnnotationsVisitor.visitClassType` in `TypeSubstitutionUtils.java`, it is correct and intentional for the `BoundKind.UNBOUND` case to fall through without extracting a bound — there is nothing to restore. Only `BoundKind.EXTENDS` and `BoundKind.SUPER` need explicit bound extraction for nullability annotation restoration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: msridhar
Repo: uber/NullAway PR: 1485
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:2119-2128
Timestamp: 2026-03-20T15:34:10.657Z
Learning: In this repo (uber/NullAway), don’t annotate local variables with `Nullable` or `NonNull`. NullAway infers nullability for local variables automatically, so adding these annotations to locals is incorrect style for this codebase.
2 participants
This is an initial (incomplete) implementation of extending our generic method inference to handle wildcard types. It has known bugs and incompleteness (#1522, #1500, #1551, #1552), but we'll handle those in follow-up PRs to keep this one at a reasonable size. This support is also behind a flag and off by default.
The key changes here are in
ConstraintSolverImpl. When constraining one class typeCto be a subtype of another class typeS, we now constrain their type arguments using "containment" (JLS 4.5.1 + the JSpecify spec) for wildcard type arguments. Containment has various cases depending on whether we haveextendsorsuperbounds. Also, when one type is constraint to be a subtype of a wildcard type, we use the wildcard's bound to generate the constraint.Beyond the above:
GenericsUtilsTypeSubstitutionUtilsto handle bothextendsandsuperboundsSee the new test cases in
WildcardTests, including some that are ignored with follow-up issues. Beyond addressing those follow ups, I plan to also go through any new reports from this support in our integration test subjects (junit, spring-framework, caffeine) and check their validity in order to make the inference more robust.Summary by CodeRabbit
New Features
Tests