Include nested annotation information in astubx files when loading external library models#1456
Conversation
Co-authored-by: Manu Sridharan <msridhar@gmail.com>
…reateNestedAnnotationInfoVisitor.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>
…reateNestedAnnotationInfoVisitor.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>
…ullnessAnnotationSerializer.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>
…ullnessAnnotationSerializer.java Co-authored-by: Manu Sridharan <msridhar@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1456 +/- ##
============================================
+ Coverage 88.41% 88.48% +0.07%
- Complexity 2715 2723 +8
============================================
Files 99 99
Lines 9022 9077 +55
Branches 1805 1812 +7
============================================
+ Hits 7977 8032 +55
- Misses 517 518 +1
+ Partials 528 527 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jar-infer/jar-infer-lib/build.gradle
Outdated
| api libs.commons.io | ||
| compileOnly libs.error.prone.check.api | ||
| implementation project(":library-model:library-model-generator") | ||
| compileOnly project(':jdk-javac-plugin') |
There was a problem hiding this comment.
I missed reverting that change when moving the NestedAnnotationInfo class. I removed it.
| configurations = [ | ||
| project.configurations.runtimeClasspath | ||
| ] | ||
| from sourceSets.main.output |
There was a problem hiding this comment.
This is also a missed part of reverting my changes.
WalkthroughThis PR adds end-to-end support for nested type annotations. It introduces a new ImmutableSetMultimap<Integer, NestedAnnotationInfo> field on MethodAnnotationsRecord, wires nested annotation construction through the astubx generator and javac plugin, persists nested annotation data in stubx writing/parsing and the Stubx cache, and exposes nested annotations via ExternalStubxLibraryModels.nestedAnnotationsForMethods(). Tests and test fixtures were updated to cover nested annotations on type arguments, wildcards, arrays, and mixed generics. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java (1)
126-180:⚠️ Potential issue | 🟠 Majorastubx format changed without a version bump.
The nested-annotation block is inserted before NullMarked classes but the magic number remains VERSION_1. Older readers will misparse new files and new readers will misparse old files. Please bump the magic/version and gate parsing by version (or handle the missing section) to maintain backward compatibility.
🤖 Fix all issues with AI agents
In `@nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java`:
- Around line 80-82: The nestedAnnotationInfoCache field in StubxCacheUtil is
currently declared as a SetMultimap<String, SetMultimap<Integer,
NestedAnnotationInfo>>, which places a mutable SetMultimap inside a HashMultimap
value and can corrupt the set when mutated; change the field to Map<String,
SetMultimap<Integer, NestedAnnotationInfo>> (and likewise replace other similar
SetMultimap-of-SetMultimap fields/uses noted around lines 96-97, 113-116,
224-234) and update insertion/access sites to use computeIfAbsent() to create
and retrieve the inner SetMultimap (e.g., in methods that currently put or
mutate nestedAnnotationInfoCache) so you never store a mutable collection as a
value inside a HashMultimap. Ensure all call sites that assumed SetMultimap
behavior still operate on the inner SetMultimap retrieved from the Map.
nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java
Outdated
Show resolved
Hide resolved
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Looking pretty good! I have a few comments.
| ImmutableList.of( | ||
| new TypePathEntry(Kind.ARRAY_ELEMENT, -1), | ||
| new TypePathEntry(Kind.TYPE_ARGUMENT, 0))))), | ||
| "ParameterizedTypeArray:java.util.List<? extends java.lang.String> wildcardIdentity(java.util.List<? extends java.lang.String>)", |
There was a problem hiding this comment.
Maybe:
| "ParameterizedTypeArray:java.util.List<? extends java.lang.String> wildcardIdentity(java.util.List<? extends java.lang.String>)", | |
| "ParameterizedTypeArray:java.util.List<? extends java.lang.String> wildcardIdentity(java.util.List<? super java.lang.String>)", |
To test a super bound?
...otations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java
Show resolved
Hide resolved
...ations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java
Show resolved
Hide resolved
| } | ||
|
|
||
| public static List<@Nullable String> nestedAnnotTypeArg() { | ||
| List<String> list = new ArrayList<>(); |
There was a problem hiding this comment.
You don't need this code here, but if you're going to write it, this should be:
| List<String> list = new ArrayList<>(); | |
| List<@Nullable String> list = new ArrayList<>(); |
| } | ||
|
|
||
| public static @Nullable String[] nestedAnnotArrayElement() { | ||
| return new String[] {"populated", "value", null}; |
There was a problem hiding this comment.
If you want to keep the code:
| return new String[] {"populated", "value", null}; | |
| return new @Nullable String[] {"populated", "value", null}; |
| @@ -12,7 +11,6 @@ | |||
| * outermost type. Otherwise, each entry indicates one step in how to navigate to the nested | |||
| * type. | |||
| */ | |||
| @NullMarked | |||
There was a problem hiding this comment.
I have added it back.
| for (NestedAnnotationInfo.TypePathEntry.Kind kind : | ||
| NestedAnnotationInfo.TypePathEntry.Kind.values()) { | ||
| if (encodingDictionary.containsKey(kind.name())) { | ||
| continue; | ||
| } | ||
| strings.add(kind.name()); | ||
| encodingDictionary.put(kind.name(), numStringEntries); | ||
| ++numStringEntries; | ||
| } | ||
| for (NestedAnnotationInfo.Annotation annotation : NestedAnnotationInfo.Annotation.values()) { | ||
| if (encodingDictionary.containsKey(annotation.name())) { | ||
| continue; | ||
| } | ||
| strings.add(annotation.name()); | ||
| encodingDictionary.put(annotation.name(), numStringEntries); | ||
| ++numStringEntries; | ||
| } |
There was a problem hiding this comment.
I think you can delete both these loops by adding the result of these values() calls to the keysets variable above. To convert to a Collection you can do List.of([...].values()).
nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java
Outdated
Show resolved
Hide resolved
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 (1)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
63-64:⚠️ Potential issue | 🔴 CriticalSame import path issue as StubxCacheUtil.
The imports use
com.uber.nullaway.librarymodelbutNestedAnnotationInfois defined incom.uber.nullaway.libmodel.🐛 Proposed fix for the import path
-import com.uber.nullaway.librarymodel.AddAnnotationToNestedTypeVisitor; -import com.uber.nullaway.librarymodel.NestedAnnotationInfo; -import com.uber.nullaway.librarymodel.NestedAnnotationInfo.Annotation; +import com.uber.nullaway.libmodel.AddAnnotationToNestedTypeVisitor; +import com.uber.nullaway.libmodel.NestedAnnotationInfo; +import com.uber.nullaway.libmodel.NestedAnnotationInfo.Annotation;
🤖 Fix all issues with AI agents
In `@library-model/library-model-generator/build.gradle`:
- Around line 21-24: The jspecify dependency is leaked in the public API because
the public record NestedAnnotationInfo is annotated with `@NullMarked`; change the
dependency scope from implementation to api so consumers see jspecify
transitively. Update the dependencies block where libs.jspecify is declared (the
dependency line referencing jspecify) to use the api configuration instead of
implementation so the public annotation type is available to downstream modules.
In `@nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java`:
- Around line 30-33: Update the imports in StubxCacheUtil to reference the
correct package com.uber.nullaway.libmodel instead of
com.uber.nullaway.librarymodel: replace imports for NestedAnnotationInfo and its
nested types (NestedAnnotationInfo, NestedAnnotationInfo.Annotation,
NestedAnnotationInfo.TypePathEntry, NestedAnnotationInfo.TypePathEntry.Kind) so
the class StubxCacheUtil uses com.uber.nullaway.libmodel.NestedAnnotationInfo
and the corresponding nested-type imports.
2 participants
Fixes #1410
This pull request includes nested annotation information when loading external libraries.
First, the
astubx-generatorproject is updated to include nested annotation information to astubx.Second, the
StubxCacheUtilclass is updated so that it will read the newly added information and store it.Finally, the
LibraryModelsHandlerclass will now load the nested annotation information from theStubxCacheUtiland add it to external library models.This is the final step from #1410.
The unit tests for this pull request are included in the following test classes.
jdk-annotations/astubx-generator/src/test/java/com/uber/nullaway/jdkannotations/AstubxTest.javajdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.javaSummary by CodeRabbit
New Features
Public API
Tests