Enable JSpecify JDK models for regression tests - #1646
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1646 +/- ##
=========================================
Coverage 88.16% 88.16%
- Complexity 3065 3066 +1
=========================================
Files 105 105
Lines 10302 10303 +1
Branches 2079 2080 +1
=========================================
+ Hits 9083 9084 +1
Misses 576 576
Partials 643 643 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f056a9a to
0156e31
Compare
bd65c72 to
69e379d
Compare
| static class K<T extends @Nullable Object> {} | ||
| void foo(K<@Nullable Object> k) { | ||
| K<? extends @Nullable Object> k2 = k; | ||
| // TODO should get no error here |
| "-d", | ||
| temporaryFolder.getRoot().getAbsolutePath(), | ||
| "-XepOpt:NullAway:AnnotatedPackages=com.uber")) | ||
| JSpecifyJavacConfig.withJSpecifyModeArgs( |
There was a problem hiding this comment.
Run this one in JSpecify mode to ensure that loading of JDK models does not also load external astubx files from JarInfer
There was a problem hiding this comment.
Might be worth putting this comment into the code itself?
69e379d to
62af508
Compare
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
SGTM, modulo the follow ups
| "-d", | ||
| temporaryFolder.getRoot().getAbsolutePath(), | ||
| "-XepOpt:NullAway:AnnotatedPackages=com.uber")) | ||
| JSpecifyJavacConfig.withJSpecifyModeArgs( |
There was a problem hiding this comment.
Might be worth putting this comment into the code itself?
62af508 to
0b639da
Compare
![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)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
1680-1694: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail fast when the JSpecify JDK model is missing.
With JSpecify JDK models enabled, a missing
jspecify-jdk.astubxresource goes to a no-op debug log, so NullAway continues without the JDK nullability models and can miss reports such as those covered byJSpecifyJDKModelsTest. Treat the missing required resource as a normal warning/error.🤖 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 1680 - 1694, The JSpecify JDK model loading branch must not silently continue when JSPECIFY_JDK_ASTUBX_FILENAME is absent. Update the null-resource handling in the isJSpecifyJDKEnabled block to emit a normal warning or error and fail fast, while preserving successful parsing and existing IOException 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/test/java/com/uber/nullaway/jspecify/GenericsTests.java`:
- Around line 2008-2010: Update the local variable declaration for o in the
toArray() test to remove the top-level `@Nullable` annotation and express nullable
array-reference semantics as Object `@Nullable` [] (or rely on var/inferred typing
if appropriate). Preserve the ternary behavior and existing toArray() model
TODO.
---
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 1680-1694: The JSpecify JDK model loading branch must not silently
continue when JSPECIFY_JDK_ASTUBX_FILENAME is absent. Update the null-resource
handling in the isJSpecifyJDKEnabled block to emit a normal warning or error and
fail fast, while preserving successful parsing and existing IOException
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: 81ae1b3e-5898-48b4-8a27-d2073113196d
📒 Files selected for processing (7)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.javanullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.javanullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/StreamNullabilityPropagatorTests.java
0b639da to
224af39
Compare
….java Co-authored-by: Lázaro Clapp <lazaro@uber.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java (1)
2008-2010: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAnnotate the nullable array reference, not its component type.
@Nullable Object[]makes the component type nullable, soois still a non-null array, while the false branch isnull. UseObject@nullable[] ofor the intended ternary result.Based on learnings,
Nullable Object[]annotates the component type, whereasObject@nullable[]annotates the array reference.Proposed fix
- `@Nullable` Object[] o = a != null ? a.toArray() : null; + Object `@Nullable` [] o = a != null ? a.toArray() : null;🤖 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/test/java/com/uber/nullaway/jspecify/GenericsTests.java` around lines 2008 - 2010, Update the declaration of variable o in the toArray() ternary to annotate the array reference rather than its component type, using the project’s array-annotation syntax so the nullable false branch is accepted.Source: Learnings
🤖 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.
Duplicate comments:
In `@nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java`:
- Around line 2008-2010: Update the declaration of variable o in the toArray()
ternary to annotate the array reference rather than its component type, using
the project’s array-annotation syntax so the nullable false branch is accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a264c52-32a1-4c05-a16d-467f26da245f
📒 Files selected for processing (7)
jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.javanullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.javanullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/StreamNullabilityPropagatorTests.java
2 participants
This PR enables the JSpecify JDK models for all regression tests run in JSpecify mode.
The change exposed a bug where enabling the JDK models also enabled loading of external astubx files. We fix that here and only load external astubx files if JarInfer is enabled.
There were a couple of issues exposed by this change by our current tests. One is documented in #1616. Another is fixed in #1647, a follow-up to this PR. Another is just a change in the warning that gets printed.
Summary by CodeRabbit
-XepOpt:NullAway:JSpecifyJDKModels=true).