Fix: Report unboxing.of.nullable in conditional expressions with primitive result#7433
Fix: Report unboxing.of.nullable in conditional expressions with primitive result#7433Suvrat1629 wants to merge 8 commits intotypetools:masterfrom
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)
📝 WalkthroughWalkthroughThe visitor method visitConditionalExpression in NullnessVisitor was extended to, when the conditional expression has a primitive target type and one or both arms are reference types, invoke checkForNullability on the relevant arm(s) with the unboxing.of.nullable check and return early if that check reports an error. A new test Issue6849.java was added demonstrating a ternary expression combining a nullable generic reference and a primitive constant to exercise this unboxing nullness check. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.javachecker/tests/nullness/Issue6849.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java:470-515
Timestamp: 2025-10-22T20:40:48.819Z
Learning: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.
📚 Learning: 2025-10-22T20:40:48.819Z
Learnt from: kelloggm
Repo: typetools/checker-framework PR: 7166
File: checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java:470-515
Timestamp: 2025-10-22T20:40:48.819Z
Learning: In MustCallVisitor.java (Checker Framework), prefer using Name.equals(...) or Objects.equals(...) for com.sun.source.util.Name comparisons (instead of ==/!=). This should be the default unless the Interning Checker is explicitly used to guarantee reference equality.
Applied to files:
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
🧬 Code graph analysis (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1)
javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)
🔇 Additional comments (13)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (12)
71-72: Formatting-only change — no functional impact.
144-146: Formatting-only change — no functional impact.
157-163: Formatting-only change — no functional impact.
192-196: Formatting-only change — no functional impact.
221-226: Formatting-only change — no functional impact.
394-400: Formatting-only change — no functional impact.
442-444: Formatting-only change — no functional impact.
571-571: Formatting-only change — no functional impact.
699-701: Formatting-only change — no functional impact.
825-836: Formatting-only change — no functional impact.
841-842: Formatting-only change — no functional impact.
787-816: Logic correctly detects unboxing in conditional expressions, verified by existing tests.The implementation properly:
- Checks if the overall conditional result type is primitive
- Identifies operands that need unboxing (reference types being assigned to primitive)
- Reports
unboxing.of.nullablewhen nullable references would be unboxed- Returns early after reporting an error to avoid cascading errors (consistent with
visitTypeCastpattern)The new logic is exercised by existing tests in
Issue6849.javaandIssue3614.java, which verify that unboxing errors are correctly reported in conditional expressions (e.g.,int y = ((true) ? method() : 10)when method returns@Nullable Integer).When both operands need unboxing and both are nullable, only the first error is reported due to the early return. This is acceptable—the cascading error prevention is intentional, though users would need to re-run after fixing the first issue to see subsequent errors.
checker/tests/nullness/Issue6849.java (1)
6-8: Consider adding@Nullableannotation to the return type for explicitness.The method
mreturnsT, which when instantiated as@Nullable Integercorrectly returns a nullable value. The current implementation is correct, but you might consider whether adding a test case that explicitly shows the return type could help document the behavior.
|
@smillst gentle ping please take a look |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`:
- Around line 780-805: Replace the explicit primitive checks that call
TypesUtils.isPrimitive(TreeUtils.typeOf(...)) with the class's existing helper
isPrimitive(ExpressionTree) for consistency and readability inside
NullnessVisitor (the conditional-expression handling that computes
trueNeedsUnboxing/falseNeedsUnboxing); keep the rest of the logic intact (still
call checkForNullability on trueExpr/falseExpr with UNBOXING_OF_NULLABLE and
preserve the early-return behavior when checkForNullability returns false) so
only the primitive-test expressions are swapped to use
isPrimitive(ExpressionTree).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
| // If the overall conditional expression has a primitive Java type but one or both | ||
| // operand expressions are reference types, then unboxing will occur as part of | ||
| // evaluating the conditional. In that case, check the operand(s) for possible | ||
| // nullness (unboxing.of.nullable). | ||
| AnnotatedTypeMirror type = atypeFactory.getAnnotatedType(tree); | ||
| // Only check for unboxing when javac has chosen a primitive underlying type | ||
| // for the conditional expression, but one or both operands are non-primitive. | ||
| if (type.getKind().isPrimitive()) { | ||
| ExpressionTree trueExpr = tree.getTrueExpression(); | ||
| ExpressionTree falseExpr = tree.getFalseExpression(); | ||
| boolean trueNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(trueExpr)); | ||
| boolean falseNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(falseExpr)); | ||
|
|
||
| if (trueNeedsUnboxing) { | ||
| if (!checkForNullability(trueExpr, UNBOXING_OF_NULLABLE)) { | ||
| // If we reported an unboxing.of.nullable error for the true arm, stop | ||
| // further checking to avoid cascading errors. | ||
| return null; | ||
| } | ||
| } | ||
| if (falseNeedsUnboxing) { | ||
| if (!checkForNullability(falseExpr, UNBOXING_OF_NULLABLE)) { | ||
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Logic is correct; consider using the existing helper method for consistency.
The fix correctly identifies when javac implicitly unboxes a reference-typed operand in a conditional expression with a primitive result type and reports unboxing.of.nullable when appropriate. The early return pattern is consistent with visitTypeCast (lines 509-513).
Minor suggestion: The existing isPrimitive(ExpressionTree) helper method (lines 714-716) could be reused for slightly more readable and consistent code.
♻️ Suggested refactor to use existing helper
if (type.getKind().isPrimitive()) {
ExpressionTree trueExpr = tree.getTrueExpression();
ExpressionTree falseExpr = tree.getFalseExpression();
- boolean trueNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(trueExpr));
- boolean falseNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(falseExpr));
+ boolean trueNeedsUnboxing = !isPrimitive(trueExpr);
+ boolean falseNeedsUnboxing = !isPrimitive(falseExpr);
if (trueNeedsUnboxing) {📝 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.
| // If the overall conditional expression has a primitive Java type but one or both | |
| // operand expressions are reference types, then unboxing will occur as part of | |
| // evaluating the conditional. In that case, check the operand(s) for possible | |
| // nullness (unboxing.of.nullable). | |
| AnnotatedTypeMirror type = atypeFactory.getAnnotatedType(tree); | |
| // Only check for unboxing when javac has chosen a primitive underlying type | |
| // for the conditional expression, but one or both operands are non-primitive. | |
| if (type.getKind().isPrimitive()) { | |
| ExpressionTree trueExpr = tree.getTrueExpression(); | |
| ExpressionTree falseExpr = tree.getFalseExpression(); | |
| boolean trueNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(trueExpr)); | |
| boolean falseNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(falseExpr)); | |
| if (trueNeedsUnboxing) { | |
| if (!checkForNullability(trueExpr, UNBOXING_OF_NULLABLE)) { | |
| // If we reported an unboxing.of.nullable error for the true arm, stop | |
| // further checking to avoid cascading errors. | |
| return null; | |
| } | |
| } | |
| if (falseNeedsUnboxing) { | |
| if (!checkForNullability(falseExpr, UNBOXING_OF_NULLABLE)) { | |
| return null; | |
| } | |
| } | |
| } | |
| // If the overall conditional expression has a primitive Java type but one or both | |
| // operand expressions are reference types, then unboxing will occur as part of | |
| // evaluating the conditional. In that case, check the operand(s) for possible | |
| // nullness (unboxing.of.nullable). | |
| AnnotatedTypeMirror type = atypeFactory.getAnnotatedType(tree); | |
| // Only check for unboxing when javac has chosen a primitive underlying type | |
| // for the conditional expression, but one or both operands are non-primitive. | |
| if (type.getKind().isPrimitive()) { | |
| ExpressionTree trueExpr = tree.getTrueExpression(); | |
| ExpressionTree falseExpr = tree.getFalseExpression(); | |
| boolean trueNeedsUnboxing = !isPrimitive(trueExpr); | |
| boolean falseNeedsUnboxing = !isPrimitive(falseExpr); | |
| if (trueNeedsUnboxing) { | |
| if (!checkForNullability(trueExpr, UNBOXING_OF_NULLABLE)) { | |
| // If we reported an unboxing.of.nullable error for the true arm, stop | |
| // further checking to avoid cascading errors. | |
| return null; | |
| } | |
| } | |
| if (falseNeedsUnboxing) { | |
| if (!checkForNullability(falseExpr, UNBOXING_OF_NULLABLE)) { | |
| return null; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`
around lines 780 - 805, Replace the explicit primitive checks that call
TypesUtils.isPrimitive(TreeUtils.typeOf(...)) with the class's existing helper
isPrimitive(ExpressionTree) for consistency and readability inside
NullnessVisitor (the conditional-expression handling that computes
trueNeedsUnboxing/falseNeedsUnboxing); keep the rest of the logic intact (still
call checkForNullability on trueExpr/falseExpr with UNBOXING_OF_NULLABLE and
preserve the early-return behavior when checkForNullability returns false) so
only the primitive-test expressions are swapped to use
isPrimitive(ExpressionTree).
|
@Suvrat1629 Could you please respond to the code review from CodeRabbit? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1)
784-785: 🧹 Nitpick | 🔵 TrivialReuse the existing
isPrimitive(ExpressionTree)helper for consistency.Lines 784–785 re-implement what the private helper
isPrimitive(ExpressionTree)at line 708 already does. Using the helper keeps this method consistent withvisitTypeCast(line 507) andisUnboxingOperation(line 682).♻️ Proposed refactor
- boolean trueNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(trueExpr)); - boolean falseNeedsUnboxing = !TypesUtils.isPrimitive(TreeUtils.typeOf(falseExpr)); + boolean trueNeedsUnboxing = !isPrimitive(trueExpr); + boolean falseNeedsUnboxing = !isPrimitive(falseExpr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java` around lines 784 - 785, Replace the ad-hoc primitive checks for trueNeedsUnboxing and falseNeedsUnboxing by calling the existing private helper isPrimitive(ExpressionTree) instead of TypesUtils.isPrimitive(TreeUtils.typeOf(...)); update the assignments for trueNeedsUnboxing and falseNeedsUnboxing to use isPrimitive(trueExpr) and isPrimitive(falseExpr) so this logic matches the other usages in visitTypeCast and isUnboxingOperation and keeps behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`:
- Around line 787-798: In NullnessVisitor, when checking the conditional arms,
don't early-return after checkForNullability(trueExpr, UNBOXING_OF_NULLABLE);
instead always call checkForNullability on both trueExpr and falseExpr (using
UNBOXING_OF_NULLABLE) and record each result, then return null if either check
failed; mirror the pattern used in visitBinary so both operands can report
unboxing errors while still stopping further processing if any failed. Ensure
you reference the existing trueNeedsUnboxing/falseNeedsUnboxing guards and keep
the existing behavior of returning null when any arm's check fails.
---
Duplicate comments:
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`:
- Around line 784-785: Replace the ad-hoc primitive checks for trueNeedsUnboxing
and falseNeedsUnboxing by calling the existing private helper
isPrimitive(ExpressionTree) instead of
TypesUtils.isPrimitive(TreeUtils.typeOf(...)); update the assignments for
trueNeedsUnboxing and falseNeedsUnboxing to use isPrimitive(trueExpr) and
isPrimitive(falseExpr) so this logic matches the other usages in visitTypeCast
and isUnboxingOperation and keeps behavior consistent.
🪄 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: a82e6db6-7e99-4027-b8ab-a0b94a998ee1
📒 Files selected for processing (1)
checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java
| if (trueNeedsUnboxing) { | ||
| if (!checkForNullability(trueExpr, UNBOXING_OF_NULLABLE)) { | ||
| // If we reported an unboxing.of.nullable error for the true arm, stop | ||
| // further checking to avoid cascading errors. | ||
| return null; | ||
| } | ||
| } | ||
| if (falseNeedsUnboxing) { | ||
| if (!checkForNullability(falseExpr, UNBOXING_OF_NULLABLE)) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Early return after true arm error skips checking the false arm.
If the true arm fails the nullability check, the method returns before checking falseExpr, so a second unboxing error on the false arm is suppressed in the same conditional. visitTypeCast (lines 507–513) has only one operand so the early-return pattern is equivalent there, but a conditional expression has two independent operands that can each require unboxing. Consider reporting on both arms before returning, to match how visitBinary (lines 476–479) checks both operands unconditionally.
♻️ Proposed change
- if (trueNeedsUnboxing) {
- if (!checkForNullability(trueExpr, UNBOXING_OF_NULLABLE)) {
- // If we reported an unboxing.of.nullable error for the true arm, stop
- // further checking to avoid cascading errors.
- return null;
- }
- }
- if (falseNeedsUnboxing) {
- if (!checkForNullability(falseExpr, UNBOXING_OF_NULLABLE)) {
- return null;
- }
- }
+ boolean ok = true;
+ if (trueNeedsUnboxing) {
+ ok &= checkForNullability(trueExpr, UNBOXING_OF_NULLABLE);
+ }
+ if (falseNeedsUnboxing) {
+ ok &= checkForNullability(falseExpr, UNBOXING_OF_NULLABLE);
+ }
+ if (!ok) {
+ // Avoid cascading errors from the superclass visit.
+ return null;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java`
around lines 787 - 798, In NullnessVisitor, when checking the conditional arms,
don't early-return after checkForNullability(trueExpr, UNBOXING_OF_NULLABLE);
instead always call checkForNullability on both trueExpr and falseExpr (using
UNBOXING_OF_NULLABLE) and record each result, then return null if either check
failed; mirror the pattern used in visitBinary so both operands can report
unboxing errors while still stopping further processing if any failed. Ensure
you reference the existing trueNeedsUnboxing/falseNeedsUnboxing guards and keep
the existing behavior of returning null when any arm's check fails.
3 participants
@Nullableboxed value in a ternary expression with primitive result type was unboxed without warning.NullnessVisitor.visitConditionalExpressionto detect whenjavacperforms unboxing conversion on one or both operands.Issue6849.java.Closes #6849