Account for annotations in extends / implements when computing view as supertype#1266
Conversation
WalkthroughThreads Config into generics nullability logic, replaces direct Types.asSuper(...) calls with TypeSubstitutionUtils.asSuper(..., config), adds utilities to restore explicit nullability on type-variable substitutions, exposes APIs to compute type-var annotations along inheritance, removes an obsolete inference visitor, and updates/adds tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GC as GenericsChecks
participant CIV as CheckIdenticalNullabilityVisitor
participant TSU as TypeSubstitutionUtils
participant CDU as ClassDeclarationNullnessAnnotUtils
participant JT as Javac Types
GC->>CIV: new CheckIdenticalNullabilityVisitor(state,this,config)
CIV->>TSU: asSuper(types, rhsType, lhsClassSym, config)
TSU->>JT: types.asSuper(subtype, superTypeSymbol)
JT-->>TSU: asSuperResult or null
alt asSuperResult non-null
TSU->>CDU: getAnnotsOnTypeVarsFromSubtypes(subtype, superTypeSymbol, types, config)
CDU-->>TSU: annotsMap
TSU-->>CIV: restoredSupertype (asSuperResult with explicit nullability)
else asSuperResult null/raw
TSU-->>CIV: null/raw (no restoration)
end
CIV-->>GC: continue checks using restoredSupertype
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1266 +/- ##
============================================
+ Coverage 88.12% 88.55% +0.43%
- Complexity 2426 2431 +5
============================================
Files 92 91 -1
Lines 8033 8013 -20
Branches 1600 1598 -2
============================================
+ Hits 7079 7096 +17
+ Misses 494 458 -36
+ Partials 460 459 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
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/TypeSubstitutionUtils.java (1)
206-211: Avoid unsafe cast from AnnotationMirror to Attribute.TypeCompoundThis cast can CCE if the value isn’t a type-use annotation (rare but possible). Guard it.
- Attribute.TypeCompound typeArgAnnot = - (Attribute.TypeCompound) annotsOnTypeVarsFromSubtypes.get(other.tsym); - if (typeArgAnnot != null) { - return typeWithAnnot(t, typeArgAnnot); - } + AnnotationMirror maybeAnnot = annotsOnTypeVarsFromSubtypes.get(other.tsym); + if (maybeAnnot instanceof Attribute.TypeCompound) { + return typeWithAnnot(t, (Attribute.TypeCompound) maybeAnnot); + }nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (1)
82-83: Compare enclosing types after aligning RHS base: use rhsTypeAsSuperUsing rhsType.getEnclosingType() can mismatch when RHS base was adjusted via asSuper.
- return lhsType.getEnclosingType().accept(this, rhsType.getEnclosingType()); + return lhsType.getEnclosingType().accept(this, rhsTypeAsSuper.getEnclosingType());
🧹 Nitpick comments (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
1984-2008: Add order-variant of intersection type to harden coverageIntersection types are unordered semantically, but parser/LUB differences can surface. Consider also testing (Serializable & A<...>) to guard against order-sensitive regressions.
Apply this diff to extend the test body:
static void test1(Object o) { var x = (A<String> & Serializable) o; // BUG: Diagnostic contains: Cannot assign from type B to type A<String> & Serializable x = new B(); // ok x = new C(); + var y = (Serializable & A<String>) o; + // BUG: Diagnostic contains: Cannot assign from type B to type Serializable & A<String> + y = new B(); + // ok + y = new C(); } static void test2(Object o) { var x = (A<@Nullable String> & Serializable) o; x = new B(); // BUG: Diagnostic contains: Cannot assign from type C to type A<@Nullable String> & Serializable x = new C(); + var y = (Serializable & A<@Nullable String>) o; + y = new B(); + // BUG: Diagnostic contains: Cannot assign from type C to type Serializable & A<@Nullable String> + y = new C(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java(3 hunks)nullaway/src/main/java/com/uber/nullaway/generics/ClassDeclarationNullnessAnnotUtils.java(2 hunks)nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.922Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
📚 Learning: 2025-08-28T04:54:20.922Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.922Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.javanullaway/src/main/java/com/uber/nullaway/generics/ClassDeclarationNullnessAnnotUtils.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java
🧬 Code graph analysis (3)
nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
TypeSubstitutionUtils(22-287)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
TypeSubstitutionUtils(22-287)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
TypeSubstitutionUtils(22-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: JDK 17 on windows-latest with Error Prone 2.41.0
🔇 Additional comments (12)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
1984-2008: Unified intersection-type test across JDKs — LGTMConsolidation to a single expectation looks correct and matches the PR intent.
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (2)
103-105: Switch to TypeSubstitutionUtils.asSuper preserves nullability — LGTMThis will restore explicit nullability from extends/implements in the supertype view and aligns with the PR’s goal.
103-105: No remaining direct Types.asSuper calls
All.asSuperinvocations now use theTypeSubstitutionUtils.asSuperwrapper; no further migration needed.nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.java (1)
34-36: Use of TypeSubstitutionUtils.asSuper here is correctKeeps assignment-context inference consistent with annotation restoration.
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (2)
717-717: Pass Config into CheckIdenticalNullabilityVisitor — LGTMConstructor threading of Config is appropriate given downstream asSuper/memberType usage.
717-717: All instantiations of CheckIdenticalNullabilityVisitor use the updated(state, this, config)signature; no mixed constructors remain.nullaway/src/test/java/com/uber/nullaway/jspecify/GenericInheritanceTests.java (1)
235-253: New interfaceInheritance test exercises extends-annotation restoration — LGTMCovers the reported Sub extends Base<@nullable T> scenario and validates the fix.
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (2)
43-44: Switch to TypeSubstitutionUtils.asSuper preserves nullability. LGTM
21-26: All call sites include the Config parameter Verified via ripgrep that the lone invocation in GenericsChecks.java uses all three arguments—no further updates needed.nullaway/src/main/java/com/uber/nullaway/generics/ClassDeclarationNullnessAnnotUtils.java (3)
48-66: New API for supertype-based annotation mapping. LGTM
106-107: Delegation to class-based path helper. LGTM
119-133: Path construction logic looks correct and immutable. LGTM
|
I've confirmed the Spring error on CI is valid: This is because Though the error is valid, it's also incomprehensible; the type parameters look the same! Let me think about if we can do something better. |
I've decided this is a bit involved and should be fixed in a follow up; filed #1268. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
24-52: Config-aware asSuper wrapper looks correct and addresses JDK<=21 behaviorGood: narrowed param to
Symbol.ClassSymbol, mirrorsmemberType(...)restoration path, and uses unsubstitutedsuperTypeSymbol.asType()as template. This also resolves the earlier review about unsafe casts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java(4 hunks)nullaway/src/main/java/com/uber/nullaway/generics/ClassDeclarationNullnessAnnotUtils.java(2 hunks)nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.java(2 hunks)nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
- nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
- nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java
- nullaway/src/main/java/com/uber/nullaway/generics/ClassDeclarationNullnessAnnotUtils.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.922Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
📚 Learning: 2025-08-28T04:54:20.922Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.922Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java
🧬 Code graph analysis (2)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
TypeSubstitutionUtils(22-287)
nullaway/src/main/java/com/uber/nullaway/generics/InferGenericMethodSubstitutionViaAssignmentContextVisitor.java (1)
nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java (1)
TypeSubstitutionUtils(22-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: JDK 11 on ubuntu-latest with Error Prone 2.31.0
- GitHub Check: JDK 17 on macos-latest with Error Prone 2.41.0
- GitHub Check: JDK 17 on ubuntu-latest with Error Prone 2.14.0
- GitHub Check: JDK 17 on ubuntu-latest with Error Prone 2.41.0
- GitHub Check: JDK 17 on windows-latest with Error Prone 2.41.0
- GitHub Check: JDK 11 on ubuntu-latest with Error Prone 2.14.0
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java (1)
22-27: LGTM: thread Config and use TypeSubstitutionUtils.asSuperConstructor change and call site updates are consistent; cast to
ClassSymbolis safe here sincelhsTypeis aType.ClassType. Enclosing-type comparison now correctly uses the super-view.Also applies to: 44-46, 85-85
lazaroclapp
left a comment
There was a problem hiding this comment.
I have one minor question, but either way it LGTM.
| // should also match. When there is no enclosing type, getEnclosingType() returns a NoType | ||
| // object, which gets handled by the fallback visitType() method | ||
| return lhsType.getEnclosingType().accept(this, rhsType.getEnclosingType()); | ||
| return lhsType.getEnclosingType().accept(this, rhsTypeAsSuper.getEnclosingType()); |
There was a problem hiding this comment.
Was this working before? What's the change in semantics between using rhsType vs rhsTypeAsSuper here to get the enclosing type? (Just curious)
There was a problem hiding this comment.
This is a very good question and I went down a bit of a rabbit hole trying to figure it out :-) The change was suggested by Code Rabbit and seemed harmless, but as I dug more I couldn't construct a test where it actually mattered. If we just use rhsType, then when comparing enclosing types, we should recursively call asSuper() and everything should work out. I undid the change but added a comment to this effect.
While I was digging into this I found #1274 and typetools/checker-framework#7239 🙂
Fixes #1252
In JDK 21 and earlier, the
Types.asSupermethod did not account for annotations present in extends / implements clauses. So, here we wrap calls toasSuperin extra logic to re-introduce these annotations. We already introduced the logic to fix the same issue withTypes.memberType.This fix also (unexpectedly) addresses an issue in another test which was misdiagnosed as #1022. #1022 indicated annotations were missing in cast types in earlier JDKs, but this was incorrect (that issue should be closed). In fact, the bug observed there (in test
intersectionTypeInvalidAssign) was in fact theasSuperbug fixed here. We misdiagnosed the problem partly because in some JDK version after JDK 21,Types.asSuperstarted behaving differently and preserving annotations in extends / implements clauses.As a related cleanup, delete the unused
InferGenericMethodSubstitutionViaAssignmentContextVisitorclass.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores