Support method-based conditional library model postconditions - #1677
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a library-model contract for conditional receiver method nullness. Aggregates and optimizes these mappings across library models. Conditional analysis resolves target methods through class hierarchies and marks receiver access paths non-null on the true branch. Default models relate Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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: 2
🤖 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 350-356: Add Javadoc for the private method
setConditionalArgumentNullness in
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
lines 350-356, documenting its inputs, branch updates, and receiver-method
refinement. Also document the non-trivial private method at lines 390-404 in the
same file, describing its hierarchy lookup behavior and nullable return
semantics.
- Around line 390-404: Update lookupMethodSymbol to traverse classSymbol’s
directly implemented interfaces, recursively resolving targetRef before
returning null; retain superclass traversal so interfaces inherited through
superclasses are also searched. Add a regression test using a conditional model
targeting a default interface method and verify the true-branch update is
applied.
🪄 Autofix
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: 457727cc-fc99-412f-8357-826c9f9fae08
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/LibraryModels.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/test/java/com/uber/nullaway/FrameworkTests.java
f2a7cf5 to
bc1795a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
bc1795a to
71d949d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/LibraryModels.java`:
- Around line 90-100: Correct the contract for nullImpliesFalseMethodCalls:
update its Javadoc to describe that a null target-method result implies the
querying method returns false, with the non-null result implying true by
contrapositive. Keep the method name and default empty ImmutableSetMultimap
implementation unchanged.
🪄 Autofix
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: 6f9ec139-b3fc-48fc-8bc7-aa9bf6de159a
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/LibraryModels.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/test/java/com/uber/nullaway/FrameworkTests.javatest-java-lib/src/main/java/com/uber/lib/unannotated/CustomInterface.javatest-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java
| /** | ||
| * Get (method, target method) pairs where returning <code>false</code> implies the target method | ||
| * on the receiver is <code>null</code> (and returning <code>true</code> implies the target method | ||
| * on the receiver is non-null). | ||
| * | ||
| * @return map from querying methods to target receiver methods that are non-null when returning | ||
| * true. | ||
| */ | ||
| default ImmutableSetMultimap<MethodRef, MethodRef> nullImpliesFalseMethodCalls() { | ||
| return ImmutableSetMultimap.of(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the conditional-model contract.
The Javadoc states that a false result implies that the target method returns null. The model name and implementation encode the opposite implication: a null target result implies false. The true-branch non-null result follows by contrapositive.
Proposed fix
- * Get (method, target method) pairs where returning <code>false</code> implies the target method
- * on the receiver is <code>null</code> (and returning <code>true</code> implies the target method
- * on the receiver is non-null).
+ * Get (querying method, target receiver method) pairs where a <code>null</code> target result
+ * implies that the querying method returns <code>false</code>.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Get (method, target method) pairs where returning <code>false</code> implies the target method | |
| * on the receiver is <code>null</code> (and returning <code>true</code> implies the target method | |
| * on the receiver is non-null). | |
| * | |
| * @return map from querying methods to target receiver methods that are non-null when returning | |
| * true. | |
| */ | |
| default ImmutableSetMultimap<MethodRef, MethodRef> nullImpliesFalseMethodCalls() { | |
| return ImmutableSetMultimap.of(); | |
| } | |
| /** | |
| * Get (querying method, target receiver method) pairs where a <code>null</code> target result | |
| * implies that the querying method returns <code>false</code>. | |
| * | |
| * `@return` map from querying methods to target receiver methods that are non-null when returning | |
| * true. | |
| */ | |
| default ImmutableSetMultimap<MethodRef, MethodRef> nullImpliesFalseMethodCalls() { | |
| return ImmutableSetMultimap.of(); | |
| } |
🤖 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/LibraryModels.java` around lines 90
- 100, Correct the contract for nullImpliesFalseMethodCalls: update its Javadoc
to describe that a null target-method result implies the querying method returns
false, with the non-null result implying true by contrapositive. Keep the method
name and default empty ImmutableSetMultimap implementation unchanged.
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks, this looks promising! I have some review comments below
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Whoops, meant to request changes before
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1677 +/- ##
============================================
- Coverage 87.88% 87.82% -0.06%
- Complexity 3175 3181 +6
============================================
Files 109 109
Lines 10763 10809 +46
Branches 2174 2185 +11
============================================
+ Hits 9459 9493 +34
- Misses 618 622 +4
- Partials 686 694 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@msridhar Thanks for the review! I'll address these comments and push an updated revision shortly. |
71d949d to
8db933d
Compare
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. I have another comment below.
Also, for future reference, please respond to my review comments and describe how they were addressed. This will make it easier for me to do a follow-up review. (And, I'd prefer these responses not be copy-pasted from an LLM.)
8db933d to
df58539
Compare
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Now mostly LGTM! Just one more change and I think we'll be good to go
Signed-off-by: Shankar V <shankarvelmurugan2018@gmail.com>
df58539 to
53bb09f
Compare
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
|
Thank you for the review and guidance throughout the process! I really appreciate it. |
2 participants
Summary
Fixes #1664.
This PR extends
LibraryModelsto support conditional postconditions on receiver methods, allowing library models to express relationships where the return value of one method refines the nullability of another method on the same receiver.As an initial use case, it models the JDK relationship:
Class.isArray()returningtrueimpliesClass.getComponentType()is non-null.This eliminates the false positive described in the issue while reusing NullAway's existing conditional dataflow infrastructure (
AccessPath,thenUpdates, andLibraryModelsHandler).The implementation is intentionally limited to library models and does not change the parsing or semantics of user-written
@EnsuresNonNullIfannotations.Changes
LibraryModelsfor modeling conditional receiver method postconditions.LibraryModelsHandlerto propagate these postconditions using the existingAccessPathand conditional dataflow mechanisms.Class.isArray() -> Class.getComponentType()Testing
Added a regression test verifying that:
does not produce a nullness warning, while dereferencing
getComponentType()outside the refined branch continues to report a warning.Verified with:
AI Usage
I used ChatGPT to better understand the existing NullAway architecture, discuss implementation approaches, and review the design. I manually implemented, reviewed, tested, and verified all code changes before submitting this PR.
Summary by CodeRabbit
New Features
Class.isArray()andClass.getComponentType().Bug Fixes