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 6 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)
📝 WalkthroughWalkthroughvisitConditionalExpression in NullnessVisitor now performs unboxing-aware nullness checks (UNBOXING_OF_NULLABLE) on the true and false operands when the conditional expression has a primitive target type but one or both operands are reference types. If an unboxing check reports a nullness error for an operand, the method returns early to avoid cascading diagnostics; otherwise it continues with the existing flow. A new test Issue6849.java was added demonstrating a ternary expression combining a nullable reference and a primitive constant to exercise this check. 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).
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