issue 1275 - report unboxing warning for for-each loop #1281
Conversation
WalkthroughAdds a JSpecify-mode check in EnhancedForLoop processing that reports UNBOX_NULLABLE when a primitive loop variable would unbox potentially @nullable array elements; adds tests covering nullable and non-null array element cases and a test-only TYPE_USE Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Visitor/Analyzer
participant NullAway as NullAway.enhancedForLoop
participant Config as Config (JSpecifyMode)
participant Types as ASTHelpers/Types
Caller->>NullAway: visitEnhancedForLoop(node)
NullAway->>Config: isJSpecifyMode()
alt Not JSpecify
NullAway-->>Caller: NO_MATCH / continue existing checks
else JSpecify
NullAway->>Types: getType(loopVariable)
alt loopVariable is primitive
NullAway->>Types: getType(loopExpression)
alt expression is array
NullAway->>Types: resolve array symbol / isArrayElementNullable(..., config)
alt elements nullable
Note right of NullAway #lightblue: New UNBOX_NULLABLE path
NullAway-->>Caller: report UNBOX_NULLABLE ("unboxing of a @Nullable value")
else elements non-null
NullAway-->>Caller: NO_MATCH / continue existing checks
end
else not array
NullAway-->>Caller: continue existing logic
end
else non-primitive
NullAway-->>Caller: continue existing logic
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1281 +/- ##
============================================
- Coverage 88.45% 88.43% -0.02%
- Complexity 2454 2457 +3
============================================
Files 92 92
Lines 8100 8114 +14
Branches 1610 1614 +4
============================================
+ Hits 7165 7176 +11
Misses 472 472
- Partials 463 466 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nullaway/src/test/java/com/uber/nullaway/CoreTests.java (1)
1176-1191: Pre-wire the expected diagnostic for the Iterable case (kept @ignore).When you un-ignore this, assert the unboxing diagnostic so the intent is explicit.
@Ignore @Test public void unboxForEachLoopIterable() { defaultCompilationHelper .addSourceLines( "Test.java", "import org.jspecify.annotations.*;", "import java.util.List;", "@NullMarked", "class Test {", " void f(List<@Nullable Integer> numbers) {", + " // BUG: Diagnostic contains: unboxing of a @Nullable value", " for (int x : numbers) {}", " }", "}") .doTest(); }If you want, I can implement the Iterable path in NullAway.java to make this pass (inspect generic element nullness in JSpecify mode).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(2 hunks)nullaway/src/test/java/com/uber/nullaway/CoreTests.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/NullAway.java
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/CoreTests.java
🧬 Code graph analysis (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java (1)
NullabilityUtil(66-665)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on macos-latest
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build spring-framework with snapshot
🔇 Additional comments (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
95-95: LGTM: import is correct and used.ArrayType is required for component-type inspection in the new for-each logic.
nullaway/src/test/java/com/uber/nullaway/CoreTests.java (1)
26-26: LGTM: Ignore import is fine.Used by the iterable test below.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| if (mayBeNullExpr(state, expr)) { | ||
| return errorBuilder.createErrorDescription(errorMessage, buildDescription(expr), state, null); | ||
| } | ||
| // auto-unboxing check |
There was a problem hiding this comment.
before doing this check, we should bail out if config.isJSpecifyMode() returns false. We only want to do this check in JSpecify mode
| return Description.NO_MATCH; | ||
| } | ||
| Type expressionType = ASTHelpers.getType(expr); | ||
| boolean isElementNullable = false; |
There was a problem hiding this comment.
I don't think we need this variable. Just move the logic for reporting the error within the innermost if
| "class Test {", | ||
| " void f(@Nullable Integer[] array) {", | ||
| " // BUG: Diagnostic contains:unboxing of a @Nullable value", | ||
| " for (int x : array) {}", |
There was a problem hiding this comment.
Please add a negative test, to ensure we don't get a warning when unboxing elements of an array that doesn't have @Nullable contents
| .doTest(); | ||
| } | ||
|
|
||
| // @Ignore |
| @Ignore | ||
| @Test | ||
| public void unboxForEachLoopIterable() { | ||
| defaultCompilationHelper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import org.jspecify.annotations.*;", | ||
| "import java.util.List;", | ||
| "@NullMarked", | ||
| "class Test {", | ||
| " void f(List<@Nullable Integer> numbers) {", | ||
| " for (int x : numbers) {}", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
Delete this new test. We'll add it later when we handle iterables
|
|
||
| // @Ignore | ||
| @Test | ||
| public void unboxForEachLoop() { |
There was a problem hiding this comment.
This test belongs in JSpecifyArrayTests. It should only report an error in JSpecify mode.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nullaway/src/main/java/com/uber/nullaway/NullAway.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
| @Test | ||
| public void unboxForEachLoop() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Outer.java", | ||
| "import org.jspecify.annotations.*;", | ||
| "@NullMarked", | ||
| "class Test {", | ||
| " void f(@Nullable Integer[] array) {", | ||
| " // BUG: Diagnostic contains:unboxing of a @Nullable value", | ||
| " for (int x : array) {}", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
Add package declaration so NullAway runs; fix BUG marker spacing
These snippets lack package com.uber;. Since the helper sets -XepOpt:NullAway:AnnotatedPackages=com.uber, the analyzer won’t run on default-package sources and the expected diagnostic won’t be produced. Also, keep the “BUG: Diagnostic contains:” formatting consistent with existing tests (space after colon).
"Outer.java",
- "import org.jspecify.annotations.*;",
+ "package com.uber;",
+ "import org.jspecify.annotations.*;",
"@NullMarked",
"class Test {",
" void f(@Nullable Integer[] array) {",
- " // BUG: Diagnostic contains:unboxing of a @Nullable value",
+ " // BUG: Diagnostic contains: unboxing of a @Nullable value",
" for (int x : array) {}",
" }",
"}")Optional: for consistency with the rest of this class, consider naming the file "Test.java" instead of "Outer.java".
📝 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.
| @Test | |
| public void unboxForEachLoop() { | |
| makeHelper() | |
| .addSourceLines( | |
| "Outer.java", | |
| "import org.jspecify.annotations.*;", | |
| "@NullMarked", | |
| "class Test {", | |
| " void f(@Nullable Integer[] array) {", | |
| " // BUG: Diagnostic contains:unboxing of a @Nullable value", | |
| " for (int x : array) {}", | |
| " }", | |
| "}") | |
| .doTest(); | |
| } | |
| @Test | |
| public void unboxForEachLoop() { | |
| makeHelper() | |
| .addSourceLines( | |
| "Outer.java", | |
| "package com.uber;", | |
| "import org.jspecify.annotations.*;", | |
| "@NullMarked", | |
| "class Test {", | |
| " void f(@Nullable Integer[] array) {", | |
| " // BUG: Diagnostic contains: unboxing of a @Nullable value", | |
| " for (int x : array) {}", | |
| " }", | |
| "}") | |
| .doTest(); | |
| } |
🤖 Prompt for AI Agents
In nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
around lines 675-689, the test source is in the default package so NullAway
won't analyze it; add the package declaration "package com.uber;" at the top of
the generated source string so the test is in the AnnotatedPackages scope, and
adjust the BUG marker to include a space after the colon ("// BUG: Diagnostic
contains: unboxing of a @Nullable value") to match existing test formatting;
optionally rename the virtual filename from "Outer.java" to "Test.java" for
consistency.
There was a problem hiding this comment.
The package is not necessary. Since the class is marked as @NullMarked, NullAway will run.
There was a problem hiding this comment.
@msridhar Thank you for the correction! You're absolutely right. Looking at the retrieved learnings, I can see that classes annotated with @NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration - the @NullMarked annotation guarantees NullAway analysis.
So the package declaration is indeed unnecessary. However, the spacing issue in the BUG marker is still worth fixing for consistency:
- " // BUG: Diagnostic contains:unboxing of a @Nullable value",
+ " // BUG: Diagnostic contains: unboxing of a @Nullable value",This matches the formatting used throughout the rest of the test file.
🧠 Learnings used
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Learnt from: msridhar
PR: uber/NullAway#1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.
Learnt from: msridhar
PR: uber/NullAway#1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
| @Test | ||
| public void unboxForEachLoopNonNull() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Outer.java", | ||
| "import org.jspecify.annotations.*;", | ||
| "@NullMarked", | ||
| "class Test {", | ||
| " void f(Integer[] array) {", | ||
| " for (int x : array) {}", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
Add package declaration so the negative test is analyzed
Same package issue here; without package com.uber; this compiles outside annotated packages and may mask regressions.
"Outer.java",
- "import org.jspecify.annotations.*;",
+ "package com.uber;",
+ "import org.jspecify.annotations.*;",
"@NullMarked",
"class Test {",
" void f(Integer[] array) {",
" for (int x : array) {}",
" }",
"}")If the implementation also intends to cover enhanced-for over Iterable<@Nullable Integer>, add a companion test to lock that behavior in.
📝 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.
| @Test | |
| public void unboxForEachLoopNonNull() { | |
| makeHelper() | |
| .addSourceLines( | |
| "Outer.java", | |
| "import org.jspecify.annotations.*;", | |
| "@NullMarked", | |
| "class Test {", | |
| " void f(Integer[] array) {", | |
| " for (int x : array) {}", | |
| " }", | |
| "}") | |
| .doTest(); | |
| } | |
| @Test | |
| public void unboxForEachLoopNonNull() { | |
| makeHelper() | |
| .addSourceLines( | |
| "Outer.java", | |
| "package com.uber;", | |
| "import org.jspecify.annotations.*;", | |
| "@NullMarked", | |
| "class Test {", | |
| " void f(Integer[] array) {", | |
| " for (int x : array) {}", | |
| " }", | |
| "}") | |
| .doTest(); | |
| } |
🤖 Prompt for AI Agents
In nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
around lines 691 to 704, the test source lacks a package declaration which
causes it to compile outside the annotated package and may hide regressions; add
"package com.uber;" as the first line inside the added source so the negative
test is analyzed in the intended annotated package, and if you also want to lock
behavior for enhanced-for over Iterable<@Nullable Integer>, add a companion test
method that supplies an Iterable<@Nullable Integer> source and asserts the same
unboxing/for-each behavior.
The core of the solution is a new check that activates when the loop variable is a primitive type
For arrays (e.g., @nullable Integer[]), the existing NullabilityUtil.isArrayElementNullable utility is used to determine if the elements can be null.
Summary by CodeRabbit
New Features
Tests