Handle most remaining wildcard subtyping / containment cases#1548
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1548 +/- ##
============================================
+ Coverage 88.49% 88.52% +0.02%
- Complexity 2859 2862 +3
============================================
Files 103 103
Lines 9525 9522 -3
Branches 1914 1911 -3
============================================
Hits 8429 8429
+ Misses 529 528 -1
+ Partials 567 565 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
changed the title
msridhar
force-pushed
the
wildcard-opposite-bounds
branch
from
627c9d2 to
ef71fef
Compare
WalkthroughThis pull request refactors wildcard containment logic in Possibly related PRs
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}
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/CheckIdenticalNullabilityVisitor.java`:
- Around line 197-203: Update the inline comment inside
CheckIdenticalNullabilityVisitor in the branch guarded by rhsWildcard.kind !=
BoundKind.SUPER to mention that this branch covers both `? extends T` and the
unbounded `?` rhs forms (not just `? extends T`); make the one-word addition to
the existing comment so it reads that the rhs could be an arbitrary subtype of T
(or unbounded `?`) to keep the rationale accurate for both shapes handled here.
- Around line 179-186: The method wildcardUpperBound dereferences
wildcardType.bound without a null check; add a defensive guard so null bounds
are handled like other code in this class — either return the symbol table
object type when bound is null (matching OpenJDK pattern) or wrap access with
the existing castToNonNull helper used elsewhere (e.g.,
castToNonNull(wildcardType.bound).getUpperBound()), ensuring you reference
wildcardUpperBound and wildcardType.bound when making the change.
🪄 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: d95989de-7603-4f0a-bcae-747b8f294c97
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
| private Type wildcardUpperBound(Type.WildcardType wildcardType) { | ||
| if (wildcardType.kind == BoundKind.EXTENDS) { | ||
| return wildcardType.getExtendsBound(); | ||
| } | ||
| // For ? and ? super L, javac stores the wildcard's corresponding type variable in the `bound` | ||
| // field. The upper bound of that type variable is the wildcard's effective upper bound. | ||
| return wildcardType.bound.getUpperBound(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing usages/handling of WildcardType.bound in the repo to see whether
# null guards are applied elsewhere when reading the corresponding TypeVar.
rg -nP --type=java -C3 '\bwildcard(Type)?\.bound\b|\bWildcardType\b.*\bbound\b'
rg -nP --type=java -C2 '\bgetExtendsBound\(\)|getSuperBound\(\)'Repository: uber/NullAway
Length of output: 8276
🌐 Web query:
javac com.sun.tools.javac.code.Type.WildcardType bound field nullable
💡 Result:
Yes, the bound field in com.sun.tools.javac.code.Type.WildcardType is nullable. Multiple OpenJDK source code snippets show explicit null checks like w.bound == null ? syms.objectType : w.bound.getUpperBound in Types.wildUpperBound(Type t) and similar in other methods. The Checker Framework documentation confirms: "This field is sometimes null, in that case this method will return null." An OpenJDK bug report (JDK-8043279) also accesses ((WildcardType) args.head).bound and handles oldBound == null, confirming nullability. This behavior is consistent across JDK versions from 7+ to recent (e.g., snippets from JDK 17, recent OpenJDK repos).
Citations:
- 1: https://github.com/openjdk-mirror/jdk7u-langtools/blob/master/src/share/classes/com/sun/tools/javac/code/Types.java
- 2: https://code.yawk.at/java/17/jdk.compiler/com/sun/tools/javac/code/Type.java
- 3: https://bugs.openjdk.org/browse/JDK-8043279
- 4: https://openjdk.github.io/cr-archive/valhalla/32/webrev.00/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java.html
- 5: https://checkerframework.org/api/org/checkerframework/javacutil/TypesUtils.html
- 6: https://github.com/openjdk/jdk/blob/3edc24a71d29632e0a2166a64fc25ce83f631b47/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
- 7: https://cr.openjdk.org/~briangoetz/JDK-8042275/webrev/src/share/classes/com/sun/tools/javac/code/Types.java.html
- 8: https://github.com/fiji/javac/blob/master/src/main/java/com/sun/tools/javac/code/Type.java
Add a null guard or confirm bound is always non-null for wildcards reaching this path.
Line 185 dereferences wildcardType.bound.getUpperBound() without null-checking bound. Javac's WildcardType.bound field is documented as nullable and OpenJDK source shows explicit null guards elsewhere (e.g., w.bound == null ? syms.objectType : w.bound.getUpperBound()). While the method comment claims javac stores bound for non-EXTENDS wildcards, this doesn't account for incomplete attribution during inference or capture conversion. Other parts of this class defensively use castToNonNull() for similar operations (lines 195, 204). Either confirm this code path never encounters null bound given the PR's scope, or apply a null guard.
🤖 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/CheckIdenticalNullabilityVisitor.java`
around lines 179 - 186, The method wildcardUpperBound dereferences
wildcardType.bound without a null check; add a defensive guard so null bounds
are handled like other code in this class — either return the symbol table
object type when bound is null (matching OpenJDK pattern) or wrap access with
the existing castToNonNull helper used elsewhere (e.g.,
castToNonNull(wildcardType.bound).getUpperBound()), ensuring you reference
wildcardUpperBound and wildcardType.bound when making the change.
There was a problem hiding this comment.
You're right about this one, but will fix in a follow up (#1549)
There was a problem hiding this comment.
@msridhar, understood — thanks for confirming! I'll look for the null-guard fix in #1549.
🧠 Learnings used
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1507
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java:66-79
Timestamp: 2026-04-01T15:40:51.141Z
Learning: In NullAway's `GenericTypePrettyPrintingVisitor` (nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java), the `appendNullableAnnotationIfPresent` helper intentionally prints ONLY `Nullable` type-use annotations and silently suppresses all other type-use annotations (including `NonNull`). This is by design: non-null is the default in NullAway's model and other type-use qualifiers are not relevant to NullAway diagnostics, so only `Nullable` is meaningful to display in error messages.
Learnt from: msridhar
Repo: uber/NullAway PR: 1518
File: nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java:135-141
Timestamp: 2026-04-11T17:13:58.256Z
Learning: In NullAway's `RestrictiveAnnotationHandler` (`nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java`), the handler only applies to null-unmarked (unannotated) code (`isAnnotated == false`) and its primary purpose is to enforce `NonNull` restrictive annotations. The `Nullable` branch exists for completeness but the handler does NOT need to propagate `Nullable` to varargs-array nullness (`setVarargsArrayNullness`), since that is not within the scope of restrictive annotation handling for unannotated code.
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T19:11:30.777Z
Learning: Applies to **/*.java : Do not add top-level `Nullable` annotations on local variables in Java code. NullAway infers the nullability of local variables and ignores these explicit annotations.
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.
| Foo<? extends @Nullable Object> fromNullableSuper = nullableSuperFoo; | ||
| Foo<? extends @Nullable Object> fromUnbounded = unboundedFoo; | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<? super String> cannot be converted to Test.Foo<? extends Object> | ||
| Foo<? extends Object> badFromNonnullSuper = nonnullSuperFoo; |
There was a problem hiding this comment.
Presumably:
Foo<? super Bar> nonnullSuperFoo = ...
Foo<? extends Baz> badFromNonnullSuper = nonnullSuperFoo;
Is never valid unless Baz is @Nullable Object, right? Because all we know about the type argument of nonnullSuperFoo is that it is a super type of Bar, up to an including @Nullable Object, and @Nullable Object can't extend anything but itself...
Is that correct?
There was a problem hiding this comment.
Yes, this is correct. Foo's type variable has a @Nullable upper bound for this case. So, when we write Foo<? super X>, the actual type argument may be @Nullable X or any supertype of that, which is not a subtype of @NonNull Object.
If I try to write:
Foo<? extends @Nullable String> another = nonnullSuperFoo;Then javac doesn't accept the code, since the type argument for nonnullSuperFoo could be Object, which is not a subtype of String. Here's the javac error message:
incompatible types: Test.Foo<capture#1 of ? super java.lang.String> cannot be converted to Test.Foo<? extends java.lang.String>
2 participants
We now handle unbounded wildcards (which have an implicit upper bound identical to the corresponding type variable), and cases where a
? super Swildcard is assigned to a? extends Twildcard. The remaining unhandled cases can only arise in the context of generic type variables and inference, which we will handle later.Also refactor the code a bit and document a bit better.
Summary by CodeRabbit
Refactor
Tests