Fix library modeling for varargs arrays#1485
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds JSpecify generics checks in NullAway.handleInvocation for annotated method calls and introduces a varargs-aware override path used during invocation-time nullability checks. A new handler hook, onOverrideMethodInvocationVarargsArrayNullability, was added to Handler and implemented in CompositeHandler, LibraryModelsHandler, and RestrictiveAnnotationHandler; NullAway now uses the hook to derive varargs-array nullability and decide per-argument may-be-null checks. Library models, tests, and nullability utilities were updated to support modeling of varargs-array vs. varargs-element nullability. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java`:
- Around line 273-287: The nested-annotation mappings use the wrong owner class:
the methodRef entries for "nonNullContents(java.lang.String...)" and
"bothNonNull(java.lang.String...)" refer to
com.uber.lib.unannotated.NullMarkedVarargsWithModel but the actual methods are
declared on com.uber.lib.unannotated.NullUnmarkedVarargsWithModel; update those
two methodRef owner strings to
"com.uber.lib.unannotated.NullUnmarkedVarargsWithModel" so the
NestedAnnotationInfo entries apply to the correct methods (look for the
methodRef calls creating mappings for nonNullContents and bothNonNull).
In
`@test-library-models/src/test/java/com/uber/nullaway/VarargsLibraryModelsTests.java`:
- Around line 60-61: Re-enable the suppressed regression test by removing the
`@Ignore` annotation on the jspecifyModeRestrictive method so CI will run
jspecifyModeRestrictive; locate the test method named jspecifyModeRestrictive in
VarargsLibraryModelsTests and delete the `@Ignore`("Temporarily disabled while
restrictive varargs library-model handling is fixed") line (or replace it with
no-op annotation if needed) so the test is executed during PR validation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
nullaway/src/main/java/com/uber/nullaway/NullAway.javatest-java-lib/src/main/java/com/uber/lib/unannotated/NullMarkedVarargsWithModel.javatest-java-lib/src/main/java/com/uber/lib/unannotated/NullUnmarkedVarargsWithModel.javatest-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.javatest-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.javatest-library-models/src/test/java/com/uber/nullaway/VarargsLibraryModelsTests.java
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1485 +/- ##
============================================
- Coverage 88.43% 88.40% -0.03%
- Complexity 2790 2799 +9
============================================
Files 99 99
Lines 9279 9360 +81
Branches 1872 1880 +8
============================================
+ Hits 8206 8275 +69
- Misses 524 529 +5
- Partials 549 556 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2147-2157: The code currently initializes argIsNonNull only from
modeledParamNullness which lets individually-passed varargs skip element
non-null checks when the slot is unknown; change the initialization of
argIsNonNull (near methodSymbol, argPos, formalParams, varArgsPassedAsArray,
modeledParamNullness, formalParamType) to consider nested element annotations
for the varargs case too: set argIsNonNull to true if modeledParamNullness ==
Nullness.NONNULL OR (isVarargsArg && !varArgsPassedAsArray &&
!Nullness.hasNullableAnnotation(formalParamType.getAnnotationMirrors().stream(),
config)); remove or adjust the later block that unconditionally sets
argIsNonNull = false for nullable nested annotations so the combined
initialization handles both modeled slot nullness and nested element annotations
correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/Handler.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javatest-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2146-2157:⚠️ Potential issue | 🟠 MajorIndividually passed varargs can still bypass required element null checks.
At Line 2148,
argIsNonNullis initialized from slot nullness. When that slot is nullable/unknown for varargs-array modeling, individually passed varargs elements can skip checks even if the element type should be treated non-null. This is the same gap previously called out.🔧 Proposed fix
- boolean argIsNonNull = Objects.equals(Nullness.NONNULL, modeledParamNullness); - if (isVarargsArg && !varArgsPassedAsArray) { - // Individually passed varargs arguments correspond to the element type, not the array - // itself. Nested type annotations (including library models) are reflected on - // formalParamType via InvocationArguments. - if (Nullness.hasNullableAnnotation( - formalParamType.getAnnotationMirrors().stream(), config)) { - argIsNonNull = false; - } - } + boolean argIsNonNull = Objects.equals(Nullness.NONNULL, modeledParamNullness); + if (isVarargsArg && !varArgsPassedAsArray) { + // Individually passed varargs map to element nullability, not array nullability. + boolean elementIsNullable = + Nullness.hasNullableAnnotation( + formalParamType.getAnnotationMirrors().stream(), config); + argIsNonNull = + !elementIsNullable + && (isMethodAnnotated || Objects.equals(Nullness.NONNULL, modeledParamNullness)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java` around lines 2146 - 2157, The code currently sets argIsNonNull from finalArgumentPositionNullness[argPos] which is the array slot for varargs; when isVarargsArg && !varArgsPassedAsArray this can skip element null checks. Change the logic in the varargs branch (around modeledParamNullness, isVarargsArg, varArgsPassedAsArray, argIsNonNull) to determine element nullness instead of using the array slot: when individually passed varargs (isVarargsArg && !varArgsPassedAsArray) compute argIsNonNull by inspecting the element type's annotations (e.g., check formalParamType's component/element type annotations via formalParamType.getComponentType() or the InvocationArguments-modeled element nullness) and/or consult a corresponding element slot in finalArgumentPositionNullness if available; only set argIsNonNull = true when that element-level nullness is NONNULL and no nullable annotation is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2146-2157: The code currently sets argIsNonNull from
finalArgumentPositionNullness[argPos] which is the array slot for varargs; when
isVarargsArg && !varArgsPassedAsArray this can skip element null checks. Change
the logic in the varargs branch (around modeledParamNullness, isVarargsArg,
varArgsPassedAsArray, argIsNonNull) to determine element nullness instead of
using the array slot: when individually passed varargs (isVarargsArg &&
!varArgsPassedAsArray) compute argIsNonNull by inspecting the element type's
annotations (e.g., check formalParamType's component/element type annotations
via formalParamType.getComponentType() or the InvocationArguments-modeled
element nullness) and/or consult a corresponding element slot in
finalArgumentPositionNullness if available; only set argIsNonNull = true when
that element-level nullness is NONNULL and no nullable annotation is present.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2114-2125:⚠️ Potential issue | 🟠 MajorIndividually passed varargs can still bypass element non-null checks.
At Line 2116,
argIsNonNullis seeded only from slot nullness. When slot nullness is unknown/nullable (array-level modeling), individually passed varargs elements may skip required non-null checks.Suggested fix
Nullness modeledParamNullness = finalArgumentPositionNullness[argPos]; boolean isVarargsArg = methodSymbol.isVarArgs() && argPos == formalParams.size() - 1; boolean argIsNonNull = Objects.equals(Nullness.NONNULL, modeledParamNullness); if (isVarargsArg && !varArgsPassedAsArray) { // Individually passed varargs arguments correspond to the element type, not the array // itself. Nested type annotations (including library models) are reflected on // formalParamType via InvocationArguments. - if (Nullness.hasNullableAnnotation( - formalParamType.getAnnotationMirrors().stream(), config)) { - argIsNonNull = false; - } + boolean elementIsNullable = + Nullness.hasNullableAnnotation( + formalParamType.getAnnotationMirrors().stream(), config); + if (modeledParamNullness == null) { + argIsNonNull = !elementIsNullable && !formalParamType.getAnnotationMirrors().isEmpty(); + } else if (Objects.equals(modeledParamNullness, Nullness.NULLABLE)) { + argIsNonNull = isMethodAnnotated && !elementIsNullable; + } else if (elementIsNullable) { + argIsNonNull = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java` around lines 2114 - 2125, The current seeding of argIsNonNull from finalArgumentPositionNullness can miss element-level NONNULL for individually passed varargs; update the varargs branch in the isVarargsArg && !varArgsPassedAsArray case (around modeledParamNullness, argIsNonNull, varArgsPassedAsArray, formalParamType, finalArgumentPositionNullness) to also inspect the varargs element type's annotations (the component/nested element reflected by InvocationArguments/formalParamType) and set argIsNonNull = true when that element type has a NONNULL annotation even if the array-level slot was unknown/nullable; keep the existing check that sets argIsNonNull = false when a nullable annotation is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java`:
- Around line 225-250: The method
onOverrideMethodInvocationVarargsArrayNullability documents that null is a valid
sentinel for both the varargsArrayNullness parameter and the return value but
the signature is non-nullable; update the API by annotating the
varargsArrayNullness parameter and the method return type as `@Nullable` to match
the Javadoc (and add the appropriate `@Nullable` import used across the codebase),
keeping the method name Context and Symbol.MethodSymbol types unchanged so
handlers can accept and return null as documented.
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2114-2125: The current seeding of argIsNonNull from
finalArgumentPositionNullness can miss element-level NONNULL for individually
passed varargs; update the varargs branch in the isVarargsArg &&
!varArgsPassedAsArray case (around modeledParamNullness, argIsNonNull,
varArgsPassedAsArray, formalParamType, finalArgumentPositionNullness) to also
inspect the varargs element type's annotations (the component/nested element
reflected by InvocationArguments/formalParamType) and set argIsNonNull = true
when that element type has a NONNULL annotation even if the array-level slot was
unknown/nullable; keep the existing check that sets argIsNonNull = false when a
nullable annotation is present.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2114-2125:⚠️ Potential issue | 🟠 MajorIndividually passed varargs can still skip required non-null element checks when slot nullness is unknown.
At Line [2116],
argIsNonNullis initialized only frommodeledParamNullness. For individually passed varargs, the block at Line [2121]-Line [2124] only turns checks off for nullable elements; it never turns checks on when element metadata is explicitly non-null andmodeledParamNullnessisnull.Suggested fix
- boolean argIsNonNull = Objects.equals(Nullness.NONNULL, modeledParamNullness); + boolean argIsNonNull = Objects.equals(Nullness.NONNULL, modeledParamNullness); if (isVarargsArg && !varArgsPassedAsArray) { // Individually passed varargs arguments correspond to the element type, not the array // itself. Nested type annotations (including library models) are reflected on // formalParamType via InvocationArguments. - if (Nullness.hasNullableAnnotation( - formalParamType.getAnnotationMirrors().stream(), config)) { - argIsNonNull = false; - } + boolean elementIsNullable = + Nullness.hasNullableAnnotation( + formalParamType.getAnnotationMirrors().stream(), config); + if (modeledParamNullness == null) { + // Slot nullness can be intentionally unknown for varargs; fall back to explicit + // element-level metadata when present on the adapted formal type. + argIsNonNull = !elementIsNullable && !formalParamType.getAnnotationMirrors().isEmpty(); + } else if (elementIsNullable) { + argIsNonNull = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java` around lines 2114 - 2125, The code initializes argIsNonNull from modeledParamNullness but for individually passed varargs (isVarargsArg && !varArgsPassedAsArray) it only clears non-nullness when a nullable annotation is present, so non-null element annotations on formalParamType are ignored; update the varargs branch in NullAway.java around modeledParamNullness/argIsNonNull so that when isVarargsArg and the formalParamType contains an explicit non-null annotation (use Nullness.hasNullableAnnotation and its complement or equivalent check on formalParamType.getAnnotationMirrors().stream(), config), you set argIsNonNull = true (rather than leaving it null), while still setting argIsNonNull = false when a nullable annotation is present, so finalArgumentPositionNullness handling correctly enforces non-null checks for individually passed varargs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java`:
- Line 146: Remove the stale commented-out condition in
RestrictiveAnnotationHandler: delete the commented fragment "&&
!NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(varargsParamSymbol)"
(the dead comment at the varargs/array override check) so the varargs-array
override logic is not obscured; update the surrounding comment if needed to
reflect the current behavior and run tests to confirm no behavioral changes.
In `@nullaway/src/main/java/com/uber/nullaway/Nullness.java`:
- Around line 315-320: The method varargsArrayIsNonNull performs an unchecked
cast of paramSymbol to Symbol.VarSymbol when calling
NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation; guard this by checking
the symbol type first (e.g., use instanceof or symbol.kind/type check) and only
call NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation((Symbol.VarSymbol)
paramSymbol) when paramSymbol is actually a VarSymbol, otherwise skip that
clause (return false for that branch). Update varargsArrayIsNonNull to perform
the safe type check before casting to avoid ClassCastException.
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2114-2125: The code initializes argIsNonNull from
modeledParamNullness but for individually passed varargs (isVarargsArg &&
!varArgsPassedAsArray) it only clears non-nullness when a nullable annotation is
present, so non-null element annotations on formalParamType are ignored; update
the varargs branch in NullAway.java around modeledParamNullness/argIsNonNull so
that when isVarargsArg and the formalParamType contains an explicit non-null
annotation (use Nullness.hasNullableAnnotation and its complement or equivalent
check on formalParamType.getAnnotationMirrors().stream(), config), you set
argIsNonNull = true (rather than leaving it null), while still setting
argIsNonNull = false when a nullable annotation is present, so
finalArgumentPositionNullness handling correctly enforces non-null checks for
individually passed varargs.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/Nullness.javanullaway/src/main/java/com/uber/nullaway/handlers/Handler.javanullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java
| 1, | ||
| new NestedAnnotationInfo( | ||
| Annotation.NULLABLE, ImmutableList.of(new TypePathEntry(TYPE_ARGUMENT, 0))))) | ||
| .put( |
There was a problem hiding this comment.
Relevant changes start here. The rest of the changes stem from having to switch from an of() constructor to using a builder
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.JUnit4; | ||
|
|
||
| @RunWith(JUnit4.class) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 167-176: The loop in LibraryModelsHandler that currently picks the
first ARRAY_ELEMENT nestedAnnotation (iterating
nestedAnnotations.get(varargsIndex)) must instead scan all nestedAnnotation
entries for that ARRAY_ELEMENT typePath and detect if both Annotation.NULLABLE
and NONNULL element annotations are present; implement logic in the block
handling ARRAY_ELEMENT to track seenNullable and seenNonNull flags (using
nestedAnnotation.annotation(), Annotation.NULLABLE, NULLABLE, NONNULL) and after
the loop set argumentPositionNullness[varargsIndex] only if exactly one kind is
present, otherwise surface a model conflict (e.g., throw or report a clear error
mentioning varargsIndex and the conflicting nested models) so conflicting
varargs-element models are detected rather than first-match winning.
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2123-2132: The local variable varargsArrayNullness is assigned
null in some branches but not explicitly declared nullable; update its
declaration to make nullability explicit (e.g., annotate or declare as
`@Nullable/nullable` type) to improve readability and intent. Locate the
declaration and initialization around
Nullness.varargsArrayIsNullable(varargsFormalParam, config) and ensure the
variable holding that result (varargsArrayNullness) is explicitly nullable
before it is passed into
handler.onOverrideMethodInvocationVarargsArrayNullability(state.context,
methodSymbol, isMethodAnnotated, varargsArrayNullness).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f297f02-3c36-403d-bf16-9181514ea620
📒 Files selected for processing (7)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/Nullness.javanullaway/src/main/java/com/uber/nullaway/handlers/Handler.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.javatest-library-models/src/test/java/com/uber/nullaway/VarargsLibraryModelsTests.java
| Nullness varargsArrayNullness = | ||
| (isMethodAnnotated && !Nullness.varargsArrayIsNullable(varargsFormalParam, config)) | ||
| ? Nullness.NONNULL | ||
| : null; | ||
| // 2. For @NullUnmarked methods, there should be an explicit @NonNull annotation. This | ||
| // is handled by the RestrictiveAnnotationHandler | ||
| // 3. Library model indicating it is @NonNull (handled in LibraryModelsHandler) | ||
| varargsArrayNullness = | ||
| handler.onOverrideMethodInvocationVarargsArrayNullability( | ||
| state.context, methodSymbol, isMethodAnnotated, varargsArrayNullness); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make varargsArrayNullness explicitly nullable for readability.
At Line [2123], the local can be null; annotating it improves intent clarity and avoids future confusion.
♻️ Suggested refactor
- Nullness varargsArrayNullness =
+ `@Nullable` Nullness varargsArrayNullness =
(isMethodAnnotated && !Nullness.varargsArrayIsNullable(varargsFormalParam, config))
? Nullness.NONNULL
: null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java` around lines 2123 -
2132, The local variable varargsArrayNullness is assigned null in some branches
but not explicitly declared nullable; update its declaration to make nullability
explicit (e.g., annotate or declare as `@Nullable/nullable` type) to improve
readability and intent. Locate the declaration and initialization around
Nullness.varargsArrayIsNullable(varargsFormalParam, config) and ensure the
variable holding that result (varargsArrayNullness) is explicitly nullable
before it is passed into
handler.onOverrideMethodInvocationVarargsArrayNullability(state.context,
methodSymbol, isMethodAnnotated, varargsArrayNullness).
There was a problem hiding this comment.
NullAway infers the nullability of local variables; they should not be annotated
There was a problem hiding this comment.
@msridhar, thanks for the clarification! That makes sense — since NullAway itself infers the nullability of local variables, annotating them is unnecessary. I'll retract the suggestion.
✏️ Learnings added
Learnt from: msridhar
Repo: uber/NullAway PR: 1485
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:2119-2128
Timestamp: 2026-03-20T15:34:10.657Z
Learning: In NullAway's own codebase (uber/NullAway), local variables should NOT be annotated with `Nullable` or `NonNull`. NullAway infers the nullability of local variables automatically, so adding such annotations to locals is incorrect style for this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: msridhar
Repo: uber/NullAway PR: 1464
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:610-621
Timestamp: 2026-02-15T00:29:58.053Z
Learning: In NullAway's codebase, the `InvocationArguments.forEach` method has special-case handling for varargs and other edge cases, so it should not be replaced with a simple indexed loop. Using `forEach` with an `AtomicReference` or similar pattern is appropriate when you need to find a specific argument and its corresponding formal parameter type.
Learnt from: msridhar
Repo: uber/NullAway PR: 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.
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Learnt from: msridhar
Repo: uber/NullAway PR: 1421
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:339-340
Timestamp: 2025-12-31T23:59:20.009Z
Learning: In Error Prone's ASTHelpers class, the getSymbol(MethodInvocationTree) overload is guaranteed to return a non-null Symbol.MethodSymbol. This differs from other overloads of getSymbol() that may return null. Therefore, no null check is required after calling ASTHelpers.getSymbol() with a MethodInvocationTree parameter.
Learnt from: msridhar
Repo: uber/NullAway PR: 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
Repo: uber/NullAway PR: 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.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
163-176:⚠️ Potential issue | 🟠 MajorDetect conflicting varargs-element nested models instead of first-match wins.
At Line 175, the
breakmakes behavior order-dependent when both nullable and non-nullARRAY_ELEMENTmodels are present for the same varargs parameter. This can silently mask an invalid model merge and produce unstable nullability decisions.Suggested fix
if (varArgsMethod) { ImmutableSetMultimap<Integer, NestedAnnotationInfo> nestedAnnotations = optimizedLibraryModels.nestedAnnotationsForMethods(methodSymbol); int varargsIndex = methodSymbol.getParameters().size() - 1; + `@Nullable` Nullness varargsElementNullness = null; for (NestedAnnotationInfo nestedAnnotation : nestedAnnotations.get(varargsIndex)) { // check if it's for the array element if (nestedAnnotation.typePath().size() == 1 && nestedAnnotation.typePath().get(0).kind() == ARRAY_ELEMENT) { - // this is a nested annotation for the varargs array element, which applies when - // individual parameters are passed - argumentPositionNullness[varargsIndex] = - nestedAnnotation.annotation() == Annotation.NULLABLE ? NULLABLE : NONNULL; - break; + Nullness candidate = + nestedAnnotation.annotation() == Annotation.NULLABLE ? NULLABLE : NONNULL; + if (varargsElementNullness != null && varargsElementNullness != candidate) { + throw new IllegalStateException( + String.format( + "Library models give conflicting varargs element nullability for method %s", + methodSymbol.getQualifiedName())); + } + varargsElementNullness = candidate; } } + if (varargsElementNullness != null) { + argumentPositionNullness[varargsIndex] = varargsElementNullness; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java` around lines 163 - 176, The current varargs handling uses a first-match break and can hide conflicting ARRAY_ELEMENT nested annotations; instead iterate all entries from optimizedLibraryModels.nestedAnnotationsForMethods(methodSymbol).get(varargsIndex), filter those whose nestedAnnotation.typePath() has size 1 and kind() == ARRAY_ELEMENT, and collect their nestedAnnotation.annotation() values (compare to Annotation.NULLABLE/ NONNULL). If you observe both NULLABLE and NONNULL for the same varargsIndex, report a model merge/conflict (use the existing model error reporting path) rather than picking the first; otherwise set argumentPositionNullness[varargsIndex] to NULLABLE or NONNULL based on the single observed annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 163-176: The current varargs handling uses a first-match break and
can hide conflicting ARRAY_ELEMENT nested annotations; instead iterate all
entries from
optimizedLibraryModels.nestedAnnotationsForMethods(methodSymbol).get(varargsIndex),
filter those whose nestedAnnotation.typePath() has size 1 and kind() ==
ARRAY_ELEMENT, and collect their nestedAnnotation.annotation() values (compare
to Annotation.NULLABLE/ NONNULL). If you observe both NULLABLE and NONNULL for
the same varargsIndex, report a model merge/conflict (use the existing model
error reporting path) rather than picking the first; otherwise set
argumentPositionNullness[varargsIndex] to NULLABLE or NONNULL based on the
single observed annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 954ca131-ab46-4845-a5bb-bd0b642455cd
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
|
This makes sense to me, and I'll review the actual code as soon as I can:
But I also want to bring up an option, architecturally, which would have been to return a more complex value out of Edit: Ah, I think I see the issue. We do the above for other kinds of generic nullability, but var args is somehow different because the actuals are directly annotated types at the top-level whereas the formal is the var args array with a more complicated type? (Not sure I am wording this correctly, will have to read this closer tomorrow) |
I agree this design is kind of gross. Really, all we should need as of now is the What do you think? |
Actually, more than happy to do it as two steps. Since that way the big method change becomes a "pure" refactor, without new or changing tests or library models. |
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
Actually, got a chance to look at it today. Might need a more careful glance at the logic in NullAway.java itself, since I ended up rushing a bit there, but I feel reasonably convinced of everything else.
|
|
||
| InvocationArguments invArgs = new InvocationArguments(tree, methodSymbol.type.asMethodType()); | ||
| Type.MethodType invokedMethodType = | ||
| handler.onOverrideMethodType(methodSymbol, methodSymbol.type.asMethodType(), state); |
There was a problem hiding this comment.
Not sure I understand. Is this change also related to var args library models handling? This handler extension point hasn't changed and I see no other changes to our handling of invArgs. What behavior does this change modify?
There was a problem hiding this comment.
Yeah, this change is unnecessary. I will undo it.
| * @param methodSymbol The method symbol for the method in question. | ||
| * @param isAnnotated A boolean flag indicating whether the called method is annotated | ||
| * @param varargsArrayNullness current value for the varargs array nullability, or {@code null} if | ||
| * none |
There was a problem hiding this comment.
We might want to copy the language of the other handler method to the effect of:
Some entries may be {@code null}, indicating upstream handlers and the base analysis consider the parameter to be nullness-unknown,
here. I got confused when originally looking at NullAway.java on this point 😉
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
167-185:⚠️ Potential issue | 🟠 MajorUnreachable throw - conflict detection for varargs contents is incomplete.
The throw at lines 179-181 is dead code. The control flow is:
varargsContentsNullnessstarts asnull(line 167)- When first
ARRAY_ELEMENTis found: condition at line 174 is true → set value (lines 175-177)- Immediately
breakat line 183The
elsebranch (throw) can never execute because we break after the firstARRAY_ELEMENT. If library models contain conflicting element annotations (both@Nullableand@NonNullfor the same varargs element), the first one wins silently.Proposed fix to properly detect conflicts
if (varArgsMethod) { ImmutableSetMultimap<Integer, NestedAnnotationInfo> nestedAnnotations = optimizedLibraryModels.nestedAnnotationsForMethods(methodSymbol); int varargsIndex = methodSymbol.getParameters().size() - 1; Nullness varargsContentsNullness = null; for (NestedAnnotationInfo nestedAnnotation : nestedAnnotations.get(varargsIndex)) { // check if it's for the array element if (nestedAnnotation.typePath().size() == 1 && nestedAnnotation.typePath().get(0).kind() == ARRAY_ELEMENT) { // this is a nested annotation for the varargs array element, which applies when // individual parameters are passed - if (varargsContentsNullness == null) { - varargsContentsNullness = - nestedAnnotation.annotation() == Annotation.NULLABLE ? NULLABLE : NONNULL; - argumentPositionNullness[varargsIndex] = varargsContentsNullness; - } else { + Nullness candidate = + nestedAnnotation.annotation() == Annotation.NULLABLE ? NULLABLE : NONNULL; + if (varargsContentsNullness != null && varargsContentsNullness != candidate) { throw new IllegalStateException( "varargs contents nullness set multiple times: " + nestedAnnotations.get(varargsIndex)); } - break; + varargsContentsNullness = candidate; } } + if (varargsContentsNullness != null) { + argumentPositionNullness[varargsIndex] = varargsContentsNullness; + } }Note: The conflict detection for the varargs array itself (in
onOverrideMethodInvocationVarargsArrayNullabilityat lines 204-209) is correctly implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java` around lines 167 - 185, The loop over nestedAnnotations.get(varargsIndex) currently breaks on the first ARRAY_ELEMENT match so conflicting annotations can never be detected; update the logic in LibraryModelsHandler to scan all NestedAnnotationInfo entries for the varargsIndex: when nestedAnnotation.typePath().get(0).kind() == ARRAY_ELEMENT, if varargsContentsNullness is null set it (and set argumentPositionNullness[varargsIndex]) otherwise compare the new annotation's nullness to varargsContentsNullness and throw an IllegalStateException if they differ (do not break on the first match), ensuring conflicting `@Nullable/`@NonNull for the varargs element are detected; keep existing behavior for onOverrideMethodInvocationVarargsArrayNullability unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 167-185: The loop over nestedAnnotations.get(varargsIndex)
currently breaks on the first ARRAY_ELEMENT match so conflicting annotations can
never be detected; update the logic in LibraryModelsHandler to scan all
NestedAnnotationInfo entries for the varargsIndex: when
nestedAnnotation.typePath().get(0).kind() == ARRAY_ELEMENT, if
varargsContentsNullness is null set it (and set
argumentPositionNullness[varargsIndex]) otherwise compare the new annotation's
nullness to varargsContentsNullness and throw an IllegalStateException if they
differ (do not break on the first match), ensuring conflicting
`@Nullable/`@NonNull for the varargs element are detected; keep existing behavior
for onOverrideMethodInvocationVarargsArrayNullability unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77712f69-ad92-44ac-a00c-fd91f6ba7179
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/Handler.javanullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
2 participants
Fixes #1481, which should enable us to handle #1480
This gives us the ability to model more nullability scenarios for varargs arrays via library models. For example, we can now write a library model for a method like
Objects.hash(@Nullable Object @Nullable ...)that can accept both a@Nullablearray argument and@Nullableindividual arguments at call sites.To do this, we had to more cleanly separate library modeling of the nullability of the varargs array itself versus its contents. To this end, we add a new
Handler.onOverrideMethodInvocationVarargsArrayNullabilitymethod for modeling the nullability of the array. Now,Handler.onOverrideMethodInvocationParametersNullabilityonly handles the nullability of the varargs array contents, i.e., whether individual@Nullablearguments can be passed at call sites. This separation and new method required fixes and handling inLibraryModelsHandlerandRestrictiveAnnotationHandler.Sorry for the size of this PR. Suggested guide to reviewing:
NullUnmarkedVarargsWithModelandNullMarkedVarargsWithModelTestLibraryModelsVarargsLibraryModelsTestsSummary by CodeRabbit
New Features
Bug Fixes
Tests