-
Notifications
You must be signed in to change notification settings - Fork 331
Conversation
We missed this optimization when first adding this feature
Summary by CodeRabbit
- Refactor
- Improved null-safety analysis: the generic method-type-variable handling now uses additional analysis context, enabling more accurate detection of nullable upper bounds for overridden methods and reducing incorrect diagnostics when validating generic method type arguments.
Tip: You can customize this high-level summary in your review settings.
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 touchesFailed 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 usedLearnings (1)Common learningsLearnt from: msridhar
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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} upper
Committable suggestion skipped: line range outside the PR's diff.
Prompt for AI Agents
In nullaway/src/main/java/com/uber/nullaway/handlers/Handler.ja va 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:
|