Fix Optional factory method tracking (Partial fix for #889)#1600
Fix Optional factory method tracking (Partial fix for #889)#1600dyrpsf wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces a new dataflow-analysis hook, onDataflowVisitAssignment, to NullAway's handler interface. The hook enables handlers to observe and refine nullness inference during assignment-node visits in the dataflow graph. OptionalEmptinessHandler implements this hook to recognize and propagate non-null Optional-content facts through assignments originating from Optional.of() and Optional.ofNullable() factory methods. The hook is invoked by the core dataflow engine in AccessPathNullnessPropagation and dispatched to all handlers via CompositeHandler. A new test validates these cases. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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/CompositeHandler.java (1)
380-398:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Missing closing brace for
onDataflowVisitAssignmentmethod.The
onDataflowVisitAssignmentmethod (starting at line 380) is missing its closing brace. Line 389 only closes theforloop, but the method body itself is never closed before the next method declaration begins at line 390.🐛 Proposed fix
for (Handler h : handlers) { h.onDataflowVisitAssignment(node, state, apContext, inputs, store, updates); } + } + + `@Override` public boolean isSingleArgNullImpliesFalseMethod(🤖 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/CompositeHandler.java` around lines 380 - 398, The method onDataflowVisitAssignment is missing its closing brace causing the subsequent isSingleArgNullImpliesFalseMethod declaration to be inside it; add the missing closing brace after the for-loop that iterates over handlers to properly terminate onDataflowVisitAssignment (ensure the method signature public void onDataflowVisitAssignment(...) and its body that calls h.onDataflowVisitAssignment(...) are closed), keep the handlers iteration unchanged, then recompile/format and run tests to confirm no other brace mismatches.
🤖 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/CompositeHandler.java`:
- Around line 380-398: The method onDataflowVisitAssignment is missing its
closing brace causing the subsequent isSingleArgNullImpliesFalseMethod
declaration to be inside it; add the missing closing brace after the for-loop
that iterates over handlers to properly terminate onDataflowVisitAssignment
(ensure the method signature public void onDataflowVisitAssignment(...) and its
body that calls h.onDataflowVisitAssignment(...) are closed), keep the handlers
iteration unchanged, then recompile/format and run tests to confirm no other
brace mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9995ab65-cd3e-4a04-9579-f7540813060e
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
Description
This PR addresses the false positives in
Optionalemptiness tracking related to factory method initializations, addressing the primary concern raised in #889.Motivation and Context
In #889, it was reported that
CheckOptionalEmptinessignores the initial value passed to an Optional factory (e.g.,Optional.ofandOptional.ofNullable), leading to false positives when developers safely call.get()on those variables. Previously, NullAway dropped emptiness tracking facts when anOptionalwas assigned to a variable or initialized inline.Specific Changes
onDataflowVisitAssignmentin theHandlerinterface (andCompositeHandler/AccessPathNullnessPropagation) to allow plugins to intercept assignment nodes.OptionalEmptinessHandlerto peel back implicit compiler wrappers (TypeCastNode,NullChkNode,WideningConversionNode,NarrowingConversionNode) to safely reach the underlyingMethodInvocationNodeduring assignments.Optional.of(...)(always non-empty) andOptional.ofNullable(...)(non-empty if the argument is@NonNull).base.getTree() != nullcheck inupdateNonNullAPsForOptionalContentthat was silently dropping tracking facts for inline variable initializations.@Nullableannotation to the parameter inlogOptionalOfNullableWithNullableto align the test expectations, and ensuredOptional.offlows cleanly.Note: This addresses the factory initialization cases for #889. The trickier
.or()fallback provider case mentioned in the issue may require additional modeling in a future PR, but this lays the necessary architectural groundwork.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests