Skip to content

Handle nested restrictive annotations on type variables in @NullUnmarked code #1510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
officialasishkumar wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Handle nested restrictive annotations on type variables in @NullUnmarked code #1510

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6acc8f
fix: handle nested restrictive annotations on type variables in @Null…
officialasishkumar Apr 4, 2026
603babb
refactor: extract unannotated-mismatch check into helper method
officialasishkumar Apr 5, 2026
4d35c90
Merge branch 'master' into issue-1488
msridhar Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2083,15 +2083,20 @@ private Description handleInvocation(
}
}

// perform generics checks for calls to annotated methods in JSpecify mode
if (config.isJSpecifyMode()) {
genericsChecks.compareGenericTypeParameterNullabilityForCall(methodSymbol, tree, state);
if (!methodSymbol.getTypeParameters().isEmpty()) {
genericsChecks.checkGenericMethodCallTypeArguments(tree, state);
}
// perform generics checks for type arguments on calls to annotated methods in JSpecify mode
if (config.isJSpecifyMode() && !methodSymbol.getTypeParameters().isEmpty()) {
genericsChecks.checkGenericMethodCallTypeArguments(tree, state);
}
}

// Perform generics checks for calls in JSpecify mode, including calls to @NullUnmarked
// methods. For @NullUnmarked methods, only restrictive (explicit) nullness annotations
// on type parameters are enforced (see #1488).
if (config.isJSpecifyMode()) {
genericsChecks.compareGenericTypeParameterNullabilityForCall(
methodSymbol, tree, state, isMethodAnnotated);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// Allow handlers to override the list of non-null argument positions. For a varargs parameter,
// this array holds the nullness of individual varargs arguments; the case of a varargs array is
// handled separately
Expand Down
38 changes: 36 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.uber.nullaway.Config;
import com.uber.nullaway.Nullness;
import java.util.List;
import javax.lang.model.type.NullType;
import javax.lang.model.type.TypeKind;
Expand All @@ -18,12 +19,22 @@ public class CheckIdenticalNullabilityVisitor extends Types.DefaultTypeVisitor<B
private final VisitorState state;
private final GenericsChecks genericsChecks;
private final Config config;
private final boolean isMethodUnannotated;

CheckIdenticalNullabilityVisitor(
VisitorState state, GenericsChecks genericsChecks, Config config) {
this(state, genericsChecks, config, false);
}

CheckIdenticalNullabilityVisitor(
VisitorState state,
GenericsChecks genericsChecks,
Config config,
boolean isMethodUnannotated) {
this.state = state;
this.genericsChecks = genericsChecks;
this.config = config;
this.isMethodUnannotated = isMethodUnannotated;
}

@Override
Expand Down Expand Up @@ -71,7 +82,12 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsTypeArgument);
boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsTypeArgument);
if (isLHSNullableAnnotated != isRHSNullableAnnotated) {
return false;
if (shouldSkipMismatchInUnannotatedMethod(lhsTypeArgument, isLHSNullableAnnotated)) {
// LHS is unannotated in @NullUnmarked context; skip this level but still check
// nested types below (which may have restrictive annotations)
} else {
return false;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
// nested generics
if (!lhsTypeArgument.accept(this, rhsTypeArgument)) {
Expand Down Expand Up @@ -104,11 +120,29 @@ public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) {
boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsComponentType);
boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsComponentType);
if (isRHSNullableAnnotated != isLHSNullableAnnotated) {
return false;
if (shouldSkipMismatchInUnannotatedMethod(lhsComponentType, isLHSNullableAnnotated)) {
// LHS component is unannotated in @NullUnmarked context; skip this level
} else {
return false;
}
}
return lhsComponentType.accept(this, rhsComponentType);
}

