JSpecify: initial handling of generic enclosing types for inner class…

…es (#837) This is a step towards fixing #836. We now properly handle the case of a generic enclosing type for an inner class type, both in checking matching of type argument nullability and in pretty-printing for error messages. We still don't handle `NewClassTree` types correctly which is why #836 is not yet fixed. Also fixes a bug in where generics checks were performed for `return` statements. Previously we would only check generic type arguments if the top-level nullability of the return type was `@NonNull`.
uber · Oct 18, 2023 · 5355c7c · 5355c7c
1 parent e7623f7
commit 5355c7c
Show file tree

Hide file tree

Showing 3 changed files with 124 additions and 65 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java
@@ -129,9 +129,9 @@ private static void reportInvalidAssignmentInstantiationError(
             ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE,
             String.format(
                 "Cannot assign from type "
-                    + prettyTypeForError(rhsType)
+                    + prettyTypeForError(rhsType, state)
                     + " to type "
-                    + prettyTypeForError(lhsType)
+                    + prettyTypeForError(lhsType, state)
                     + " due to mismatched nullability of type parameters"));
     state.reportMatch(
         errorBuilder.createErrorDescription(
@@ -146,9 +146,9 @@ private static void reportInvalidReturnTypeError(
             ErrorMessage.MessageTypes.RETURN_NULLABLE_GENERIC,
             String.format(
                 "Cannot return expression of type "
-                    + prettyTypeForError(returnType)
+                    + prettyTypeForError(returnType, state)
                     + " from method with return type "
-                    + prettyTypeForError(methodType)
+                    + prettyTypeForError(methodType, state)
                     + " due to mismatched nullability of type parameters"));
     state.reportMatch(
         errorBuilder.createErrorDescription(
@@ -163,9 +163,9 @@ private static void reportMismatchedTypeForTernaryOperator(
             ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE,
             String.format(
                 "Conditional expression must have type "
-                    + prettyTypeForError(expressionType)
+                    + prettyTypeForError(expressionType, state)
                     + " but the sub-expression has type "
-                    + prettyTypeForError(subPartType)
+                    + prettyTypeForError(subPartType, state)
                     + ", which has mismatched nullability of type parameters"));
     state.reportMatch(
         errorBuilder.createErrorDescription(
@@ -183,9 +183,9 @@ private static void reportInvalidParametersNullabilityError(
         new ErrorMessage(
             ErrorMessage.MessageTypes.PASS_NULLABLE_GENERIC,
             "Cannot pass parameter of type "
-                + prettyTypeForError(actualParameterType)
+                + prettyTypeForError(actualParameterType, state)
                 + ", as formal parameter has type "
-                + prettyTypeForError(formalParameterType)
+                + prettyTypeForError(formalParameterType, state)
                 + ", which has mismatched type parameter nullability");
     state.reportMatch(
         errorBuilder.createErrorDescription(
@@ -203,9 +203,9 @@ private static void reportInvalidOverridingMethodReturnTypeError(
         new ErrorMessage(
             ErrorMessage.MessageTypes.WRONG_OVERRIDE_RETURN_GENERIC,
             "Method returns "
-                + prettyTypeForError(overridingMethodReturnType)
+                + prettyTypeForError(overridingMethodReturnType, state)
                 + ", but overridden method returns "
-                + prettyTypeForError(overriddenMethodReturnType)
+                + prettyTypeForError(overriddenMethodReturnType, state)
                 + ", which has mismatched type parameter nullability");
     state.reportMatch(
         errorBuilder.createErrorDescription(
@@ -223,9 +223,9 @@ private static void reportInvalidOverridingMethodParamTypeError(
         new ErrorMessage(
             ErrorMessage.MessageTypes.WRONG_OVERRIDE_PARAM_GENERIC,
             "Parameter has type "
-                + prettyTypeForError(methodParamType)
+                + prettyTypeForError(methodParamType, state)
                 + ", but overridden method has parameter type "
-                + prettyTypeForError(typeParameterType)
+                + prettyTypeForError(typeParameterType, state)
                 + ", which has mismatched type parameter nullability");
     state.reportMatch(
         errorBuilder.createErrorDescription(
@@ -546,7 +546,10 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
           return false;
         }
       }
-      return true;
+      // If there is an enclosing type (for non-static inner classes), its type argument nullability
+      // should also match.  When there is no enclosing type, getEnclosingType() returns a NoType
+      // object, which gets handled by the fallback visitType() method
+      return lhsType.getEnclosingType().accept(this, rhsType.getEnclosingType());
     }
 
     @Override
@@ -992,57 +995,66 @@ private static Nullness getTypeNullness(Type type, Config config) {
    * Returns a pretty-printed representation of type suitable for error messages. The representation
    * uses simple names rather than fully-qualified names, and retains all type-use annotations.
    */
-  public static String prettyTypeForError(Type type) {
-    return type.accept(PRETTY_TYPE_VISITOR, null);
+  public static String prettyTypeForError(Type type, VisitorState state) {
+    return type.accept(new PrettyTypeVisitor(state), null);
   }
 
   /** This code is a modified version of code in {@link com.google.errorprone.util.Signatures} */
-  private static final Type.Visitor<String, Void> PRETTY_TYPE_VISITOR =
-      new Types.DefaultTypeVisitor<String, Void>() {
-        @Override
-        public String visitWildcardType(Type.WildcardType t, Void unused) {
-          StringBuilder sb = new StringBuilder();
-          sb.append(t.kind);
-          if (t.kind != BoundKind.UNBOUND) {
-            sb.append(t.type.accept(this, null));
-          }
-          return sb.toString();
-        }
+  private static final class PrettyTypeVisitor extends Types.DefaultTypeVisitor<String, Void> {
 
-        @Override
-        public String visitClassType(Type.ClassType t, Void s) {
-          StringBuilder sb = new StringBuilder();
-          for (Attribute.TypeCompound compound : t.getAnnotationMirrors()) {
-            sb.append('@');
-            sb.append(compound.type.accept(this, null));
-            sb.append(' ');
-          }
-          sb.append(t.tsym.getSimpleName());
-          if (t.getTypeArguments().nonEmpty()) {
-            sb.append('<');
-            sb.append(
-                t.getTypeArguments().stream()
-                    .map(a -> a.accept(this, null))
-                    .collect(joining(", ")));
-            sb.append(">");
-          }
-          return sb.toString();
-        }
+    private final VisitorState state;
 
-        @Override
-        public String visitCapturedType(Type.CapturedType t, Void s) {
-          return t.wildcard.accept(this, null);
-        }
+    PrettyTypeVisitor(VisitorState state) {
+      this.state = state;
+    }
 
-        @Override
-        public String visitArrayType(Type.ArrayType t, Void unused) {
-          // TODO properly print cases like int @Nullable[]
-          return t.elemtype.accept(this, null) + "[]";
-        }
+    @Override
+    public String visitWildcardType(Type.WildcardType t, Void unused) {
+      // NOTE: we have not tested this code yet as we do not yet support wildcard types
+      StringBuilder sb = new StringBuilder();
+      sb.append(t.kind);
+      if (t.kind != BoundKind.UNBOUND) {
+        sb.append(t.type.accept(this, null));
+      }
+      return sb.toString();
+    }
 
-        @Override
-        public String visitType(Type t, Void s) {
-          return t.toString();
-        }
-      };
+    @Override
+    public String visitClassType(Type.ClassType t, Void s) {
+      StringBuilder sb = new StringBuilder();
+      Type enclosingType = t.getEnclosingType();
+      if (!ASTHelpers.isSameType(enclosingType, Type.noType, state)) {
+        sb.append(enclosingType.accept(this, null)).append('.');
+      }
+      for (Attribute.TypeCompound compound : t.getAnnotationMirrors()) {
+        sb.append('@');
+        sb.append(compound.type.accept(this, null));
+        sb.append(' ');
+      }
+      sb.append(t.tsym.getSimpleName());
+      if (t.getTypeArguments().nonEmpty()) {
+        sb.append('<');
+        sb.append(
+            t.getTypeArguments().stream().map(a -> a.accept(this, null)).collect(joining(", ")));
+        sb.append(">");
+      }
+      return sb.toString();
+    }
+
+    @Override
+    public String visitCapturedType(Type.CapturedType t, Void s) {
+      return t.wildcard.accept(this, null);
+    }
+
+    @Override
+    public String visitArrayType(Type.ArrayType t, Void unused) {
+      // TODO properly print cases like int @Nullable[]
+      return t.elemtype.accept(this, null) + "[]";
+    }
+
+    @Override
+    public String visitType(Type t, Void s) {
+      return t.toString();
+    }
+  }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -867,6 +867,10 @@ private Description checkReturnExpression(
       // support)
       return Description.NO_MATCH;
     }
+    // Do the generics checks here, since we need to check the type arguments regardless of the
+    // top-level nullability of the return type
+    GenericsChecks.checkTypeParameterNullnessForFunctionReturnType(
+        retExpr, methodSymbol, this, state);
     if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) {
       return Description.NO_MATCH;
     }
@@ -880,8 +884,6 @@ private Description checkReturnExpression(
           state,
           methodSymbol);
     }
-    GenericsChecks.checkTypeParameterNullnessForFunctionReturnType(
-        retExpr, methodSymbol, this, state);
     return Description.NO_MATCH;
   }
 

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
@@ -192,6 +192,51 @@ public void testOKNewClassInstantiationForOtherAnnotations() {
         .doTest();
   }
 
+  @Test
+  public void nestedGenericTypes() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  class Wrapper<P extends @Nullable Object> {",
+            "    abstract class Fn<R extends @Nullable Object> {",
+            "      abstract R apply(P p);",
+            "    }",
+            "  }",
+            "  static void param(@Nullable Wrapper<String>.Fn<String> p) {}",
+            "  static void positiveParam() {",
+            "    Wrapper<@Nullable String>.Fn<String> x = null;",
+            "    // BUG: Diagnostic contains: Cannot pass parameter of type Test.Wrapper<@Nullable String>.Fn<String>",
+            "    param(x);",
+            "  }",
+            "  static void positiveAssign() {",
+            "    Wrapper<@Nullable String>.Fn<String> p1 = null;",
+            "    // BUG: Diagnostic contains: Cannot assign from type Test.Wrapper<@Nullable String>.Fn<String> to type Test.Wrapper<String>.Fn<String>",
+            "    Wrapper<String>.Fn<String> p2 = p1;",
+            "  }",
+            "  static @Nullable Wrapper<String>.Fn<String> positiveReturn() {",
+            "    Wrapper<@Nullable String>.Fn<String> p1 = null;",
+            "    // BUG: Diagnostic contains: Cannot return expression of type Test.Wrapper<@Nullable String>.Fn<String>",
+            "    return p1;",
+            "  }",
+            "  static void negativeParam() {",
+            "    Wrapper<String>.Fn<String> x = null;",
+            "    param(x);",
+            "  }",
+            "  static void negativeAssign() {",
+            "    Wrapper<@Nullable String>.Fn<String> p1 = null;",
+            "    Wrapper<@Nullable String>.Fn<String> p2 = p1;",
+            "  }",
+            "  static @Nullable Wrapper<@Nullable String>.Fn<String> negativeReturn() {",
+            "    Wrapper<@Nullable String>.Fn<String> p1 = null;",
+            "    return p1;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void downcastInstantiation() {
     makeHelper()
@@ -280,7 +325,7 @@ public void superTypeAssignmentChecksSingleInterface() {
             "  interface Fn<P extends @Nullable Object, R extends @Nullable Object> {}",
             "  class FnImpl implements Fn<@Nullable String, @Nullable String> {}",
             "  void testPositive() {",
-            "    // BUG: Diagnostic contains: Cannot assign from type FnImpl",
+            "    // BUG: Diagnostic contains: Cannot assign from type Test.FnImpl",
             "    Fn<@Nullable String, String> f = new FnImpl();",
             "  }",
             "  void testNegative() {",
@@ -1173,14 +1218,14 @@ public void overrideWithNestedTypeParametersInReturnType() {
             " }",
             " class TestFunc1 implements Fn<P<@Nullable String, String>, @Nullable String> {",
             " @Override",
-            "  // BUG: Diagnostic contains: Method returns P<@Nullable String, @Nullable String>, but overridden method",
+            "  // BUG: Diagnostic contains: Method returns Test.P<@Nullable String, @Nullable String>, but overridden method",
             " public P<@Nullable String, @Nullable String> apply() {",
             "   return new P<@Nullable String, @Nullable String>();",
             "  }",
             " }",
             " class TestFunc2 implements Fn<P<@Nullable String, @Nullable String>, @Nullable String> {",
             "   @Override",
-            "   // BUG: Diagnostic contains: Method returns P<@Nullable String, String>, but overridden method returns",
+            "   // BUG: Diagnostic contains: Method returns Test.P<@Nullable String, String>, but overridden method returns",
             "   public P<@Nullable String, String> apply() {",
             "     return new P<@Nullable String, String>();",
             "   }",
@@ -1209,7 +1254,7 @@ public void overrideWithNestedTypeParametersInParameterType() {
             " }",
             " class TestFunc implements Fn<P<String, String>, String> {",
             " @Override",
-            "  // BUG: Diagnostic contains: Parameter has type P<@Nullable String, String>, but overridden method has parameter type P<String, String>",
+            "  // BUG: Diagnostic contains: Parameter has type Test.P<@Nullable String, String>, but overridden method has parameter type Test.P<String, String>",
             "  public String apply(P<@Nullable String, String> p, String s) {",
             "    return s;",
             "  }",