Support adding nested nullability annotations in library models #1407
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1407 +/- ##
============================================
+ Coverage 88.21% 88.35% +0.13%
- Complexity 2615 2662 +47
============================================
Files 95 98 +3
Lines 8672 8895 +223
Branches 1741 1765 +24
============================================
+ Hits 7650 7859 +209
- Misses 509 514 +5
- Partials 513 522 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds support for nested nullability annotations in library models and wires that data through analysis. Introduces NestedAnnotationInfo and AddAnnotationToNestedTypeVisitor, extends LibraryModels with nestedAnnotationsForMethods(), updates Combined/Optimized/Default library-model implementations to aggregate per-method nested annotation maps, and adds a handler hook onOverrideMethodType to apply nested annotations to MethodType arguments and returns. Refactors generics/type-restoration paths: GenericsChecks now uses handler-provided override-aware MethodType, TypeSubstitutionUtils adopts a direct update flow for restoring annotations, and CompositeHandler and Handler gain the onOverrideMethodType hook. New tests and a test-library-models module are added; build dependency updated. 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)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
🧰 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.
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.
📚 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/generics/GenericsChecks.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.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). (4)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (5)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (5)
488-492: LGTM! Override-aware method type resolution applied correctly.The invocation now uses
handler.onOverrideMethodType()to obtain a potentially modified method type before restoring nullability annotations. This correctly enables library models to inject nested annotations on method return types.
779-787: LGTM! Constraint generation consistently uses override-aware method type.The method now obtains the method type via
handler.onOverrideMethodType()and uses it consistently for:
- Return type constraint generation (line 784)
- InvocationArguments construction (line 787)
This ensures library model annotations are properly incorporated during generic method inference.
1317-1319: LGTM! Parameter nullability comparison uses override-aware method type.The method type is now obtained via
handler.onOverrideMethodType()before constructing InvocationArguments, ensuring parameter type checks account for library model annotations.
2090-2090: LGTM! Public API exposure for synthetic annotation type accessor.The visibility change from private to public static is intentional per the PR summary, enabling external components (like handlers) to access the synthetic
@Nullableannotation type for creating annotated types programmatically.
2102-2122: LGTM! New public accessor for synthetic @nonnull annotation type.The new method mirrors
getSyntheticNullableAnnotTypeand correctly creates a synthetic@NonNullannotation type. This complements the nullable variant and supports the library models feature for nested annotations.Note: The thread-safety concern for lazy initialization applies here as well (see comment on lines 2074-2075).
| private static @Nullable Type syntheticNullableAnnotType; | ||
| private static @Nullable Type syntheticNonNullAnnotType; |
There was a problem hiding this comment.
Add synchronization for thread-safe lazy initialization.
The static fields are lazily initialized without synchronization in their respective getter methods (lines 2091-2099 and 2113-2121). In multi-threaded compilation scenarios, this can cause race conditions where multiple ErrorType instances are created or visibility issues occur.
🔎 Suggested fix using double-checked locking
- private static @Nullable Type syntheticNullableAnnotType;
- private static @Nullable Type syntheticNonNullAnnotType;
+ private static volatile @Nullable Type syntheticNullableAnnotType;
+ private static volatile @Nullable Type syntheticNonNullAnnotType;Then update the getter methods to use double-checked locking:
For getSyntheticNullableAnnotType:
public static Type getSyntheticNullableAnnotType(VisitorState state) {
if (syntheticNullableAnnotType == null) {
- Names names = Names.instance(state.context);
- Symtab symtab = Symtab.instance(state.context);
- Name name = names.fromString("nullaway.synthetic");
- Symbol.PackageSymbol packageSymbol = new Symbol.PackageSymbol(name, symtab.noSymbol);
- Name simpleName = names.fromString("Nullable");
- syntheticNullableAnnotType = new Type.ErrorType(simpleName, packageSymbol, Type.noType);
+ synchronized (GenericsChecks.class) {
+ if (syntheticNullableAnnotType == null) {
+ Names names = Names.instance(state.context);
+ Symtab symtab = Symtab.instance(state.context);
+ Name name = names.fromString("nullaway.synthetic");
+ Symbol.PackageSymbol packageSymbol = new Symbol.PackageSymbol(name, symtab.noSymbol);
+ Name simpleName = names.fromString("Nullable");
+ syntheticNullableAnnotType = new Type.ErrorType(simpleName, packageSymbol, Type.noType);
+ }
+ }
}
return syntheticNullableAnnotType;
}Apply the same pattern to getSyntheticNonNullAnnotType (lines 2113-2121).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (lines
2074-2075), the two static lazy-initialized fields must be made thread-safe:
change both declarations to private static volatile Type
syntheticNullableAnnotType and private static volatile Type
syntheticNonNullAnnotType, and update their getters (around lines 2091-2099 and
2113-2121) to use double-checked locking — first check for null, then
synchronize on GenericsChecks.class, re-check null inside the block and
initialize if still null, then return the field.
Fixes #1157 Leverages recent new functionality in `LibraryModels` (#1407, #1345) to properly support inference for calls to the `AtomicReferenceFieldUpdater::newUpdater` generic method. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced null-safety analysis for AtomicReferenceFieldUpdater to properly support nullable type variable upper bounds and nested annotations for generic method parameters. * **Tests** * Added test coverage for AtomicReferenceFieldUpdater functionality within JDK library integration and null-marked code scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR adds support for modeling nested type annotations via library models, to support cases like #1157. Specifically we make the following changes:
LibraryModels::nestedAnnotationsForMethodsto specify nested annotations, with corresponding data types likeNestedAnnotationInfoandTypePathEntry. Also add support inCombinedLibraryModelsandOptimizedLibraryModels.Handler:: onOverrideMethodTypeto allow handlers to override method types, and invoke it from the appropriate places inGenericsChecks.LibraryModelsHandler:: onOverrideMethodTypeto insert annotations specified by the library models. Beyond that method, the primary logic is in the new visitorAddAnnotationToNestedTypeVisitor.RestoreNullnessAnnotationsVisitorto restore annotations from class types and array types (previously it would only do type variables).Tests are in
CustomLibraryModelsTests, based on the code inNestedAnnotsand the sample library models for that code inTestLibraryModels.There are other minor related changes. Apologies for the size of this PR.
Once this lands, we can work on parsing this info out of jspecify/jdk and adding to astubx for our JDK models.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.