Enable JSpecify JDK models (under a flag) - #1641
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds the Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 1681-1692: Rename the bundled resource from output.astubx to
nullaway-jspecify-jdk.astubx and update the JDK model loading block guarded by
isJSpecifyJDKEnabled to use
ExternalStubxLibraryModels.class.getResourceAsStream(...) instead of accessing
getClass().getClassLoader(). Update the missing-resource log, parseStubStream
call, and success log to reference the new resource name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74e7363e-d06a-4ecd-982c-2359bca75b98
📒 Files selected for processing (9)
nullaway/build.gradlenullaway/src/main/java/com/uber/nullaway/Config.javanullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.javanullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.javanullaway/src/main/java/com/uber/nullaway/LibraryModels.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/resources/output.astubxnullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1641 +/- ##
============================================
- Coverage 88.20% 88.16% -0.04%
- Complexity 3060 3065 +5
============================================
Files 105 105
Lines 10282 10302 +20
Branches 2074 2079 +5
============================================
+ Hits 9069 9083 +14
- Misses 572 576 +4
- Partials 641 643 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if (stubxInputStream == null) { | ||
| throw new RuntimeException("could not get input stream for " + astubxPath); | ||
| } |
There was a problem hiding this comment.
This change is due to the new JDK models (which model the return of getResourceAsStream as @Nullable)
| Matcher matcher = METHOD_SIG_PATTERN.matcher(methodSignature); | ||
| if (matcher.find()) { | ||
| String methodName = matcher.group(2); | ||
| String methodName = castToNonNull(matcher.group(2)); |
There was a problem hiding this comment.
This change is due to the new JDK models (which model the return of Matcher.group as @Nullable)
| ImmutableMap<MethodRef, ImmutableSetMultimap<Integer, NestedAnnotationInfo>> refs) { | ||
| return makeOptimizedLookup( | ||
| names, refs.keySet(), ref -> NullabilityUtil.castToNonNull(refs.get(ref))); | ||
| return makeOptimizedLookup(names, refs.keySet(), ref -> castToNonNull(refs.get(ref))); |
There was a problem hiding this comment.
This change is due to the new static import
|
@coderabbitai review |
✅ Action performedReview finished.
|
439fe21 to
d220b1a
Compare
12d4d1b to
5b7d5bc
Compare
a1a802f to
0f1d1b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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/StubxCacheUtil.java (1)
130-139: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose provider resource streams.
parseStubStream()does not closestubxInputStream; wrap the stream acquisition and parsing in try-with-resources to avoid retaining provider JAR resources.Proposed fix
- InputStream stubxInputStream = providerClass.getResourceAsStream(astubxPath); - if (stubxInputStream == null) { - throw new RuntimeException("could not get input stream for " + astubxPath); - } String stubxLocation = providerClass + ":" + astubxPath; - try { + try (InputStream stubxInputStream = providerClass.getResourceAsStream(astubxPath)) { + if (stubxInputStream == null) { + throw new RuntimeException("could not get input stream for " + astubxPath); + } parseStubStream(stubxInputStream, stubxLocation); LOG(DEBUG, "DEBUG", "loaded stubx file " + stubxLocation); } catch (IOException e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java` around lines 130 - 139, Update the stubx stream handling around providerClass.getResourceAsStream and parseStubStream in StubxCacheUtil to use try-with-resources, ensuring stubxInputStream is closed after parsing while preserving the existing null check, logging, and RuntimeException handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 1680-1694: Update the missing-resource branch in the JSpecify JDK
model loading block to throw an exception naming JSPECIFY_JDK_ASTUBX_FILENAME
instead of only calling astubxLoadLog. Preserve the existing parsing and
successful-load behavior when the resource is present.
- Around line 1658-1660: Update ExternalStubxLibraryModels so constructing
StubxCacheUtil does not eagerly discover JarInfer providers. Separate provider
discovery from cache construction, and perform the discovery only when
isJarInferEnabled is true, preserving JSpecify-only runs without external
JarInfer models.
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java`:
- Around line 14-32: Update modelsDisabledDoesNotLoadAstubxModel to construct
its CompilationTestHelper with JSpecifyJavacConfig.withJSpecifyModeArgs(...),
while omitting JSpecifyJDKModels from the configured models. Preserve the
existing AnnotatedPackages=foo setup and Attributes.get() assertion so the test
specifically exercises the disabled JDK-model path in JSpecify mode.
---
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java`:
- Around line 130-139: Update the stubx stream handling around
providerClass.getResourceAsStream and parseStubStream in StubxCacheUtil to use
try-with-resources, ensuring stubxInputStream is closed after parsing while
preserving the existing null check, logging, and RuntimeException handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f065239-1a9f-42c5-be6c-70b404bc5b19
📒 Files selected for processing (11)
jdk-javac-plugin/README.mdnullaway/build.gradlenullaway/src/main/java/com/uber/nullaway/Config.javanullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.javanullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.javanullaway/src/main/java/com/uber/nullaway/LibraryModels.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.javanullaway/src/main/resources/jspecify-jdk.astubxnullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.javanullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jdk-javac-plugin/README.md`:
- Around line 22-37: Update the README code block around the shell commands to
include blank lines before and after the fence, declare the fence language as
bash, and ensure the file ends with exactly one newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ee54848-0cfb-4498-b571-e7004bcbc0e3
📒 Files selected for processing (11)
jdk-javac-plugin/README.mdnullaway/build.gradlenullaway/src/main/java/com/uber/nullaway/Config.javanullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.javanullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.javanullaway/src/main/java/com/uber/nullaway/LibraryModels.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.javanullaway/src/main/resources/jspecify-jdk.astubxnullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.javanullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java
There was a problem hiding this comment.
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)
1658-1660: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not eagerly discover JarInfer models in JSpecify-only mode.
Constructing
StubxCacheUtilinvokesloadStubxFiles()from its constructor (Line 97 ofnullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java). Therefore,isJarInferEnabled=falseandisJSpecifyJDKEnabled=truestill discovers and parses JarInfer providers, violating the two flags’ independence and potentially failing on unrelated JarInfer resources.Split cache construction from JarInfer provider discovery, or pass the JarInfer flag into
StubxCacheUtiland load providers only when it is enabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java` around lines 1658 - 1660, Update ExternalStubxLibraryModels so StubxCacheUtil does not eagerly discover or parse JarInfer providers when isJarInferEnabled is false, including JSpecify-only mode. Split cache construction from provider discovery or pass the JarInfer flag into StubxCacheUtil and conditionally load providers only when enabled, while preserving JSpecify model handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 1658-1660: Update ExternalStubxLibraryModels so StubxCacheUtil
does not eagerly discover or parse JarInfer providers when isJarInferEnabled is
false, including JSpecify-only mode. Split cache construction from provider
discovery or pass the JarInfer flag into StubxCacheUtil and conditionally load
providers only when enabled, while preserving JSpecify model handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 756a3459-91ab-4b0c-b4c8-1bed3f780fa6
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java
3 participants
This PR adds an astubx capturing JSpecify JDK models of nullability, and loads and uses those models when a flag is enabled. We enable the flag for checking NullAway itself. The new JDK models can only be used in JSpecify mode, since JSpecify-mode checking is required to not get a lot of false positive warnings.
We also add a basic document on how to run our code to generate the astubx file.
For now, we add a separate flag for loading JDK models from the JarInfer flag. We can consider merging these flags in a follow-up if desirable.
Some changes in this PR fix new NullAway warnings that are due to the new models. I could try to split out those changes into a separate PR, but they're pretty localized so I just decided to do them here.
This PR was originally authored by @haewiful
Summary by CodeRabbit
.astubxartifact.