Better handle calls to super constructors and superclass methods in JSpecify mode (#1248)

msridhar · web-flow · commit d7e9b24796ec · 2025-08-30T12:02:11.000-07:00
Fixes #1246 We were computing the enclosing type for such calls incorrectly before; now we use the enclosing class of the call itself. We extract the logic for computing enclosing types to its own function and use it consistently everywhere. This change exposed a subtle bug with type substitutions; relevant test case is `nullableWildcardFromCaffeine`. We fix that bug here as well, though there is still another inference bug to be fixed. Opened #1267 and will fix in a follow-up.  ## Summary by CodeRabbit - New Features - More precise determination of enclosing types for generic method and constructor calls, improving nullability analysis for unqualified calls. - Bug Fixes - Reduced false positives/negatives in generic parameter and return nullability checks at invocation sites. - Tests - Replaced an ignored superclass-constructor test with two targeted super-constructor nullability tests. - Added a new test exercising complex wildcard/substitution scenarios inspired by Caffeine.
diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
@@ -10,6 +10,7 @@
 import com.sun.source.tree.AnnotatedTypeTree;
 import com.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.ConditionalExpressionTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
@@ -366,6 +367,7 @@ private void reportInvalidOverridingMethodParamTypeError(
    * @return Type of the tree with preserved annotations.
    */
   private @Nullable Type getTreeType(Tree tree) {
+    tree = ASTHelpers.stripParentheses(tree);
     if (tree instanceof NewClassTree
         && ((NewClassTree) tree).getIdentifier() instanceof ParameterizedTypeTree) {
       ParameterizedTypeTree paramTypedTree =
@@ -842,36 +844,18 @@ public void compareGenericTypeParameterNullabilityForCall(
       return;
     }
     Type invokedMethodType = methodSymbol.type;
-    // substitute class-level type arguments for instance methods
-    if (!methodSymbol.isStatic() && tree instanceof MethodInvocationTree) {
-      ExpressionTree methodSelect = ((MethodInvocationTree) tree).getMethodSelect();
-      Type enclosingType;
-      if (methodSelect instanceof MemberSelectTree) {
-        enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression());
-      } else {
-        // implicit this parameter
-        enclosingType = methodSymbol.owner.type;
-      }
-      if (enclosingType != null) {
-        invokedMethodType =
-            TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol, config);
-      }
-    }
-
-    // substitute type arguments for constructor call
-    if (tree instanceof NewClassTree) {
-      // get the type arguments from the NewClassTree itself
-      Type enclosingType = getTreeType(tree);
-      if (enclosingType != null) {
-        invokedMethodType =
-            TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol, config);
-      }
+    Type enclosingType = getEnclosingTypeForCallExpression(methodSymbol, tree, state);
+    if (enclosingType != null) {
+      invokedMethodType =
+          TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol, config);
     }
 
     // substitute type arguments for generic methods with explicit type arguments
-    if (tree instanceof MethodInvocationTree && methodSymbol.type instanceof Type.ForAll) {
-      invokedMethodType = substituteTypeArgsInGenericMethodType(tree, methodSymbol, state);
+    if (tree instanceof MethodInvocationTree && invokedMethodType instanceof Type.ForAll) {
+      invokedMethodType =
+          substituteTypeArgsInGenericMethodType(tree, (Type.ForAll) invokedMethodType, state);
     }
+
     List<Type> formalParamTypes = invokedMethodType.getParameterTypes();
     int n = formalParamTypes.size();
     if (isVarArgs) {
@@ -1071,8 +1055,9 @@ public Nullness getGenericReturnNullnessAtInvocation(
     // If generic method invocation
     if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
       // Substitute type arguments inside the return type
+      Type.ForAll forAllType = (Type.ForAll) invokedMethodSymbol.type;
       Type substitutedReturnType =
-          substituteTypeArgsInGenericMethodType(tree, invokedMethodSymbol, state).getReturnType();
+          substituteTypeArgsInGenericMethodType(tree, forAllType, state).getReturnType();
       // If this condition evaluates to false, we fall through to the subsequent logic, to handle
       // type variables declared on the enclosing class
       if (substitutedReturnType != null
@@ -1081,15 +1066,11 @@ public Nullness getGenericReturnNullnessAtInvocation(
       }
     }
 
-    if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) {
-      return Nullness.NONNULL;
-    }
-    Type methodReceiverType =
-        getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression());
-    if (methodReceiverType == null) {
+    Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state);
+    if (enclosingType == null) {
       return Nullness.NONNULL;
     } else {
-      return getGenericMethodReturnTypeNullness(invokedMethodSymbol, methodReceiverType, state);
+      return getGenericMethodReturnTypeNullness(invokedMethodSymbol, enclosingType, state);
     }
   }
 
@@ -1108,33 +1089,31 @@ private static com.sun.tools.javac.util.List<Type> convertTreesToTypes(
   /**
    * Substitutes the type arguments from a generic method invocation into the method's type.
    *
-   * @param tree the method invocation or new class tree
-   * @param methodSymbol symbol for the invoked generic method
+   * @param tree the method invocation tree
+   * @param forAllType the generic method type
    * @param state the visitor state
    * @return the substituted method type for the generic method
    */
   private Type substituteTypeArgsInGenericMethodType(
-      Tree tree, Symbol.MethodSymbol methodSymbol, VisitorState state) {
+      Tree tree, Type.ForAll forAllType, VisitorState state) {
+    Type.MethodType methodType = forAllType.asMethodType();
 
     List<? extends Tree> typeArgumentTrees =
         (tree instanceof MethodInvocationTree)
             ? ((MethodInvocationTree) tree).getTypeArguments()
             : ((NewClassTree) tree).getTypeArguments();
     com.sun.tools.javac.util.List<Type> explicitTypeArgs = convertTreesToTypes(typeArgumentTrees);
 
-    Type.ForAll forAllType = (Type.ForAll) methodSymbol.type;
-    Type.MethodType underlyingMethodType = (Type.MethodType) forAllType.qtype;
-
     // There are no explicit type arguments, so use the inferred types
     if (explicitTypeArgs.isEmpty()) {
       if (inferredTypeVarNullabilityForGenericCalls.containsKey(tree)
           && tree instanceof MethodInvocationTree) {
         return getTypeWithInferredNullability(
-            state, underlyingMethodType, inferredTypeVarNullabilityForGenericCalls.get(tree));
+            state, methodType, inferredTypeVarNullabilityForGenericCalls.get(tree));
       }
     }
     return TypeSubstitutionUtils.subst(
-        state.getTypes(), underlyingMethodType, forAllType.tvars, explicitTypeArgs, config);
+        state.getTypes(), methodType, forAllType.tvars, explicitTypeArgs, config);
   }
 
   /**
@@ -1178,8 +1157,9 @@ public Nullness getGenericParameterNullnessAtInvocation(
     // If generic method invocation
     if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
       // Substitute the argument types within the MethodType
-      Type substituted = substituteTypeArgsInGenericMethodType(tree, invokedMethodSymbol, state);
-      List<Type> substitutedParamTypes = substituted.getParameterTypes();
+      Type.ForAll forAllType = (Type.ForAll) invokedMethodSymbol.type;
+      List<Type> substitutedParamTypes =
+          substituteTypeArgsInGenericMethodType(tree, forAllType, state).getParameterTypes();
       // If this condition evaluates to false, we fall through to the subsequent logic, to handle
       // type variables declared on the enclosing class
       if (substitutedParamTypes != null
@@ -1190,27 +1170,51 @@ public Nullness getGenericParameterNullnessAtInvocation(
       }
     }
 
-    if (tree instanceof MethodInvocationTree) {
-      if (!(((MethodInvocationTree) tree).getMethodSelect() instanceof MemberSelectTree)
-          || invokedMethodSymbol.isStatic()) {
-        return Nullness.NONNULL;
-      }
+    Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state);
+    if (enclosingType == null) {
+      return Nullness.NONNULL;
     }
 
+    return getGenericMethodParameterNullness(paramIndex, invokedMethodSymbol, enclosingType, state);
+  }
+
+  /**
+   * Gets the enclosing type for a non-static method call expression, which is either the type of
+   * the enclosing class (for implicit this calls) or the type of the receiver expression. For a
+   * constructor call, we treat the type being allocated as the enclosing type.
+   *
+   * @param invokedMethodSymbol symbol for the invoked method
+   * @param tree the tree for the invocation
+   * @param state the visitor state
+   * @return the enclosing type for the method call, or null if it cannot be determined
+   */
+  private @Nullable Type getEnclosingTypeForCallExpression(
+      Symbol.MethodSymbol invokedMethodSymbol, Tree tree, VisitorState state) {
     Type enclosingType = null;
     if (tree instanceof MethodInvocationTree) {
-      enclosingType =
-          getTreeType(
-              ((MemberSelectTree) ((MethodInvocationTree) tree).getMethodSelect()).getExpression());
-
+      if (invokedMethodSymbol.isStatic()) {
+        return null;
+      }
+      ExpressionTree methodSelect =
+          ASTHelpers.stripParentheses(((MethodInvocationTree) tree).getMethodSelect());
+      if (methodSelect instanceof IdentifierTree) {
+        // implicit this parameter, or a super call.  in either case, use the type of the enclosing
+        // class.
+        ClassTree enclosingClassTree =
+            ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class);
+        if (enclosingClassTree != null) {
+          enclosingType = castToNonNull(ASTHelpers.getType(enclosingClassTree));
+        }
+      } else if (methodSelect instanceof MemberSelectTree) {
+        enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression());
+      }
     } else {
       Verify.verify(tree instanceof NewClassTree);
       // for a constructor invocation, the type from the invocation itself is the "enclosing type"
       // for the purposes of determining type arguments
       enclosingType = getTreeType(tree);
     }
