Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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 @@ -37,6 +37,7 @@
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.tree.TreeInfo;
import com.sun.tools.javac.util.ListBuffer;
import com.sun.tools.javac.util.Name;
import com.sun.tools.javac.util.Names;
import com.uber.nullaway.CodeAnnotationInfo;
Expand Down Expand Up @@ -1949,6 +1950,9 @@ private Type substituteTypeArgsInGenericMethodType(
Type.MethodType methodTypeAtCallSite =
castToNonNull(ASTHelpers.getType(invocationTree.getMethodSelect())).asMethodType();
if (result instanceof InferenceSuccess successResult) {
methodTypeAtCallSite =
restoreNestedNullabilityForTypeVarArguments(
invocationTree, methodType, methodTypeAtCallSite, state);
return TypeSubstitutionUtils.updateMethodTypeWithInferredNullability(
methodTypeAtCallSite, methodType, successResult.typeVarNullability, state, config);
} else {
Expand All @@ -1960,6 +1964,90 @@ private Type substituteTypeArgsInGenericMethodType(
state.getTypes(), methodType, forAllType.tvars, explicitTypeArgs, config);
}

/**
* For some calls, javac drops nested type-use nullability annotations in inferred substitutions
* for method type variables. Recover these annotations from the corresponding actual argument
* types, while preserving one consistent top-level substitution per method type variable.
*/
@SuppressWarnings("ReferenceEquality")
private Type.MethodType restoreNestedNullabilityForTypeVarArguments(
MethodInvocationTree invocationTree,
Type.MethodType origMethodType,
Type.MethodType methodTypeAtCallSite,
VisitorState state) {
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(invocationTree);
if (methodSymbol.isVarArgs()) {
// TODO handle varargs methods
return methodTypeAtCallSite;
}
com.sun.tools.javac.util.List<Type> origArgTypes = origMethodType.getParameterTypes();
com.sun.tools.javac.util.List<Type> callSiteArgTypes = methodTypeAtCallSite.getParameterTypes();
List<? extends ExpressionTree> callArgs = invocationTree.getArguments();
if (origArgTypes.size() != callSiteArgTypes.size() || callArgs.size() != origArgTypes.size()) {
return methodTypeAtCallSite;
}

// use this map to store repaired substitutions for method type variables, to ensure we use the
// same repaired
// substitution for all occurrences of the same method type variable
Map<Symbol.TypeVariableSymbol, Type> repairedTopLevelSubstitutions = new HashMap<>();
ListBuffer<Type> updatedArgTypes = new ListBuffer<>();
boolean changed = false;
for (int i = 0; i < origArgTypes.size(); i++) {
Type updatedType = callSiteArgTypes.get(i);
Type origArgType = origArgTypes.get(i);
if (origArgType instanceof Type.TypeVar typeVar
&& typeVar.tsym.owner == methodSymbol
&& !(updatedType instanceof Type.TypeVar)) {
Symbol.TypeVariableSymbol typeVarSymbol = (Symbol.TypeVariableSymbol) typeVar.tsym;
Type repairedSubstitution = repairedTopLevelSubstitutions.get(typeVarSymbol);
if (repairedSubstitution != null) {
if (!state
.getTypes()
.isSameType(
state.getTypes().erasure(repairedSubstitution),
state.getTypes().erasure(updatedType))) {
// Inconsistent substitution for the same top-level type variable; bail out.
return methodTypeAtCallSite;
}
if (repairedSubstitution != updatedType) {
changed = true;
updatedType = repairedSubstitution;
}
} else { // need to compute the substitution
Type actualArgType = getTreeType(callArgs.get(i), state);
if (actualArgType != null
&& !actualArgType.isRaw()
&& state
.getTypes()
.isSameType(
state.getTypes().erasure(actualArgType),
state.getTypes().erasure(updatedType))) {
Type restoredType =
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations(
actualArgType, updatedType, config, Collections.emptyMap());
repairedTopLevelSubstitutions.put(typeVarSymbol, restoredType);
if (restoredType != updatedType) {
changed = true;
updatedType = restoredType;
}
} else {
repairedTopLevelSubstitutions.put(typeVarSymbol, updatedType);
}
}
}
updatedArgTypes.append(updatedType);
}

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.

🧹 Nitpick | 🔵 Trivial

Optional: Replace indexed access with iterator-based traversal for com.sun.tools.javac.util.List.

origArgTypes.get(i) and callSiteArgTypes.get(i) are each O(i) on the linked-list com.sun.tools.javac.util.List, making the loop O(n²). In practice this is negligible, but the idiomatic javac pattern (used in visitTypeLists in RestoreNullnessAnnotationsVisitor) uses parallel l.tail/l.head traversal:

♻️ Proposed refactor (O(n) traversal)
-    for (int i = 0; i < origArgTypes.size(); i++) {
-      Type updatedType = callSiteArgTypes.get(i);
-      Type origArgType = origArgTypes.get(i);
+    int i = 0;
+    for (
+        com.sun.tools.javac.util.List<Type> origL = origArgTypes,
+            callSiteL = callSiteArgTypes;
+        origL.nonEmpty();
+        origL = origL.tail, callSiteL = callSiteL.tail, i++) {
+      Type updatedType = callSiteL.head;
+      Type origArgType = origL.head;
       ...
       Type actualArgType = getTreeType(callArgs.get(i), state);
🤖 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 1996 - 2040, The loop in GenericsChecks.java uses indexed get(i) on
com.sun.tools.javac.util.List (origArgTypes, callSiteArgTypes, callArgs) which
is O(n^2); replace it with parallel head/tail traversal: create three list
cursors (e.g., orig = origArgTypes, callSite = callSiteArgTypes, callArgTrees =
callArgs) and iterate while orig != null, using orig.head / orig.tail and
callSite.head / callSite.tail and callArgTrees.head / callArgTrees.tail to
obtain origArgType, updatedType, and call arg tree respectively, preserving the
existing logic that references repairedTopLevelSubstitutions,
getTreeType(callArgs.get(i), state),
TypeSubstitutionUtils.restoreExplicitNullabilityAnnotations, and returning
methodTypeAtCallSite when needed; append updatedType to updatedArgTypes as
before.

if (!changed) {
return methodTypeAtCallSite;
}
return new Type.MethodType(
updatedArgTypes.toList(),
methodTypeAtCallSite.getReturnType(),
methodTypeAtCallSite.getThrownTypes(),
methodTypeAtCallSite.tsym);
}
Comment on lines +1967 to +2061

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.

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, locate existing usage patterns of Types.subst and TypeSubstitutionUtils.subst
rg -n "Types\.subst\(|TypeSubstitutionUtils\.subst" -S --type java | head -30

Repository: uber/NullAway

Length of output: 171


🏁 Script executed:

#!/bin/bash
# Check the broader context of the GenericsChecks.java file to understand the pattern
rg -n "TypeSubstitutionUtils|Types.*subst" nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

Repository: uber/NullAway

Length of output: 1390


🏁 Script executed:

#!/bin/bash
# Look for usages of getTypeParameters and method type variable handling in the file
rg -n "getTypeParameters|TypeVariableSymbol" nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java -A 2 -B 2

Repository: uber/NullAway

Length of output: 1302


🏁 Script executed:

#!/bin/bash
# Check if there are existing tests for the restoreMissingNullabilityFromSingleTopLevelTypeVarArguments method
fd -t f "Test.*\.java" nullaway | xargs grep -l "restoreMissingNullabilityFromSingleTopLevelTypeVarArguments" 2>/dev/null

Repository: uber/NullAway

Length of output: 41


🏁 Script executed:

#!/bin/bash
# Check the context around line 1611 to see how TypeSubstitutionUtils.subst is used
sed -n '1605,1620p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

Repository: uber/NullAway

Length of output: 784


🏁 Script executed:

#!/bin/bash
# Look at the TypeSubstitutionUtils class to understand available subst methods
grep -n "\.subst\|public.*subst" nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java | head -20

Repository: uber/NullAway

Length of output: 210


🏁 Script executed:

#!/bin/bash
# Examine the entire method being reviewed to check if return types are mentioned
sed -n '1615,1697p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

Repository: uber/NullAway

Length of output: 3886


🏁 Script executed:

#!/bin/bash
# Find where restoreMissingNullabilityFromSingleTopLevelTypeVarArguments is called
rg -n "restoreMissingNullabilityFromSingleTopLevelTypeVarArguments" nullaway/

Repository: uber/NullAway

Length of output: 364


🏁 Script executed:

#!/bin/bash
# Look for tests related to this method or similar return type scenarios
fd -t f ".*Test.*\.java" nullaway | xargs grep -l "restoreMissing\|return.*type.*var" 2>/dev/null | head -5

Repository: uber/NullAway

Length of output: 39


🏁 Script executed:

#!/bin/bash
# Check if there are any TODOs or comments about return type handling near this method
sed -n '1597,1700p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | grep -i "todo\|fixme\|return\|thrown"

Repository: uber/NullAway

Length of output: 615


Consider return/thrown type repair as a follow-up enhancement.
The method currently repairs parameter substitutions but leaves return and thrown types untouched. While technically consistent within this method's narrow scope, if a method type variable appears in the return type (e.g., <T extends Supplier<?>> T acceptSup(T)), parameter repairs to Supplier<@Nullable OuterT> paired with an unrepaired return Supplier<OuterT> could create inconsistent nullability in inferred types.

The suggested approach—applying repaired substitutions to return/thrown types using TypeSubstitutionUtils.subst(state.getTypes(), type, from, to, config)—is sound. However, this extends the method's scope beyond single top-level type var arguments. Consider addressing this in a follow-up PR to keep the current change focused.

🤖 Prompt for AI Agents
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around
lines 1615 - 1697, The method
restoreMissingNullabilityFromSingleTopLevelTypeVarArguments currently only
updates parameter types but can leave return and thrown types inconsistent when
a method type variable also appears there; after computing
repairedTopLevelSubstitutions, apply those substitutions to
methodTypeAtCallSite.getReturnType() and each type in
methodTypeAtCallSite.getThrownTypes() using
TypeSubstitutionUtils.subst(state.getTypes(), type, fromMap, toMap, config) (or
equivalent API) to produce repairedReturnType and repairedThrownTypes, set
changed if any differ, and use those repaired types when constructing the new
Type.MethodType so return/thrown nullability stays consistent with the repaired
parameters.


/**
* An invocation of a generic method, and the corresponding information about its assignment
* context, for the purposes of inference.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1528,6 +1528,34 @@ public static <T> T notNull(@Nullable T object, String message) {
.doTest();
}

@Test
public void issue1455() {
makeHelperWithInferenceFailureWarning()
.addSourceLines(
"Foo.java",
"""
import org.jspecify.annotations.Nullable;
import org.jspecify.annotations.NullMarked;
@NullMarked
class Foo<OuterT> {
interface Supplier<T extends @Nullable Object> {
T get();
}
Supplier<@Nullable OuterT> sup = make();
Supplier<@Nullable OuterT> make() {
throw new RuntimeException();
}
<T extends Supplier<?>> T acceptSup(T supplier) {
return supplier;
}
void test() {
acceptSup(sup);
}
}
""")
.doTest();
}

@Test
public void issue1453() {
makeHelper()
Expand Down