Handle nested restrictive annotations on type variables in @NullUnmarked code#1510
Handle nested restrictive annotations on type variables in @NullUnmarked code#1510officialasishkumar wants to merge 3 commits intouber:masterfrom
Conversation
…Unmarked code In JSpecify mode, @NullUnmarked methods with explicit @nonnull annotations on type parameters (e.g., Foo<@nonnull T>) were not enforcing those restrictions at call sites. This happened because the generics type parameter check was only performed for annotated methods. This change: - Moves compareGenericTypeParameterNullabilityForCall outside the isMethodAnnotated guard so it also runs for @NullUnmarked methods - Threads an isMethodAnnotated flag through the check pipeline so that for @NullUnmarked methods, only explicit restrictive annotations (like @nonnull) trigger errors, while unannotated type arguments remain unconstrained - Restores explicit nullability annotations from the original method type when generic method type inference fails, preventing loss of restrictive annotations in the inferred call-site type Fixes uber#1488 Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
|
|
|
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 (2)
👮 Files not reviewed due to content moderation or server errors (2)
WalkthroughThe PR changes JSpecify-mode generics nullability checks: NullAway now calls Possibly related PRs
Suggested labels
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: 3
🤖 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/generics/CheckIdenticalNullabilityVisitor.java`:
- Around line 84-95: The nullability mismatch gating logic (the
isMethodUnannotated && !isLHSNullableAnnotated &&
!Nullness.hasNonNullAnnotation(..., config) check) is duplicated; extract it
into a helper on CheckIdenticalNullabilityVisitor (e.g.,
shouldSkipUnannotatedMismatch or similar) and replace both occurrences (the
current block around the LHS/RHS check and the similar block at lines 128-135)
with calls to that helper; the helper should accept the lhsTypeArgument,
isMethodUnannotated flag, and config (or access config from the visitor) and
return a boolean indicating whether to skip reporting the mismatch so both class
and array paths share the same policy.
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1953-1958: Add a regression unit test that forces an
InferenceFailure in the GenericsChecks path and asserts that explicit
restrictive annotations are preserved by the fallback call to
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations(...). Create a
minimal unsatisfiable-inference example (e.g., conflicting type-argument
nullability constraints) that exercises the branch returning
restoreExplicitNullabilityAnnotations(methodType, methodTypeAtCallSite, config,
Collections.emptyMap()).asMethodType(), invoke the checker to trigger the
InferenceFailure path, and assert the resulting method type at call site still
carries the original `@NonNull/`@Nullable annotations on type arguments.
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2092-2097: compareGenericTypeParameterNullabilityForCall currently
applies explicit type-argument substitution only for MethodInvocationTree, so
generic constructor calls with explicit type arguments (NewClassTree) are
skipped; update compareGenericTypeParameterNullabilityForCall (or its shared
caller handleInvocation) to detect NewClassTree and extract/apply its explicit
type arguments the same way as for MethodInvocationTree (ensure substitution
logic used around lines handling diamond constructors is reused for
NewClassTree), and add a regression test that constructs a generic type via new
with explicit type arguments calling an `@NullUnmarked` constructor to prevent
future regressions.
🪄 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: 5962bc87-5d70-4562-a8c4-64df961b7108
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java
nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java
Show resolved
Hide resolved
| // inference failed; restore explicit nullability annotations from the original method | ||
| // type to the call-site type, so restrictive annotations (e.g. @NonNull on type | ||
| // arguments) are still enforced | ||
| return TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations( | ||
| methodType, methodTypeAtCallSite, config, Collections.emptyMap()) | ||
| .asMethodType(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression for the inference-failure fallback.
This branch now preserves explicit restrictive annotations after solver failure, but the PR’s listed tests do not appear to exercise an InferenceFailure. A small unsatisfiable-inference case would keep this from regressing.
🤖 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/generics/GenericsChecks.java` around
lines 1953 - 1958, Add a regression unit test that forces an InferenceFailure in
the GenericsChecks path and asserts that explicit restrictive annotations are
preserved by the fallback call to
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations(...). Create a
minimal unsatisfiable-inference example (e.g., conflicting type-argument
nullability constraints) that exercises the branch returning
restoreExplicitNullabilityAnnotations(methodType, methodTypeAtCallSite, config,
Collections.emptyMap()).asMethodType(), invoke the checker to trigger the
InferenceFailure path, and assert the resulting method type at call site still
carries the original `@NonNull/`@Nullable annotations on type arguments.
There was a problem hiding this comment.
Agreed that a dedicated inference-failure test would be ideal here. However, constructing a minimal unsatisfiable-inference scenario that reliably triggers InferenceFailure (rather than the solver just picking a fallback) is non-trivial and somewhat fragile since it depends on internal solver behavior.
The existing test nullUnmarkedRestrictiveAnnotationsOnTypeVariables does exercise the generic identity method path (which goes through substituteTypeArgsInGenericMethodType), and the change itself is conservative -- it restores explicit annotations from the original method type, which is the same pattern used in the success path and elsewhere in the codebase (e.g., lines 563, 570, 2034). Previously, inference failure silently dropped all explicit annotations, so this is strictly an improvement.
I will look into adding a targeted inference-failure test as a follow-up if I can find a reliable way to trigger that path. For now, the change is safe because restoreExplicitNullabilityAnnotations is well-tested through other code paths.
There was a problem hiding this comment.
@officialasishkumar, that's a reasonable explanation. The UnsatisfiableConstraintsException path is indeed hard to trigger in a controlled way from a black-box test since it depends on the internal solver's constraint resolution, and a fragile test would be worse than no test.
One way to reliably hit the InferenceFailure branch without depending on solver internals would be to pass conflicting explicit type arguments — e.g., a call where the same type variable must simultaneously be @Nullable (from one argument) and @NonNull (from the return-type assignment context) with warnOnGenericInferenceFailure enabled. But that's exactly the kind of scenario that requires careful setup, so deferring makes sense.
If you'd like, I can open a follow-up issue to track adding that regression test. Just let me know!
🧠 Learnings used
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: 1507
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java:66-79
Timestamp: 2026-04-01T15:40:47.197Z
Learning: In NullAway's `GenericTypePrettyPrintingVisitor` (nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java), the `appendNullableAnnotationIfPresent` helper intentionally prints ONLY `Nullable` type-use annotations and silently suppresses all other type-use annotations (including `NonNull`). This is by design: non-null is the default in NullAway's model and other type-use qualifiers are not relevant to NullAway diagnostics, so only `Nullable` is meaningful to display in error messages.
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: 1485
File: nullaway/src/main/java/com/uber/nullaway/NullAway.java:2119-2128
Timestamp: 2026-03-20T15:34:10.657Z
Learning: In this repo (uber/NullAway), don’t annotate local variables with `Nullable` or `NonNull`. NullAway infers nullability for local variables automatically, so adding these annotations to locals is incorrect style for this codebase.
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.
@officialasishkumar can you say more on why you can't write a test for this? It should be easy to reliably trigger an inference failure.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Extract the repeated isMethodUnannotated / explicit-nullness gate into a shouldSkipMismatchInUnannotatedMethod helper in CheckIdenticalNullabilityVisitor. This deduplicates the check that was inlined in both visitClassType and visitArrayType. Also fix a minor Javadoc line-wrapping issue in GenericsChecks. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for the contribution @officialasishkumar! I'll take a closer look soon, but had one question to start.
| // inference failed; restore explicit nullability annotations from the original method | ||
| // type to the call-site type, so restrictive annotations (e.g. @NonNull on type | ||
| // arguments) are still enforced | ||
| return TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations( | ||
| methodType, methodTypeAtCallSite, config, Collections.emptyMap()) | ||
| .asMethodType(); |
There was a problem hiding this comment.
@officialasishkumar can you say more on why you can't write a test for this? It should be easy to reliably trigger an inference failure.
3 participants
Fixes #1488
A description about what and why you are contributing, even if it's trivial.
The issue number(s) or PR number(s) in the description if you are contributing in response to those.
If applicable, unit tests.
Description
In JSpecify mode,
@NullUnmarkedmethods with explicit@NonNullannotations on type parameters (e.g.,Foo<@NonNull T>) were not enforcing those restrictions at call sites. This happened becausecompareGenericTypeParameterNullabilityForCallwas only invoked for annotated methods, so restrictive annotations on type variables in@NullUnmarkedcode were silently ignored.Changes
NullAway.java: Moved thecompareGenericTypeParameterNullabilityForCallinvocation outside theif (isMethodAnnotated)guard so it also runs for@NullUnmarkedmethods. ThecheckGenericMethodCallTypeArgumentscall remains gated to annotated methods only.GenericsChecks.java: Added anisMethodAnnotatedparameter tocompareGenericTypeParameterNullabilityForCalland threaded it throughsubtypeParameterNullabilityandidenticalTypeParameterNullabilityso the visitor knows when to apply lenient checking. Also improved the inference-failure path insubstituteTypeArgsInGenericMethodTypeto restore explicit nullability annotations from the original method type, so restrictive annotations survive even when the constraint solver finds unsatisfiable constraints.CheckIdenticalNullabilityVisitor.java: Added anisMethodUnannotatedflag. When true, the visitor skips nullability mismatches for type arguments that have no explicit nullness annotation (neither@Nullablenor@NonNull), since unannotated type arguments are unconstrained in@NullUnmarkedcode. Type arguments with explicit restrictive annotations (e.g.,@NonNull) are still enforced.Test cases
nullUnmarkedRestrictiveAnnotationsOnTypeVariables: Verifies that passingFoo<@Nullable String>to a@NullUnmarkedmethod expectingFoo<@NonNull String>reports an error, both for non-generic and generic method cases.nullUnmarkedUnannotatedTypeVariableNoError: Verifies that passingList<@Nullable String>to a@NullUnmarkedmethod with unannotated type parameterList<T>does NOT report an error.Summary by CodeRabbit
Bug Fixes
Tests