Use OptimizedLibraryModels for method type variable upper bounds#1388
Use OptimizedLibraryModels for method type variable upper bounds#1388
Conversation
WalkthroughThe PR changes the signature of onOverrideMethodTypeVariableUpperBound to add a VisitorState parameter across the handlers framework. The signature is updated from (Symbol.MethodSymbol methodSymbol, int typeVarIndex) to (Symbol.MethodSymbol methodSymbol, int typeVarIndex, VisitorState state) in Handler, CompositeHandler, LibraryModelsHandler, and call sites in GenericsChecks and ConstraintSolverImpl are adjusted. LibraryModelsHandler now uses the supplied VisitorState to access optimized library models. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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). (4)
🔇 Additional comments (2)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
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/handlers/CompositeHandler.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java(1 hunks)nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2025-08-28T04:54:20.953Z
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.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
📚 Learning: 2025-10-29T23:56:18.236Z
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.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
⏰ 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). (5)
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on ubuntu-latest
🔇 Additional comments (6)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java (1)
311-314: LGTM!The call site is correctly updated to pass the
stateparameter, which is already available as an instance field. This aligns with the updatedHandlerinterface signature.nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (3)
364-370: LGTM! Optimization correctly applied.The implementation now uses
OptimizedLibraryModelsfor method type variable upper bounds lookup, which provides efficient symbol-based lookup using theNameIndexedMapstructure. This is consistent with how other library model lookups are optimized in this handler.
1264-1264: LGTM!The new field
methodTypeVariablesWithNullableUpperBoundsand its initialization correctly follow the established pattern for other optimized lookups inOptimizedLibraryModels.Also applies to: 1279-1281
1319-1322: LGTM!The new lookup method follows the same pattern as other methods like
castToNonNullMethodandfailIfNullParameters, correctly delegating tolookupImmutableSet.nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)
264-266: LGTM!The call site is correctly updated to pass the
stateparameter, enabling the optimized library models lookup for method type variable upper bounds.nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java (1)
328-338: LGTM!The composite handler correctly propagates the new
stateparameter to each handler in the chain, maintaining the existing short-circuit behavior that returnstrueas soon as any handler signals that the type variable has a nullable upper bound.
| default boolean onOverrideMethodTypeVariableUpperBound( | ||
| Symbol.MethodSymbol methodSymbol, int index) { | ||
| Symbol.MethodSymbol methodSymbol, int index, VisitorState state) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing Javadoc for new state parameter.
The @param state documentation is missing from the method's Javadoc comment. Please add documentation for this parameter to maintain consistency with the existing Javadoc.
🔎 Proposed Javadoc update
Add the following to the Javadoc block above the method:
* @param methodSymbol The method symbol
* @param index index of the generic type variable (starting at 0)
+ * @param state The current visitor state
* @return boolean true if the variable should be treated as having a {@code @Nullable} upperCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java around lines
460 to 463, the Javadoc for onOverrideMethodTypeVariableUpperBound is missing
documentation for the new state parameter; update the Javadoc block above this
method to add a @param state entry (e.g., "@param state the VisitorState
providing context for the current AST/visitor traversal and utilities such as
type/symbol resolution") so the method documentation is consistent with existing
Javadoc.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1388 +/- ##
=========================================
Coverage 88.19% 88.19%
Complexity 2616 2616
=========================================
Files 95 95
Lines 8672 8675 +3
Branches 1743 1743
=========================================
+ Hits 7648 7651 +3
Misses 510 510
Partials 514 514 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
We missed this optimization when first adding this feature
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.