Properly model AtomicReference.get() in JSpecify mode #1298
Conversation
WalkthroughMakes LibraryModels config-driven by constructing DefaultLibraryModels with Config and computing nullable-returns per-config (JSPECIFY-aware). Updates sets (ALWAYS_NULLABLE_RETURNS split, added AtomicReference handling, NULLMARKED_CLASSES and NULLABLE_VARIABLE_TYPE_UPPER_BOUNDS). Adjusts loading path to pass Config. Adds a JSPECIFY-focused test validating AtomicReference.get() behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Analyzer
participant Config
participant LibraryModelsHandler
participant DefaultLibraryModels as DefaultLibraryModels (per-config)
Analyzer->>LibraryModelsHandler: loadLibraryModels(config)
LibraryModelsHandler->>DefaultLibraryModels: new DefaultLibraryModels(config)
activate DefaultLibraryModels
note right of DefaultLibraryModels: Build nullableReturnsForConfig<br/>= ALWAYS_NULLABLE_RETURNS<br/>+ (if !config.isJSpecifyMode())<br/> NULLABLE_RETURNS_JSPECIFY_MODE_DISABLED
deactivate DefaultLibraryModels
LibraryModelsHandler-->>Analyzer: LibraryModels
Analyzer->>DefaultLibraryModels: nullableReturns()
DefaultLibraryModels-->>Analyzer: nullableReturnsForConfig
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
msridhar
requested review from
Copilot,
lazaroclapp and
yuxincs
and removed request for
Copilot
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java (2)
10-30: JSPECIFY behavior well covered; consider adding a non-JSpecify counterpart.This test correctly locks the JSPECIFY-mode semantics for AtomicReference.get(). Please add a companion test with JSpecifyMode=false to guard the legacy behavior (get() treated as @nullable), preventing regressions across modes.
32-36: Add -XDaddTypeAnnotationsToSymbol=true to stabilize type-use handling across JDKs.Keeps type annotations available on symbols during compilation, reducing flakiness across compiler versions.
private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( - "-XepOpt:NullAway:OnlyNullMarked=true", "-XepOpt:NullAway:JSpecifyMode=true")); + "-XepOpt:NullAway:OnlyNullMarked=true", + "-XepOpt:NullAway:JSpecifyMode=true", + "-XDaddTypeAnnotationsToSymbol=true")); }nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (2)
842-844: Correct gating of AtomicReference.get() for non-JSpecify mode.The separate NULLABLE_RETURNS_JSPECIFY_MODE_DISABLED keeps legacy behavior intact when JSPECIFY is off.
Optional: rename to NULLABLE_RETURNS_WHEN_JSPECIFY_DISABLED for readability.
901-905: AtomicReference added to null-marked classes; confirm no effect when JSPECIFY is off.If the NullMarkedClasses hook is consulted outside JSPECIFY mode, this could subtly change legacy semantics. If that’s possible, consider gating this via config as you did for nullableReturns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java(5 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java
🧬 Code graph analysis (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java (1)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
NullAwayTestsBase(9-51)
⏰ 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 ubuntu-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build spring-framework with snapshot
🔇 Additional comments (5)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (5)
389-389: Plumbing Config into DefaultLibraryModels is correct.This enables per-config model shaping without affecting external models ordering.
818-840: Good: AtomicReference.get() removed from always-nullable set.Matches the new JSPECIFY modeling where nullability depends on the type argument.
897-898: Add nullable upper bound for AtomicReference (index 0).This is the key to JSPECIFY semantics; looks right.
Please confirm no additional Atomic* types (e.g., AtomicReferenceArray) also need upper-bound modeling for parity.
909-922: Per-config nullableReturns cache is clean and efficient.One-time computation via constructor keeps lookups fast and logic clear.
954-956: Switched to config-derived nullable returns.Return path now reflects the chosen mode; LGTM.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1298 +/- ##
============================================
+ Coverage 88.47% 88.49% +0.01%
Complexity 2476 2476
============================================
Files 93 93
Lines 8203 8214 +11
Branches 1616 1617 +1
============================================
+ Hits 7258 7269 +11
Misses 474 474
Partials 471 471 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks like this breaks the build for both Caffeine and Spring :-) I'll see if I can do upstream PRs on those projects before landing this one (assuming the fixes don't cause new errors with the current NullAway release...) |
|
I opened a PR for the Spring failures and I'm pretty confident in it, so I'm going to go ahead and land this. |
Previously,
AtomicReference.get()was modeled as always returning@Nullable. Now, in JSpecify mode, we do not use that model. Instead, we modelAtomicReferenceas a null-marked class with a nullable upper bound on its type variable. This required some tweaking toDefaultLibraryModelswhich now takes aConfigobject in its constructor and customizes its library models a bit based on that.Fixes #681. This is an interim fix until we can address #950.
Summary by CodeRabbit
New Features
Bug Fixes
Tests