Bug fixes for array subtyping at returns / parameter passing (#980)

In certain places we were bailing out for types without type arguments, but those bailouts are wrong now that we check array subtyping. Also, for identifiers, we now get the type from the `Symbol` which is more reliable.
uber · Jun 19, 2024 · 8593fe2 · 8593fe2
1 parent 76115d4
commit 8593fe2
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 23 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
@@ -1,6 +1,7 @@
 package com.uber.nullaway.generics;
 
 import static com.google.common.base.Verify.verify;
+import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.suppliers.Supplier;
@@ -11,6 +12,7 @@
 import com.sun.source.tree.AssignmentTree;
 import com.sun.source.tree.ConditionalExpressionTree;
 import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.MethodTree;
@@ -270,11 +272,10 @@ private static Type getTreeType(Tree tree, VisitorState state) {
       return typeWithPreservedAnnotations(tree, state);
     } else {
       Type result;
-      if (tree instanceof VariableTree) {
+      if (tree instanceof VariableTree || tree instanceof IdentifierTree) {
         // type on the tree itself can be missing nested annotations for arrays; get the type from
-        // the symbol for the declared variable instead
-        VariableTree varTree = (VariableTree) tree;
-        result = ASTHelpers.getSymbol(varTree).type;
+        // the symbol for the variable instead
+        result = castToNonNull(ASTHelpers.getSymbol(tree)).type;
       } else if (tree instanceof AssignmentTree) {
         // type on the tree itself can be missing nested annotations for arrays; get the type from
         // the symbol for the assigned location instead, if available
@@ -355,10 +356,6 @@ public static void checkTypeParameterNullnessForFunctionReturnType(
     }
 
     Type formalReturnType = methodSymbol.getReturnType();
-    // check nullability of parameters only for generics
-    if (formalReturnType.getTypeArguments().isEmpty()) {
-      return;
-    }
     Type returnExpressionType = getTreeType(retExpr, state);
     if (formalReturnType != null && returnExpressionType != null) {
       boolean isReturnTypeValid =
@@ -514,28 +511,24 @@ public static void compareGenericTypeParameterNullabilityForCall(
     }
     for (int i = 0; i < n; i++) {
       Type formalParameter = formalParams.get(i).type;
-      if (!formalParameter.getTypeArguments().isEmpty()) {
-        Type actualParameter = getTreeType(actualParams.get(i), state);
-        if (actualParameter != null) {
-          if (!subtypeParameterNullability(formalParameter, actualParameter, state)) {
-            reportInvalidParametersNullabilityError(
-                formalParameter, actualParameter, actualParams.get(i), state, analysis);
-          }
+      Type actualParameter = getTreeType(actualParams.get(i), state);
+      if (actualParameter != null) {
+        if (!subtypeParameterNullability(formalParameter, actualParameter, state)) {
+          reportInvalidParametersNullabilityError(
+              formalParameter, actualParameter, actualParams.get(i), state, analysis);
         }
       }
     }
     if (isVarArgs && !formalParams.isEmpty()) {
       Type.ArrayType varargsArrayType =
           (Type.ArrayType) formalParams.get(formalParams.size() - 1).type;
       Type varargsElementType = varargsArrayType.elemtype;
-      if (!varargsElementType.getTypeArguments().isEmpty()) {
-        for (int i = formalParams.size() - 1; i < actualParams.size(); i++) {
-          Type actualParameter = getTreeType(actualParams.get(i), state);
-          if (actualParameter != null) {
-            if (!subtypeParameterNullability(varargsElementType, actualParameter, state)) {
-              reportInvalidParametersNullabilityError(
-                  varargsElementType, actualParameter, actualParams.get(i), state, analysis);
-            }
+      for (int i = formalParams.size() - 1; i < actualParams.size(); i++) {
+        Type actualParameter = getTreeType(actualParams.get(i), state);
+        if (actualParameter != null) {
+          if (!subtypeParameterNullability(varargsElementType, actualParameter, state)) {
+            reportInvalidParametersNullabilityError(
+                varargsElementType, actualParameter, actualParams.get(i), state, analysis);
           }
         }
       }

diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java
@@ -278,6 +278,56 @@ public void arraysAndGenerics() {
         .doTest();
   }
 
+  @Test
+  public void genericArraysReturnedAndPassed() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  static class Foo<T> {}",
+            "  static class Bar<T> {",
+            "    Foo<T>[] getFoosPositive() {",
+            "      @Nullable Foo<T>[] result = new Foo[0];",
+            "      // BUG: Diagnostic contains: Cannot return expression of type @Nullable Foo<T>[] from method",
+            "      return result;",
+            "    }",
+            "    Foo<T>[] getFoosNegative() {",
+            "      Foo<T>[] result = new Foo[0];",
+            "      return result;",
+            "    }",
+            "    void takeFoos(Foo<T>[] foos) {}",
+            "    void callTakeFoosPositive(@Nullable Foo<T>[] p) {",
+            "      // BUG: Diagnostic contains: Cannot pass parameter of type @Nullable Foo<T>[]",
+            "      takeFoos(p);",
+            "    }",
+            "    void callTakeFoosNegative(Foo<T>[] p) {",
+            "      takeFoos(p);",
+            "    }",
+            "    void takeFoosVarargs(Foo<T>[]... foos) {}",
+            "    void callTakeFoosVarargsPositive(@Nullable Foo<T>[] p, Foo<T>[] p2) {",
+            "      // Under the hood, a @Nullable Foo<T>[][] is passed, which is not a subtype",
+            "      // of the formal parameter type Foo<T>[][]",
+            "      // BUG: Diagnostic contains: Cannot pass parameter of type @Nullable Foo<T>[]",
+            "      takeFoosVarargs(p);",
+            "      // BUG: Diagnostic contains: Cannot pass parameter of type @Nullable Foo<T>[]",
+            "      takeFoosVarargs(p2, p);",
+            "    }",
+            "    void callTakeFoosVarargsNegative(Foo<T>[] p) {",
+            "      takeFoosVarargs(p);",
+            "    }",
+            "    void takeNullableFoosVarargs(@Nullable Foo<T>[]... foos) {}",
+            "    void callTakeNullableFoosVarargsNegative(@Nullable Foo<T>[] p1, Foo<T>[] p2) {",
+            "      takeNullableFoosVarargs(p1);",
+            "      takeNullableFoosVarargs(p2);",
+            "      takeNullableFoosVarargs(p1, p2);",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void overridesReturnType() {
     makeHelper()