JSpecify: preserve array element nullability through requireNonNull - #1645
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughJSpecify array-write validation now derives array element nullability from the inferred expression type. Possibly related issues
Possibly related PRs
Suggested reviewers: Mergeability Score: ⚪ Minimal · up to The change preserves nullable array-element information through requireNonNull and adds regression coverage, preventing a false-positive diagnostic without introducing a supported merge-readiness concern; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for the contribution! A couple thoughts below
| } | ||
| boolean isElementNullable = | ||
| isArrayElementNullable(ASTHelpers.getType(arrayExpr), config) | ||
| || (arraySymbol != null && isArrayElementNullable(arraySymbol, config)); |
There was a problem hiding this comment.
I don't understand why we need this fallback? Does ASTHelpers.getType(arrayExpr) not return the right type for arrayExpr when it is a variable?
I wonder if for the general case, we need to expose a method from GenericsCheck (like getTreeType) to properly get the nullness-enhanced type of arrayExpr. I'm not sure we can rely on ASTHelpers.getType in general to preserve the right types, even if it works for the original test. E.g., does this case work?
@NullMarked
public class Test {
private char @Nullable [] @Nullable [] foo = null;
static <T extends @Nullable Object> T id(T t) { return t; }
void test() {
Objects.requireNonNull(id(foo))[0] = null;
}
}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1645 +/- ##
============================================
- Coverage 88.17% 88.16% -0.02%
- Complexity 3056 3060 +4
============================================
Files 105 105
Lines 10270 10275 +5
Branches 2072 2075 +3
============================================
+ Hits 9056 9059 +3
Misses 573 573
- Partials 641 643 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| isArrayElementNullable(arrayType, config) | ||
| || (arrayType == null | ||
| && arraySymbol != null | ||
| && isArrayElementNullable(arraySymbol, config)); |
There was a problem hiding this comment.
Sorry, I still don't understand why we need this fallback to checking the symbol, can you explain?
There was a problem hiding this comment.
Yeah, fair question. I don't think we need the fallback.
Dropped it and just use getTreeType for the element nullability check. Still covers requireNonNull(foo) and requireNonNull(id(foo)).
|
Another potential test case here: class Test {
void test() {
char @Nullable [][] x = new char @Nullable [][]{};
x[0] = null;
requireNonNull(new char @Nullable [][]{})[0] = null;
}
}Right now I get incorrect warnings for both |
|
@kamilkrzywanski are you still able to work on this one? |
msridhar
added
pending-response
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Posting a review to make clear we're waiting on a response to #1645 (comment)
|
@msridhar I'll try check this tomorrow |
c7c010d to
a52d773
Compare
|
@msridhar added tests for the multi-dim case you mentioned. For Let me know if you meant something else. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java`:
- Around line 861-863: Remove the top-level `@Nullable` annotations from the local
array-reference variables in JSpecifyArrayTests.java at lines 861-863, 887-889,
and 892-895. Move the nullable array-reference declarations to fields or
parameters while preserving the existing array-content nullability and test
behavior; NullAway should infer local-variable nullability.
- Around line 874-890: In nullableArrayOfNonNullInnerArrays and
nullableOuterElementsOf2dArray, swap the multidimensional array declarations so
a uses the layout with nullable outer elements and b uses the layout with
non-null char[] elements. Update the associated BUG diagnostic expectations
accordingly: a[0] assignments remain valid, while both b[0] assignments must
expect the nullable-write diagnostic.
🪄 Autofix
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 Plus
Run ID: 3b328fb0-741e-40d0-80bb-52e0e0d5f9aa
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
| // char @Nullable [][] : the array reference is @Nullable; elements (char[]) are | ||
| // @NonNull. Writing null into a[0] must still be reported. | ||
| void nullableArrayOfNonNullInnerArrays() { | ||
| char @Nullable [][] a = new char @Nullable [][]{}; | ||
| // BUG: Diagnostic contains: Writing @Nullable expression into array with @NonNull contents | ||
| a[0] = null; | ||
| // BUG: Diagnostic contains: Writing @Nullable expression into array with @NonNull contents | ||
| Objects.requireNonNull(new char @Nullable [][]{})[0] = null; | ||
| } | ||
|
|
||
| // char [] @Nullable [] : outer elements are @Nullable char[]. Writes of null OK. | ||
| // (Syntax suggested by reviewer for multi-dim element nullability.) | ||
| void nullableOuterElementsOf2dArray() { | ||
| char [] @Nullable [] b = new char [] @Nullable []{}; | ||
| b[0] = null; | ||
| Objects.requireNonNull(new char [] @Nullable []{})[0] = null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'char `@Nullable` \[\]\[\]|char \[\] `@Nullable` \[\]|requireNonNull\(new char' \
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.javaRepository: uber/NullAway
Length of output: 1440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- surrounding test ---'
sed -n '820,905p' nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java
printf '%s\n' '--- related multidimensional array cases ---'
rg -n -C 4 \
'Nullable.*\[\].*\[\]|NonNull.*\[\].*\[\]|2d|multi.?dim|array with' \
nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java \
nullaway/src/test/java/com/uber/nullaway/jspecify
printf '%s\n' '--- annotation declarations and array-type guidance in the repository ---'
rg -n -C 3 \
'char `@Nullable`|Object `@Nullable`|`@Nullable` \[\]|`@Nullable`\[\]|array reference|array component|array element' \
nullaway/src/test nullaway/src/main | head -250Repository: uber/NullAway
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only lexical verifier for Java's dimension-annotation placement.
# For a type written as T D1 D2, the leftmost dimension is the component
# array (the inner dimension), and the rightmost dimension is the enclosing
# array (the outer dimension).
types = {
"char `@Nullable` [][]": ["`@Nullable`", None],
"char [] `@Nullable` []": [None, "`@Nullable`"],
}
for source, dimensions in types.items():
inner, outer = dimensions
print(f"{source}")
print(f" inner char[] dimension annotation: {inner or '<none>'}")
print(f" outer enclosing-array dimension annotation: {outer or '<none>'}")
print(f" outer elements nullable: {inner == '`@Nullable`'}")
print(f" outer array reference nullable: {outer == '`@Nullable`'}")
PYRepository: uber/NullAway
Length of output: 536
Fix the inverted multidimensional array layouts.
char @nullable [][] has nullable outer elements, so both a[0] = null writes are valid. char [] @nullable [] has non-null char[] elements, so both b[0] = null writes must report the nullable-write diagnostic. Swap the declarations and expectations.
🤖 Prompt for 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.
In `@nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java`
around lines 874 - 890, In nullableArrayOfNonNullInnerArrays and
nullableOuterElementsOf2dArray, swap the multidimensional array declarations so
a uses the layout with nullable outer elements and b uses the layout with
non-null char[] elements. Update the associated BUG diagnostic expectations
accordingly: a[0] assignments remain valid, while both b[0] assignments must
expect the nullable-write diagnostic.
Source: Coding guidelines
|
@msridhar took another look:
Squashed into one commit. |
I got confused about the syntax 🤦♂️ You are correct; thanks! |
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Looks good! Just one minor comment. Also could you pull in the latest main branch and fix any merge conflicts?
| Version Next | ||
| ------------ | ||
|
|
||
| * JSpecify: Preserve array element nullability through method invocations like requireNonNull (Fixes #1521) by @kamilkrzywanski | ||
|
|
There was a problem hiding this comment.
Remove this? When I create the release notes I do it from the commit history, so this will automatically be added.
|
@kamilkrzywanski by any chance does this PR also fix #1150? |
When checking writes into array elements, take element nullability from the expression type via GenericsChecks.getTreeType instead of the array symbol. That keeps nullness through generic wrappers like requireNonNull (and nested cases such as requireNonNull(id(foo))), which previously lost annotations and produced false positives for nullable contents. Also cover multi-dimensional array annotation placement in tests. Fixes uber#1521
a52d773 to
d408152
Compare
3 participants
Fixes #1521.
In JSpecify mode, writing
nullinto an array element looks at the array expression's symbol to decide whether the contents are@Nullable. For an expression likeObjects.requireNonNull(foo), that symbol is the method, not the original array, so element annotations from the argument type were dropped and we reported a false positive:requireNonNullonly guarantees the outer array is non-null; the element type is stillchar @Nullable [].The fix also consults the expression type (which keeps element annotations after generic substitution), while still checking the symbol for cases like local variables where annotations may only be present there.
Added a regression test covering the reported case and a negative case with non-null element types.
I used AI assistance while investigating and implementing this fix. I reviewed the change, confirmed the test fails without it and passes with it, and ran
:nullaway:test.Summary by CodeRabbit
Objects.requireNonNullscenarios.