Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.uber.nullaway.Config;
import com.uber.nullaway.handlers.Handler;
import java.util.List;
import javax.lang.model.type.NullType;
import javax.lang.model.type.TypeKind;
Expand All @@ -21,12 +22,14 @@ public class CheckIdenticalNullabilityVisitor extends Types.DefaultTypeVisitor<B
private final VisitorState state;
private final GenericsChecks genericsChecks;
private final Config config;
private final Handler handler;

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

@Override
Expand Down Expand Up @@ -155,7 +158,8 @@ private boolean wildcardContains(Type.WildcardType lhsWildcard, Type rhsTypeArgu
return switch (lhsWildcard.kind) {
case UNBOUND, EXTENDS ->
extendsBoundContains(
GenericsUtils.wildcardUpperBound(lhsWildcard, state), rhsTypeArgument);
GenericsUtils.wildcardUpperBound(lhsWildcard, state, config, handler),
rhsTypeArgument);
case SUPER -> superWildcardContains(lhsWildcard, rhsTypeArgument);
};
}
Expand All @@ -168,7 +172,7 @@ private boolean wildcardContains(Type.WildcardType lhsWildcard, Type rhsTypeArgu
private boolean extendsBoundContains(Type lhsBound, Type rhsTypeArgument) {
Type.WildcardType rhsWildcard = GenericsUtils.asWildcard(rhsTypeArgument);
if (rhsWildcard != null) {
Type rhsUpperBound = GenericsUtils.wildcardUpperBound(rhsWildcard, state);
Type rhsUpperBound = GenericsUtils.wildcardUpperBound(rhsWildcard, state, config, handler);
return typeArgumentSubtype(lhsBound, rhsUpperBound);
}
return typeArgumentSubtype(lhsBound, rhsTypeArgument);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@

import com.google.common.base.Verify;
import com.google.errorprone.VisitorState;
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.BoundKind;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Type.ClassType;
import com.sun.tools.javac.code.Type.TypeVar;
import com.sun.tools.javac.code.Type.WildcardType;
import com.sun.tools.javac.code.Types;
import com.uber.nullaway.CodeAnnotationInfo;
import com.uber.nullaway.Config;
import com.uber.nullaway.NullAway;
import com.uber.nullaway.Nullness;
Expand All @@ -34,13 +32,11 @@
*/
public final class ConstraintSolverImpl implements ConstraintSolver {
private final Config config;
private final CodeAnnotationInfo codeAnnotationInfo;
private final Handler handler;
private final VisitorState state;

public ConstraintSolverImpl(Config config, VisitorState state, NullAway analysis) {
this.config = config;
this.codeAnnotationInfo = CodeAnnotationInfo.instance(state.context);
this.handler = analysis.getHandler();
this.state = state;
}
Expand Down Expand Up @@ -210,9 +206,10 @@ private void equateTypeArguments(Type subtypeTypeArg, Type supertypeTypeArg) {
private void constrainContainedByWildcard(Type subtypeTypeArg, WildcardType supertypeWildcard) {
switch (supertypeWildcard.kind) {
case UNBOUND, EXTENDS -> {
Type subtypeUpperBound = GenericsUtils.effectiveWildcardUpperBound(subtypeTypeArg, state);
Type subtypeUpperBound =
GenericsUtils.effectiveWildcardUpperBound(subtypeTypeArg, state, config, handler);
subtypeUpperBound.accept(
this, GenericsUtils.wildcardUpperBound(supertypeWildcard, state));
this, GenericsUtils.wildcardUpperBound(supertypeWildcard, state, config, handler));
}
case SUPER -> {
Type supertypeLowerBound = castToNonNull(supertypeWildcard.getSuperBound());
Expand All @@ -238,7 +235,8 @@ private void constrainContainedByWildcard(Type subtypeTypeArg, WildcardType supe
*/
private void constrainSubtypeToWildcard(Type subtype, WildcardType supertypeWildcard) {
if (supertypeWildcard.kind != BoundKind.SUPER) {
subtype.accept(this, GenericsUtils.wildcardUpperBound(supertypeWildcard, state));
subtype.accept(
this, GenericsUtils.wildcardUpperBound(supertypeWildcard, state, config, handler));
}
}

Expand All @@ -251,7 +249,8 @@ private void constrainWildcardToSupertype(WildcardType subtypeWildcard, Type sup
if (subtypeWildcard.kind == BoundKind.SUPER) {
castToNonNull(subtypeWildcard.getSuperBound()).accept(this, supertype);
} else {
GenericsUtils.wildcardUpperBound(subtypeWildcard, state).accept(this, supertype);
GenericsUtils.wildcardUpperBound(subtypeWildcard, state, config, handler)
.accept(this, supertype);
}
}
}
Expand Down Expand Up @@ -391,7 +390,9 @@ private void constrainAsNonNull(Type t, Type knownNonNullType)
/* ───────────────────── helpers & stubs ───────────────────── */

private VarState getState(Element typeVarElement) {
return vars.computeIfAbsent(typeVarElement, v -> new VarState(upperBoundIsNullable(v)));
return vars.computeIfAbsent(
typeVarElement,
v -> new VarState(GenericsUtils.upperBoundIsNullable(v, config, handler, state)));
}

private boolean treatAsTypeVariableForInference(Type t) {
Expand All @@ -414,44 +415,4 @@ private boolean isKnownNullable(Type t) {
private boolean isKnownNonNull(Type t) {
return !isKnownNullable(t) && !treatAsTypeVariableForInference(t);
}

private boolean upperBoundIsNullable(Element typeVarElement) {
if (fromUnannotatedMethod(typeVarElement)) {
return true;
}
// first, check if library model overrides the upper bound nullability
Element enclosingElement = typeVarElement.getEnclosingElement();
if (enclosingElement instanceof Symbol.MethodSymbol methodSymbol
&& typeVarElement instanceof Symbol.TypeVariableSymbol typeVariableSymbol) {
int typeVarIndex = methodSymbol.getTypeParameters().indexOf(typeVariableSymbol);
// TODO typeVarIndex is -1 in some cases; see test
// com.uber.nullaway.jspecify.GenericMethodTests.instanceGenericMethodWithMethodRefArgument.
// Investigate further.
if (typeVarIndex >= 0
&& handler.onOverrideMethodTypeVariableUpperBound(methodSymbol, typeVarIndex, state)) {
return true;
}
} else if (enclosingElement instanceof Symbol.ClassSymbol classSymbol
&& typeVarElement instanceof Symbol.TypeVariableSymbol typeVariableSymbol) {
int typeVarIndex = classSymbol.getTypeParameters().indexOf(typeVariableSymbol);
if (typeVarIndex >= 0
&& handler.onOverrideClassTypeVariableUpperBound(classSymbol.toString(), typeVarIndex)) {
return true;
}
}
// otherwise, check the actual upper bound annotations
Type upperBound = (Type) ((TypeVariable) typeVarElement.asType()).getUpperBound();
com.sun.tools.javac.util.List<Attribute.TypeCompound> annotationMirrors =
upperBound.getAnnotationMirrors();
return com.uber.nullaway.Nullness.hasNullableAnnotation(annotationMirrors.stream(), config);
}

private boolean fromUnannotatedMethod(Element typeVarElement) {
Element enclosingElement = typeVarElement.getEnclosingElement();
if (!(enclosingElement instanceof Symbol.MethodSymbol)
&& !(enclosingElement instanceof Symbol.ClassSymbol)) {
return false;
}
return codeAnnotationInfo.isSymbolUnannotated((Symbol) enclosingElement, config, handler);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ private MethodInferenceResult runInferenceForCall(
Type polyExprTreeType = ASTHelpers.getType(argument);
if (polyExprTreeType != null) {
Type formalParamGroundTargetType =
GenericsUtils.groundTargetType(formalParamType, state, config);
GenericsUtils.groundTargetType(formalParamType, state, config, handler);
Type typeWithInferredNullability =
TypeSubstitutionUtils.updateTypeWithInferredNullability(
polyExprTreeType,
Expand Down Expand Up @@ -1261,7 +1261,7 @@ private void handleLambdaInGenericMethodInference(
Symbol.MethodSymbol fiMethod =
NullabilityUtil.getFunctionalInterfaceMethod(lambda, state.getTypes());

Type groundTargetType = GenericsUtils.groundTargetType(lhsType, state, config);
Type groundTargetType = GenericsUtils.groundTargetType(lhsType, state, config, handler);
// get the return type of the functional interface method, viewed as a member of the lhs
// type, so the generic method's type variables are substituted in
Type.MethodType fiMethodTypeAsMember =
Expand Down Expand Up @@ -1315,7 +1315,7 @@ private void handleMethodRefInGenericMethodInference(
ConstraintSolver solver,
Type lhsType,
MemberReferenceTree memberReferenceTree) {
Type groundTargetType = GenericsUtils.groundTargetType(lhsType, state, config);
Type groundTargetType = GenericsUtils.groundTargetType(lhsType, state, config, handler);
GenericsUtils.processMethodRefTypeRelations(
this,
groundTargetType,
Expand Down Expand Up @@ -1645,7 +1645,8 @@ public void checkTypeParameterNullnessForFunctionReturnType(
*/
private boolean identicalTypeParameterNullability(
Type lhsType, Type rhsType, VisitorState state) {
return lhsType.accept(new CheckIdenticalNullabilityVisitor(state, this, config), rhsType);
return lhsType.accept(
new CheckIdenticalNullabilityVisitor(state, this, config, handler), rhsType);
}

/**
Expand Down Expand Up @@ -1808,7 +1809,7 @@ public void compareGenericTypeParameterNullabilityForCall(

if (currentActualParam instanceof MemberReferenceTree memberReferenceTree) {
Type groundFormalParameter =
GenericsUtils.groundTargetType(formalParameter, state, config);
GenericsUtils.groundTargetType(formalParameter, state, config, handler);
// 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
Expand Down Expand Up @@ -2613,7 +2614,8 @@ private Nullness getReturnTypeNullness(
return Nullness.NULLABLE;
}
if (config.handleWildcardGenerics() && GenericsUtils.asWildcard(type) != null) {
Type effectiveUpperBound = GenericsUtils.effectiveWildcardUpperBound(type, state);
Type effectiveUpperBound =
GenericsUtils.effectiveWildcardUpperBound(type, state, config, handler);
return getReturnTypeNullness(effectiveUpperBound, state, true);
}
if (followTypeVarUpperBound && type instanceof Type.TypeVar typeVar) {
Expand Down Expand Up @@ -2702,7 +2704,7 @@ private void maybeStorePolyExpressionTypeFromTarget(
if (polyExpressionType == null) {
return;
}
Type groundTargetType = GenericsUtils.groundTargetType(targetType, state, config);
Type groundTargetType = GenericsUtils.groundTargetType(targetType, state, config, handler);
Type polyExpressionTypeWithTargetAnnotations =
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations(
groundTargetType, polyExpressionType, config, Collections.emptyMap());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.util.List;
import com.sun.tools.javac.util.ListBuffer;
import com.uber.nullaway.CodeAnnotationInfo;
import com.uber.nullaway.Config;
import com.uber.nullaway.NullabilityUtil;
import com.uber.nullaway.Nullness;
import com.uber.nullaway.handlers.Handler;
import javax.lang.model.element.Element;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeVariable;
import org.jspecify.annotations.Nullable;

/** Utility methods for doing generics-related checking */
Expand All @@ -40,16 +45,20 @@ enum MethodRefTypeRelationKind {
* type itself. For wildcards and captured wildcards, returns the wildcard's upper bound,
* recursing through nested wildcards and captures produced by javac.
*/
static Type effectiveWildcardUpperBound(Type typeArg, VisitorState state) {
static Type effectiveWildcardUpperBound(
Type typeArg, VisitorState state, Config config, Handler handler) {
WildcardType wildcardType = asWildcard(typeArg);
return wildcardType == null ? typeArg : wildcardUpperBound(wildcardType, state);
return wildcardType == null
? typeArg
: wildcardUpperBound(wildcardType, state, config, handler);
}

/**
* Returns the effective upper bound of a wildcard, using the corresponding type variable's upper
* bound for unbounded wildcards and {@code super} wildcards.
*/
static Type wildcardUpperBound(WildcardType wildcardType, VisitorState state) {
static Type wildcardUpperBound(
WildcardType wildcardType, VisitorState state, Config config, Handler handler) {
Type upperBound;
if (wildcardType.kind == BoundKind.EXTENDS) {
upperBound = wildcardType.getExtendsBound();
Expand All @@ -64,16 +73,72 @@ static Type wildcardUpperBound(WildcardType wildcardType, VisitorState state) {
formalTypeVar == null
? Symtab.instance(state.context).objectType
: formalTypeVar.getUpperBound();
// check if the upper bound should be treated as @Nullable, e.g., due to a library model or a
// type variable in @NullUnmarked code
if (formalTypeVar != null
&& upperBoundIsNullable(formalTypeVar.asElement(), config, handler, state)
&& !Nullness.hasNullableAnnotation(upperBound.getAnnotationMirrors().stream(), config)) {
upperBound =
TypeSubstitutionUtils.typeWithAnnot(
upperBound, GenericsChecks.getSyntheticNullableAnnotType(state));
}
}
if (upperBound instanceof WildcardType nestedWildcard) {
return wildcardUpperBound(nestedWildcard, state);
return wildcardUpperBound(nestedWildcard, state, config, handler);
}
if (upperBound instanceof CapturedType capturedType && capturedType.wildcard != null) {
return wildcardUpperBound(capturedType.wildcard, state);
return wildcardUpperBound(capturedType.wildcard, state, config, handler);
}
return upperBound;
}

/**
* Returns true if the upper bound of the given type variable should be treated as nullable.
*
* <p>A bound is nullable when the enclosing method or class comes from unannotated code, when a
* library model overrides the bound nullability for the type variable, or when the declared upper
* bound has an explicit {@code @Nullable} annotation.
*/
static boolean upperBoundIsNullable(
Element typeVarElement, Config config, Handler handler, VisitorState state) {
if (fromUnannotatedMethodOrClass(typeVarElement, config, handler, state)) {
return true;
}
// First, check if library model overrides the upper bound nullability.
Element enclosingElement = typeVarElement.getEnclosingElement();
if (enclosingElement instanceof Symbol.MethodSymbol methodSymbol
&& typeVarElement instanceof Symbol.TypeVariableSymbol typeVariableSymbol) {
int typeVarIndex = methodSymbol.getTypeParameters().indexOf(typeVariableSymbol);
// TODO typeVarIndex is -1 in some cases; see test
// com.uber.nullaway.jspecify.GenericMethodTests.instanceGenericMethodWithMethodRefArgument.
// Investigate further.
if (typeVarIndex >= 0
&& handler.onOverrideMethodTypeVariableUpperBound(methodSymbol, typeVarIndex, state)) {
return true;
Comment on lines +109 to +117
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't silently drop method-type-variable handler overrides when the index lookup fails.

If getTypeParameters().indexOf(typeVariableSymbol) returns -1, this helper skips onOverrideMethodTypeVariableUpperBound(...) and falls back to annotation-only logic. Since upperBoundIsNullable(...) now feeds both wildcard upper-bound computation here and ConstraintSolverImpl.getState(...), the modeled nullable upper bound is still lost for the method-type-variable cases already called out by the TODO. Please add a fallback lookup instead of treating -1 as "no override".

🤖 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/main/java/com/uber/nullaway/generics/GenericsUtils.java` around
lines 109 - 117, The code silently treats
methodSymbol.getTypeParameters().indexOf(typeVariableSymbol) == -1 as "no
override" and skips calling handler.onOverrideMethodTypeVariableUpperBound;
change this to perform a fallback lookup when indexOf returns -1: in
GenericsUtils (the branch where enclosingElement instanceof Symbol.MethodSymbol
methodSymbol and typeVarElement instanceof Symbol.TypeVariableSymbol
typeVariableSymbol), if indexOf yields -1, scan methodSymbol.getTypeParameters()
manually to find a matching TypeVariableSymbol (compare by symbol
identity/equality and, if needed, by simple name via
typeVariableSymbol.getSimpleName()) to derive a valid typeVarIndex, then use
that index to call handler.onOverrideMethodTypeVariableUpperBound(methodSymbol,
typeVarIndex, state) as before so method-type-variable overrides are not
dropped.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What kind of fallback loop?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think this is a weird corner case that doesn't need to be handled now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@msridhar, understood — the existing TODO already documents the known -1 case, so this can be deferred. Would you like me to open a follow-up GitHub issue to track it, or shall I just resolve this review comment?


🧠 Learnings used
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@msridhar — the loop I had in mind was a linear name-based scan as a fallback:

if (typeVarIndex < 0) {
  List<Symbol.TypeVariableSymbol> typeParams = methodSymbol.getTypeParameters();
  for (int i = 0; i < typeParams.size(); i++) {
    if (typeParams.get(i).getSimpleName()
        .equals(typeVariableSymbol.getSimpleName())) {
      typeVarIndex = i;
      break;
    }
  }
}

That said, I realise the root cause of why indexOf returns -1 in the first place hasn't been investigated yet (per the existing TODO pointing at GenericMethodTests.instanceGenericMethodWithMethodRefArgument). If the typeVariableSymbol reaching this point is a specialised/substituted copy of the original method's type variable (which javac sometimes produces during inference), then a name-based fallback could accidentally map to the wrong index or mask a deeper problem.

Given that uncertainty, it's probably more correct to first understand why indexOf fails — whether it's an identity vs. equality issue with javac's Symbol, a different MethodSymbol being used as enclosingElement, or something else — before deciding what the right fallback is.

Would you like to open a follow-up issue to track that investigation?


🧠 Learnings used
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.

}
} else if (enclosingElement instanceof Symbol.ClassSymbol classSymbol
&& typeVarElement instanceof Symbol.TypeVariableSymbol typeVariableSymbol) {
int typeVarIndex = classSymbol.getTypeParameters().indexOf(typeVariableSymbol);
if (typeVarIndex >= 0
&& handler.onOverrideClassTypeVariableUpperBound(classSymbol.toString(), typeVarIndex)) {
return true;
}
}
Type upperBound = (Type) ((TypeVariable) typeVarElement.asType()).getUpperBound();
return Nullness.hasNullableAnnotation(upperBound.getAnnotationMirrors().stream(), config);
}

private static boolean fromUnannotatedMethodOrClass(
Element typeVarElement, Config config, Handler handler, VisitorState state) {
Element enclosingElement = typeVarElement.getEnclosingElement();
if (!(enclosingElement instanceof Symbol.MethodSymbol)
&& !(enclosingElement instanceof Symbol.ClassSymbol)) {
return false;
}
return CodeAnnotationInfo.instance(state.context)
.isSymbolUnannotated((Symbol) enclosingElement, config, handler);
}

static @Nullable WildcardType asWildcard(Type typeArg) {
if (typeArg instanceof WildcardType wildcardType) {
return wildcardType;
Expand All @@ -98,7 +163,8 @@ static Type wildcardUpperBound(WildcardType wildcardType, VisitorState state) {
* href="https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.9">9.9</a>.
*/
@SuppressWarnings("ReferenceEquality")
static Type groundTargetType(Type targetType, VisitorState state, Config config) {
static Type groundTargetType(
Type targetType, VisitorState state, Config config, Handler handler) {
if (!config.handleWildcardGenerics()) {
return targetType;
}
Expand All @@ -112,7 +178,7 @@ static Type groundTargetType(Type targetType, VisitorState state, Config config)
ListBuffer<Type> groundedTypeArguments = new ListBuffer<>();
boolean changed = false;
for (Type typeArgument : typeArguments) {
Type groundedTypeArgument = groundTypeArgument(typeArgument, state);
Type groundedTypeArgument = groundTypeArgument(typeArgument, state, config, handler);
groundedTypeArguments.append(groundedTypeArgument);
changed |= groundedTypeArgument != typeArgument;
}
Expand All @@ -127,15 +193,16 @@ static Type groundTargetType(Type targetType, VisitorState state, Config config)
* rules for functional interface target types in JLS <a
* href="https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.9">9.9</a>.
*/
private static Type groundTypeArgument(Type typeArgument, VisitorState state) {
private static Type groundTypeArgument(
Type typeArgument, VisitorState state, Config config, Handler handler) {
WildcardType wildcardType = asWildcard(typeArgument);
if (wildcardType == null) {
return typeArgument;
}
if (wildcardType.kind == BoundKind.SUPER) {
return castToNonNull(wildcardType.getSuperBound());
}
return wildcardUpperBound(wildcardType, state);
return wildcardUpperBound(wildcardType, state, config, handler);
}

/**
Expand Down
Loading
Loading