-
-    return getGenericMethodParameterNullness(paramIndex, invokedMethodSymbol, enclosingType, state);
+    return enclosingType;
   }
 
   /**
diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
@@ -1083,6 +1083,43 @@ public void issue1238() {
         .doTest();
   }
 
+  /**
+   * Extracted from Caffeine; exposed some subtle bugs in substitutions involving identity of {@code
+   * Type} objects
+   */
+  @Test
+  public void nullableWildcardFromCaffeine() {
+    makeHelperWithInferenceFailureWarning()
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.NullMarked;",
+            "import org.jspecify.annotations.Nullable;",
+            "@NullMarked",
+            "public class Test {",
+            "    public interface CacheLoader<K, V extends @Nullable Object> {}",
+            "    static class JCacheLoaderAdapter<K, V> implements CacheLoader<K, @Nullable Expirable<V>> {}",
+            "    static class Expirable<V> {}",
+            "    static class Caffeine<K, V> {",
+            "        public <K1 extends K, V1 extends @Nullable V> Object build(",
+            "                CacheLoader<? super K1, V1> loader) {",
+            "            throw new RuntimeException();",
+            "        }",
+            "    }",
+            "    class Builder<K, V> {",
+            "        Caffeine<Object, Object> caffeine = new Caffeine<>();",
+            "        void test() {",
+            "            JCacheLoaderAdapter<K, V> adapter = new JCacheLoaderAdapter<>();",
+            "            caffeine.<K, @Nullable Expirable<V>>build(adapter);",
+            "            // TODO inference is buggy for this case; doesn't substitute inferred types properly",
+            "            // See https://github.com/uber/NullAway/issues/1267",
+            "            @SuppressWarnings(\"NullAway\")",
+            "            Object o = caffeine.build(adapter);",
+            "        }",
+            "    }",
+            "}")
+        .doTest();
+  }
+
   private CompilationTestHelper makeHelper() {
     return makeTestHelperWithArgs(
         Arrays.asList(
diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java
@@ -2342,7 +2342,6 @@ public void callWithConstructorReceiver() {
         .doTest();
   }
 
-  @Ignore("https://github.com/uber/NullAway/issues/1246")
   @Test
   public void nullableSuperConstructorArg() {
     makeHelper()
@@ -2364,7 +2363,50 @@ public void nullableSuperConstructorArg() {
         .doTest();
   }
 
-  @Ignore("https://github.com/uber/NullAway/issues/1246")
+  @Test
+  public void passNonNullToNullableSuperConstructorArg() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.NullMarked;",
+            "import org.jspecify.annotations.Nullable;",
+            "@NullMarked",
+            "public class Test {",
+            "  private static class A<T extends @Nullable Object> {",
+            "    A(T t) {}",
+            "  }",
+            "  private static class B extends A<@Nullable Object> {",
+            "    B(Object o) {",
+            "      super(o);",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void mismatchedTypeArgNullabilityForSuperConstructor() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.NullMarked;",
+            "import org.jspecify.annotations.Nullable;",
+            "import java.util.List;",
+            "@NullMarked",
+            "public class Test {",
+            "  private static class A<T extends @Nullable Object> {",
+            "    A(List<T> l) {}",
+            "  }",
+            "  private static class B extends A<@Nullable Object> {",
+            "    B(List<Object> l) {",
+            "      // BUG: Diagnostic contains: Cannot pass parameter",
+            "      super(l);",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void nullableSuperMethodArg() {
     makeHelper()