/**
* Returns {@code true} when a nullability mismatch should be skipped because the LHS type has no
* explicit nullness annotation and we are inside {@code @NullUnmarked} code.
*
* @param lhsType the formal (LHS) type whose annotations are inspected
* @param isLhsNullableAnnotated whether {@code lhsType} carries a {@code @Nullable} annotation
*/
private boolean shouldSkipMismatchInUnannotatedMethod(
Type lhsType, boolean isLhsNullableAnnotated) {
return isMethodUnannotated
&& !isLhsNullableAnnotated
&& !Nullness.hasNonNullAnnotation(lhsType.getAnnotationMirrors().stream(), config);
}

@Override
public Boolean visitType(Type t, Type type) {
return true;
Expand Down
48 changes: 36 additions & 12 deletions nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,19 +1438,25 @@ public void checkTypeParameterNullnessForFunctionReturnType(
* @param state the visitor state
*/
private boolean identicalTypeParameterNullability(
Type lhsType, Type rhsType, VisitorState state) {
return lhsType.accept(new CheckIdenticalNullabilityVisitor(state, this, config), rhsType);
Type lhsType, Type rhsType, VisitorState state, boolean isMethodUnannotated) {
return lhsType.accept(
new CheckIdenticalNullabilityVisitor(state, this, config, isMethodUnannotated), rhsType);
}

/**
* Like {@link #identicalTypeParameterNullability(Type, Type, VisitorState)}, but allows for
* covariant array subtyping at the top level.
* Like {@link #identicalTypeParameterNullability(Type, Type, VisitorState, boolean)}, but allows
* for covariant array subtyping at the top level.
*
* @param lhsType type for the lhs of the assignment
* @param rhsType type for the rhs of the assignment
* @param state the visitor state
*/
private boolean subtypeParameterNullability(Type lhsType, Type rhsType, VisitorState state) {
return subtypeParameterNullability(lhsType, rhsType, state, false);
}

private boolean subtypeParameterNullability(
Type lhsType, Type rhsType, VisitorState state, boolean isMethodUnannotated) {
if (lhsType.isRaw()) {
return true;
}
Expand All @@ -1466,11 +1472,19 @@ private boolean subtypeParameterNullability(Type lhsType, Type rhsType, VisitorS
boolean isRHSNullableAnnotated = isNullableAnnotated(rhsComponentType);
// an array of @Nullable references is _not_ a subtype of an array of @NonNull references
if (isRHSNullableAnnotated && !isLHSNullableAnnotated) {
return false;
// In @NullUnmarked code, skip if the LHS component has no explicit nullness annotation
if (isMethodUnannotated
&& !Nullness.hasNonNullAnnotation(
lhsComponentType.getAnnotationMirrors().stream(), config)) {
// unannotated component in @NullUnmarked context; fall through to nested check
} else {
return false;
}
}
return identicalTypeParameterNullability(lhsComponentType, rhsComponentType, state);
return identicalTypeParameterNullability(
lhsComponentType, rhsComponentType, state, isMethodUnannotated);
} else {
return identicalTypeParameterNullability(lhsType, rhsType, state);
return identicalTypeParameterNullability(lhsType, rhsType, state, isMethodUnannotated);
}
}

Expand Down Expand Up @@ -1553,9 +1567,12 @@ public void checkTypeParameterNullnessForConditionalExpression(
* @param methodSymbol the symbol for the method being called
* @param tree the tree representing the method call
* @param state the visitor state
* @param isMethodAnnotated whether the called method is in annotated (non-{@code @NullUnmarked})
* code. When {@code false}, only restrictive (explicit) nullness annotations on type
* parameters are enforced.
*/
public void compareGenericTypeParameterNullabilityForCall(
Symbol.MethodSymbol methodSymbol, Tree tree, VisitorState state) {
Symbol.MethodSymbol methodSymbol, Tree tree, VisitorState state, boolean isMethodAnnotated) {
Config config = analysis.getConfig();
if (!config.isJSpecifyMode()) {
return;
Expand Down Expand Up @@ -1602,13 +1619,15 @@ public void compareGenericTypeParameterNullabilityForCall(
// the type of the method reference tree provided by javac may not capture
// nullability of nested types. So, do explicit type checks based on the return and
// parameter types of the referenced method
boolean isUnannotated = !isMethodAnnotated;
GenericsUtils.processMethodRefTypeRelations(
this,
formalParameter,
memberReferenceTree,
state,
(subtype, supertype, relationKind) -> {
if (!subtypeParameterNullability(supertype, subtype, state)) {
if (!subtypeParameterNullability(
supertype, subtype, state, isUnannotated)) {
if (relationKind == MethodRefTypeRelationKind.RETURN) {
reportInvalidMethodReferenceReturnTypeError(
memberReferenceTree, supertype, subtype, state);
Expand Down Expand Up @@ -1646,7 +1665,8 @@ public void compareGenericTypeParameterNullabilityForCall(
false,
false);
}
if (!subtypeParameterNullability(formalParameter, actualParameterType, state)) {
if (!subtypeParameterNullability(
formalParameter, actualParameterType, state, !isMethodAnnotated)) {
reportInvalidParametersNullabilityError(
formalParameter, actualParameterType, currentActualParam, state);
}
Expand Down Expand Up @@ -1929,8 +1949,12 @@ private Type substituteTypeArgsInGenericMethodType(
return TypeSubstitutionUtils.updateMethodTypeWithInferredNullability(
methodTypeAtCallSite, methodType, successResult.typeVarNullability, state, config);
} else {
// inference failed; just return the method type at the call site with no substitutions
return methodTypeAtCallSite;
// 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();
Comment on lines +1952 to +1957
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Copy link
Copy Markdown
Author

@officialasishkumar officialasishkumar Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Collaborator

@msridhar msridhar Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
}
return TypeSubstitutionUtils.subst(
Expand Down
72 changes: 72 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,78 @@ public static void caller() {
.doTest();
}

@Test
public void nullUnmarkedRestrictiveAnnotationsOnTypeVariables() {
makeTestHelperWithArgs(
JSpecifyJavacConfig.withJSpecifyModeArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:OnlyNullMarked=true")))
.addSourceLines(
"Test.java",
"""
package com.uber;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;
@NullMarked
class Test {
interface Foo<T extends @Nullable Object> {}
@NullUnmarked
static void takeFoo(Foo<@NonNull String> f) {}
@NullUnmarked
static <T> T identity(Foo<@NonNull T> t) {
throw new RuntimeException("not implemented");
}
void test(Foo<@Nullable String> s, Foo<String> nonNullFoo) {
// Error: passing Foo<@Nullable String> where Foo<@NonNull String> is required
// BUG: Diagnostic contains: incompatible types
takeFoo(s);
// OK: passing Foo<String> where Foo<@NonNull String> is required
takeFoo(nonNullFoo);
// Error: passing Foo<@Nullable String> to generic identity method
// with restrictive @NonNull on type parameter
// BUG: Diagnostic contains: incompatible types
String x = identity(s);
// OK: passing Foo<String> where Foo<@NonNull String> is expected
String y = identity(nonNullFoo);
}
}
""")
.doTest();
}

@Test
public void nullUnmarkedUnannotatedTypeVariableNoError() {
makeTestHelperWithArgs(
JSpecifyJavacConfig.withJSpecifyModeArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:OnlyNullMarked=true")))
.addSourceLines(
"Test.java",
"""
package com.uber;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;
import java.util.List;
@NullMarked
class Test {
@NullUnmarked
static <T> void bar(List<T> list) {}
void test(List<@Nullable String> s) {
// No error: T is unannotated in @NullUnmarked code, so unconstrained
bar(s);
}
}
""")
.doTest();
}

@Test
public void nullUnmarkedOuterMethodLevelWithLocalClass() {
defaultCompilationHelper
Expand Down
Loading