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 |
![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/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:
|
msridhar
force-pushed
the
jspecify-conditional-expr-wildcard
branch
2 times, most recently
from
77cd8fd to
464e44a
Compare
msridhar
force-pushed
the
jspecify-conditional-expr-wildcard
branch
3 times, most recently
from
7e7240a to
516ec37
Compare
msridhar
force-pushed
the
jspecify-conditional-expr-wildcard
branch
from
516ec37 to
14ba0f3
Compare
msridhar
changed the title
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
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
Hey, sorry this one took me a while, but it is was indeed a fairly involved PR, so no chance to get to it during the week...
As it is, I still don't really have the entire algorithm / design swapped in memory (nor do I think I can in a feasible amount of time 😅 ), so a lot of my comments are local to what I see each individual method do and to the tests, but hopefully still useful!
Looks like we can handle a ton of complex inference cases now, though! Cool! 🎉
| return new TargetTypeAndAssignmentKind(methodSymbol.getReturnType(), false); | ||
| } | ||
| } | ||
| return new TargetTypeAndAssignmentKind(null, false); |
There was a problem hiding this comment.
Maybe silly (and valid defensive programming), but isn't this the same as the full method's default case? I think removing this return preserves the same method logic. Unless I am missing something.
| if (methodType == null) { | ||
| return new TargetTypeAndAssignmentKind(null, false); | ||
| } | ||
| return new TargetTypeAndAssignmentKind( | ||
| getFormalParameterTypeForArgument( | ||
| parentInvocation, methodType.asMethodType(), expressionTree), | ||
| false); |
There was a problem hiding this comment.
Similarly, this could be...
| if (methodType == null) { | |
| return new TargetTypeAndAssignmentKind(null, false); | |
| } | |
| return new TargetTypeAndAssignmentKind( | |
| getFormalParameterTypeForArgument( | |
| parentInvocation, methodType.asMethodType(), expressionTree), | |
| false); | |
| if (methodType != null) { | |
| return new TargetTypeAndAssignmentKind( | |
| getFormalParameterTypeForArgument( | |
| parentInvocation, methodType.asMethodType(), expressionTree), | |
| false); | |
| } |
| TreePath parentPath = expressionPath.getParentPath(); | ||
| if (parentPath == null) { | ||
| return new TargetTypeAndAssignmentKind(null, false); | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| // 1. Climb up to corresponding "assignment-like" context: |
| parent = parentPath.getLeaf(); | ||
| } | ||
| } | ||
| VisitorState parentState = state.withPath(parentPath); |
There was a problem hiding this comment.
| VisitorState parentState = state.withPath(parentPath); | |
| VisitorState parentState = state.withPath(parentPath); | |
| // 2. We are at the right point in the tree now. Three cases: | |
| // 2a. `foo = [expr];` or `var foo = [expr];` (use getTargetTypeForAssignmentContext) |
| if (parent instanceof AssignmentTree || parent instanceof VariableTree) { | ||
| return getTreeType(parent, state.withPath(parentPath)); | ||
| return getTargetTypeForAssignmentContext(parent, parentState, calledFromDataflow); | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| // 2b. `return [expr];` => target is containing method's formal return type |
| TreePath conditionalPath = | ||
| Objects.equals(state.getPath().getLeaf(), tree) | ||
| ? state.getPath() | ||
| : pathWithLeaf(state.getPath(), tree); |
There was a problem hiding this comment.
Why is state.getPath() potentially at two different places (and only those two places) here?
| } | ||
| void test() { | ||
| // BUG: Diagnostic contains: passing @Nullable parameter | ||
| var inferredFromInitializer = new Box<>(null); |
There was a problem hiding this comment.
Why does this return an error? Shouldn't it just infer Box<@Nullable Object>? (I think the test is explicitly asserting that it does not, but I am not sure I recall why those would be the semantics. Maybe worth a comment or JSpecify link?)
| void test(boolean flag) { | ||
| nullableField = flag ? id(null) : id("fallback"); | ||
| // BUG: Diagnostic contains: passing @Nullable parameter | ||
| nonNullField = flag ? id(null) : id("fallback"); |
There was a problem hiding this comment.
Silly, but, for completeness, can we add the case where the null and non-null arms reversed too?
| void test(boolean first, boolean second) { | ||
| nullableField = first ? id(null) : second ? id("value") : "fallback"; | ||
| // BUG: Diagnostic contains: passing @Nullable parameter | ||
| nonNullField = first ? id(null) : second ? id("value") : "fallback"; |
There was a problem hiding this comment.
Again, let's make sure it works with the second id(...) being the one that takes null
(I don't think we need this on every test, necessarily, but at least one plain and one nested case)
| // BUG: Diagnostic contains: passing @Nullable parameter | ||
| var inferredFromInitializer = flag ? new Box<>(null) : new Box<>("fallback"); |
There was a problem hiding this comment.
I mention it above, but again mildly surprised that var inferredFromInitializer isn't just inferred to be Box<@Nullable String> here. I suspect there is a reason for it in the JSpecify semantics, just think it might be worth a short comment (see GenericDiamondTests.java's comment)
2 participants
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