More TreePath hardening - #1684
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
WalkthroughThe change guards the shared Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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/contract/fieldcontract/EnsuresNonNullIfHandler.java`:
- Around line 123-124: Add Javadoc to the private method
buildUpReturnToEnclosingMethodMap, documenting that it records only return
statements directly enclosed by the method currently under validation. Keep the
documentation focused on this method’s behavior and retain the existing
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: 676db359-cc96-452f-b7bc-b13d74b879ea
📒 Files selected for processing (6)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.javanullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/Handler.javanullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullIfHandler.javanullaway/src/test/java/com/uber/nullaway/EnsuresNonNullIfTests.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/contract/fieldcontract/EnsuresNonNullIfHandler.java (1)
164-177: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd Javadoc for
onDataflowVisitReturn.This non-trivial method now performs path-specific return validation. Document its handling of tracked returns and nested lambdas or anonymous classes.
As per coding guidelines,
**/src/main/**/*.javarequires Javadoc for every non-trivial method, including private methods.Proposed Javadoc
+ /** + * Validates each directly enclosed return using its path-specific visitor state. + * Returns inside nested lambdas and anonymous classes are ignored. + */ `@Override` public void onDataflowVisitReturn(🤖 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/contract/fieldcontract/EnsuresNonNullIfHandler.java` around lines 164 - 177, Add Javadoc directly above onDataflowVisitReturn describing that it validates tracked return statements using the path-specific dataflow context and ignores returns outside the method under analysis, including returns nested in lambdas or anonymous classes.Source: Coding guidelines
🤖 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/contract/fieldcontract/EnsuresNonNullIfHandler.java`:
- Around line 164-177: Add Javadoc directly above onDataflowVisitReturn
describing that it validates tracked return statements using the path-specific
dataflow context and ignores returns outside the method under analysis,
including returns nested in lambdas or anonymous classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76b8638d-7669-44a3-9035-d6ebbcba0263
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullIfHandler.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1684 +/- ##
=========================================
Coverage 87.77% 87.77%
Complexity 3143 3143
=========================================
Files 109 109
Lines 10608 10613 +5
Branches 2151 2152 +1
=========================================
+ Hits 9311 9316 +5
Misses 611 611
Partials 686 686 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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/dataflow/AccessPathNullnessPropagation.java (1)
224-225: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd Javadoc for the changed non-trivial methods.
Document the constructor’s guarded
VisitorStateinvariant and the path-specific state passed to method-invocation handlers.Suggested Javadocs
+ /** + * Creates a propagation transfer function with a guarded global {`@link` VisitorState}. + * + * <p>Path-sensitive callbacks must provide an explicit path. + */ public AccessPathNullnessPropagation( + /** + * Visits a method invocation and applies its nullness updates. + * + * <p>Handlers receive a {`@link` VisitorState} rooted at the invocation tree. + */ `@Override` public TransferResult<Nullness, NullnessStore> visitMethodInvocation(As per coding guidelines,
**/src/main/**/*.javarequires Javadoc for every non-trivial method, including private methods.Also applies to: 1095-1102
🤖 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/dataflow/AccessPathNullnessPropagation.java` around lines 224 - 225, Add Javadoc to the changed non-trivial methods in AccessPathNullnessPropagation, including the constructor and the method-invocation handlers around the affected state updates. Document the constructor’s guarded VisitorState invariant and clarify that invocation handlers receive path-specific state, while preserving the existing behavior and signatures.Source: Coding guidelines
🤖 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/dataflow/AccessPathNullnessPropagation.java`:
- Around line 224-225: Add Javadoc to the changed non-trivial methods in
AccessPathNullnessPropagation, including the constructor and the
method-invocation handlers around the affected state updates. Document the
constructor’s guarded VisitorState invariant and clarify that invocation
handlers receive path-specific state, while preserving the existing behavior and
signatures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 011967af-20e1-4ec5-8012-af591314a175
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java
9b2b56b to
d0a83c4
Compare
11f35e7 to
23b21c2
Compare
d0a83c4 to
6fc043b
Compare
23b21c2 to
d681267
Compare
d681267 to
389ce9b
Compare
2 participants
For step 3 of #1680 (comment)
We modify the global
VisitorStatestored inAccessPathNullnessPropagationsuch that any use of itsTreePathcauses an immediate failure at runtime, via the new classFailingTreePath. This exposed an issue in theEnsuresNonNullIfHandlerwhere this tree path was being used to check suppressions on return trees. Now, we store correct paths for those return trees and use those instead. This allows us to remove theVisitorStateparameter fromonDataflowVisitReturnentirely.Summary by CodeRabbit
Bug Fixes
@EnsuresNonNullIfcontracts across control-flow paths and source locations.Behavior
@EnsuresNonNullIfdiagnostics can now be suppressed with@SuppressWarnings("NullAway").Tests