Better support for conditional expressions in JSpecify mode#1608
Better support for conditional expressions in JSpecify mode#1608msridhar wants to merge 13 commits into
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Walkthrough
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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/generics/GenericsChecks.java`:
- Around line 2088-2097: The `inferConditionalExpressionType` method returns a
nullable Type, but the result assigned to `actualParameterType` is used without
a null check. Add a null check immediately after the assignment from
`inferConditionalExpressionType` (within the conditional block for
ConditionalExpressionTree) to ensure `actualParameterType` is not null before it
is used at line 2098 in the `subtypeParameterNullability` call. If the result is
null, handle it appropriately by continuing to the next iteration or setting a
default value, similar to how other branches handle edge cases.
🪄 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
Run ID: b9f9ce7e-22a5-4370-9033-dd6605649ea9
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/ConditionalExprTests.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1608 +/- ##
============================================
+ Coverage 88.10% 88.17% +0.07%
- Complexity 3046 3071 +25
============================================
Files 105 105
Lines 10181 10286 +105
Branches 2062 2083 +21
============================================
+ Hits 8970 9070 +100
- Misses 572 575 +3
- Partials 639 641 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
77cd8fd to
464e44a
Compare
7e7240a to
516ec37
Compare
516ec37 to
14ba0f3
Compare
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/generics/GenericsChecks.java`:
- Around line 1872-1888: In getConditionalExpressionType, the cached
assignment-context type can still be raw and should not be returned or stored
as-is. Update the logic around inferredPolyExpressionTypes and
getTargetTypeForConditionalExpression so that any typeFromAssignmentContext is
passed through typeOrNullIfRaw(...) before caching and before returning,
ensuring inferConditionalExpressionType never sees a raw conditional target.
🪄 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
Run ID: c5df7e9e-5c51-46eb-9730-a74f0e63eb9b
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/ConditionalExprTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/GenericDiamondTests.javanullaway/src/test/java/com/uber/nullaway/jspecify/VarDeclaredLocalTests.java
We improve support for conditional expressions in JSpecify mode, by properly inferring types of conditional expressions where appropriate. We also propagate types from the assignment context through (nested) conditional expressions to correctly infer types of generic method calls nested within such expressions. See the new
ConditionalExprTestsfor examples.We extract out some common code for finding a type from the assignment context given a
TreePathinto a new methodgetTargetTypeFromParentContext, and now use it consistently to handle getting assignment contexts for conditional expressions, generic method calls, and diamond constructor calls. As part of this extraction, we make more consistent our handling ofvar-declared locals (ensuring we don't use theirjavac-inferred type during our own inference), and we add tests for this.Apologies for the size of this PR; it would have been a bit awkward to make it smaller.
Summary by CodeRabbit
Release Notes
New Features
?:) expressions for nullability analysis and generic type inference.var(when no target type is available).Tests