Properly check generic method overriding in explicitly-typed anonymou…

…s classes (#808)

This completes a task lingering from #755, which did not handle
overriding in anonymous classes. This case is slightly tricky as javac
does not preserve annotations on type arguments in the types it computes
for anonymous classes. To fix, we pass a `Symbol` where we were passing
a `Type` in a couple places, as then we can call `Symbol.isAnonymous()`
to check for an anonymous type. In such a case, we try to get the type
from the enclosing `NewClassTree` of the overriding method. Note that
this PR still does not handle anonymous classes that use the diamond
operator; we add a test to show the gaps.

nullaway/build.gradle

@@ -123,6 +123,8 @@ def jdk8Test = tasks.register("testJdk8", Test) {
     testClassesDirs = testTask.testClassesDirs
     jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}"
     filter {
+        // JDK 8 does not support diamonds on anonymous classes
+        excludeTestsMatching "com.uber.nullaway.NullAwayJSpecifyGenericsTests.overrideDiamondAnonymousClass"
         // tests cannot run on JDK 8 since Mockito version no longer supports it
         excludeTestsMatching "com.uber.nullaway.NullAwaySerializationTest.initializationError"
         excludeTestsMatching "com.uber.nullaway.handlers.contract.ContractUtilsTest.getEmptyAntecedent"

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

@@ -1,5 +1,6 @@
 package com.uber.nullaway;
 
+import static com.google.common.base.Verify.verify;
 import static com.uber.nullaway.NullabilityUtil.castToNonNull;
 import static java.util.stream.Collectors.joining;
 
@@ -22,6 +23,7 @@
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import com.sun.source.util.SimpleTreeVisitor;
+import com.sun.source.util.TreePath;
 import com.sun.tools.javac.code.Attribute;
 import com.sun.tools.javac.code.BoundKind;
 import com.sun.tools.javac.code.Symbol;
@@ -686,14 +688,53 @@ public static void checkTypeParameterNullnessForMethodOverriding(
    * String}.
    *
    * @param method the generic method
-   * @param enclosingType the enclosing type in which we want to know {@code method}'s return type
-   *     nullability
+   * @param enclosingSymbol the enclosing class in which we want to know {@code method}'s return
+   *     type nullability
    * @param state Visitor state
    * @param config The analysis config
    * @return nullability of the return type of {@code method} in the context of {@code
    *     enclosingType}
    */
   public static Nullness getGenericMethodReturnTypeNullness(
+      Symbol.MethodSymbol method, Symbol enclosingSymbol, VisitorState state, Config config) {
+    Type enclosingType = getTypeForSymbol(enclosingSymbol, state);
+    if (enclosingType == null) {
+      // we have no additional information from generics, so return NONNULL (presence of a @Nullable
+      // annotation should have been handled by the caller)
+      return Nullness.NONNULL;
+    }
+    return getGenericMethodReturnTypeNullness(method, enclosingType, state, config);
+  }
+
+  /**
+   * Get the type for the symbol, accounting for anonymous classes
+   *
+   * @param symbol the symbol
+   * @param state the visitor state
+   * @return the type for {@code symbol}
+   */
+  @Nullable
+  private static Type getTypeForSymbol(Symbol symbol, VisitorState state) {
+    if (symbol.isAnonymous()) {
+      // For anonymous classes, symbol.type does not contain annotations on generic type parameters.
+      // So, we get a correct type from the enclosing NewClassTree.
+      TreePath path = state.getPath();
+      NewClassTree newClassTree = ASTHelpers.findEnclosingNode(path, NewClassTree.class);
+      if (newClassTree == null) {
+        throw new RuntimeException(
+            "method should be inside a NewClassTree " + state.getSourceForNode(path.getLeaf()));
+      }
+      Type typeFromTree = getTreeType(newClassTree, state);
+      if (typeFromTree != null) {
+        verify(state.getTypes().isAssignable(symbol.type, typeFromTree));
+      }
+      return typeFromTree;
+    } else {
+      return symbol.type;
+    }
+  }
+
+  private static Nullness getGenericMethodReturnTypeNullness(
       Symbol.MethodSymbol method, Type enclosingType, VisitorState state, Config config) {
     Type overriddenMethodType = state.getTypes().memberType(enclosingType, method);
     if (!(overriddenMethodType instanceof Type.MethodType)) {
@@ -743,7 +784,7 @@ public static Nullness getGenericReturnNullnessAtInvocation(
     }
     Type methodReceiverType =
         castToNonNull(
-            ASTHelpers.getType(((MemberSelectTree) tree.getMethodSelect()).getExpression()));
+            getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression(), state));
     return getGenericMethodReturnTypeNullness(
         invokedMethodSymbol, methodReceiverType, state, config);
   }
@@ -791,11 +832,11 @@ public static Nullness getGenericParameterNullnessAtInvocation(
     if (!(tree.getMethodSelect() instanceof MemberSelectTree)) {
       return Nullness.NONNULL;
     }
-    Type methodReceiverType =
+    Type enclosingType =
         castToNonNull(
-            ASTHelpers.getType(((MemberSelectTree) tree.getMethodSelect()).getExpression()));
+            getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression(), state));
     return getGenericMethodParameterNullness(
-        paramIndex, invokedMethodSymbol, methodReceiverType, state, config);
+        paramIndex, invokedMethodSymbol, enclosingType, state, config);
   }
 
   /**
@@ -822,6 +863,35 @@ public static Nullness getGenericParameterNullnessAtInvocation(
    *
    * @param parameterIndex index of the parameter
    * @param method the generic method
+   * @param enclosingSymbol the enclosing symbol in which we want to know {@code method}'s parameter
+   *     type nullability
+   * @param state the visitor state
+   * @param config the config
+   * @return nullability of the relevant parameter type of {@code method} in the context of {@code
+   *     enclosingSymbol}
+   */
+  public static Nullness getGenericMethodParameterNullness(
+      int parameterIndex,
+      Symbol.MethodSymbol method,
+      Symbol enclosingSymbol,
+      VisitorState state,
+      Config config) {
+    Type enclosingType = getTypeForSymbol(enclosingSymbol, state);
+    if (enclosingType == null) {
+      // we have no additional information from generics, so return NONNULL (presence of a @Nullable
+      // annotation should have been handled by the caller)
+      return Nullness.NONNULL;
+    }
+    return getGenericMethodParameterNullness(parameterIndex, method, enclosingType, state, config);
+  }
+
+  /**
+   * Just like {@link #getGenericMethodParameterNullness(int, Symbol.MethodSymbol, Symbol,
+   * VisitorState, Config)}, but takes the enclosing {@code Type} rather than the enclosing {@code
+   * Symbol}.
+   *
+   * @param parameterIndex index of the parameter
+   * @param method the generic method
    * @param enclosingType the enclosing type in which we want to know {@code method}'s parameter
    *     type nullability
    * @param state the visitor state

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -738,7 +738,7 @@ private Description checkParamOverriding(
                     ? GenericsChecks.getGenericMethodParameterNullness(
                         i,
                         overriddenMethod,
-                        overridingParamSymbols.get(i).owner.owner.type,
+                        overridingParamSymbols.get(i).owner.owner,
                         state,
                         config)
                     : Nullness.NONNULL);
@@ -944,7 +944,7 @@ private Description checkOverriding(
     // if the super method returns nonnull, overriding method better not return nullable
     // Note that, for the overriding method, the permissive default is non-null,
     // but it's nullable for the overridden one.
-    if (overriddenMethodReturnsNonNull(overriddenMethod, overridingMethod.owner.type, state)
+    if (overriddenMethodReturnsNonNull(overriddenMethod, overridingMethod.owner, state)
         && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL)
             .equals(Nullness.NULLABLE)
         && (memberReferenceTree == null
@@ -983,7 +983,7 @@ && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL)
   }
 
   private boolean overriddenMethodReturnsNonNull(
-      Symbol.MethodSymbol overriddenMethod, Type overridingMethodType, VisitorState state) {
+      Symbol.MethodSymbol overriddenMethod, Symbol enclosingSymbol, VisitorState state) {
     Nullness methodReturnNullness =
         getMethodReturnNullness(overriddenMethod, state, Nullness.NULLABLE);
     if (!methodReturnNullness.equals(Nullness.NONNULL)) {
@@ -993,7 +993,7 @@ private boolean overriddenMethodReturnsNonNull(
     // using the type parameters from the type enclosing the overriding method
     if (config.isJSpecifyMode()) {
       return GenericsChecks.getGenericMethodReturnTypeNullness(
-              overriddenMethod, overridingMethodType, state, config)
+              overriddenMethod, enclosingSymbol, state, config)
           .equals(Nullness.NONNULL);
     }
     return true;

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

@@ -2,6 +2,7 @@
 
 import com.google.errorprone.CompilationTestHelper;
 import java.util.Arrays;
+import org.junit.Ignore;
 import org.junit.Test;
 
 public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase {
@@ -974,6 +975,163 @@ public void overrideParameterType() {
         .doTest();
   }
 
+  @Test
+  public void overrideExplicitlyTypedAnonymousClass() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  interface Fn<P extends @Nullable Object, R extends @Nullable Object> {",
+            "    R apply(P p);",
+            "  }",
+            "  static abstract class FnClass<P extends @Nullable Object, R extends @Nullable Object> {",
+            "    abstract R apply(P p);",
+            "  }",
+            "  static void anonymousClasses() {",
+            "    Fn<@Nullable String, String> fn1 = new Fn<@Nullable String, String>() {",
+            "      // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method",
+            "      public String apply(String s) { return s; }",
+            "    };",
+            "    FnClass<String, String> fn2 = new FnClass<String, String>() {",
+            "      // BUG: Diagnostic contains: method returns @Nullable, but superclass method",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    };",
+            "    Fn<String, @Nullable String> fn3 = new Fn<String, @Nullable String>() {",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    };",
+            "    FnClass<@Nullable String, String> fn4 = new FnClass<@Nullable String, String>() {",
+            "      public String apply(@Nullable String s) { return \"hello\"; }",
+            "    };",
+            "  }",
+            "  static void anonymousClassesFullName() {",
+            "    Test.Fn<@Nullable String, String> fn1 = new Test.Fn<@Nullable String, String>() {",
+            "      // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method",
+            "      public String apply(String s) { return s; }",
+            "    };",
+            "    Test.FnClass<String, String> fn2 = new Test.FnClass<String, String>() {",
+            "      // BUG: Diagnostic contains: method returns @Nullable, but superclass method",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    };",
+            "    Test.Fn<String, @Nullable String> fn3 = new Test.Fn<String, @Nullable String>() {",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    };",
+            "    Test.FnClass<@Nullable String, String> fn4 = new Test.FnClass<@Nullable String, String>() {",
+            "      public String apply(@Nullable String s) { return \"hello\"; }",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Ignore("https://github.com/uber/NullAway/issues/836")
+  @Test
+  public void overrideAnonymousNestedClass() {
+    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);",
+            "    }",
+            "  }",
+            "  void anonymousNestedClasses() {",
+            "    Wrapper<@Nullable String>.Fn<String> fn1 = (this.new Wrapper<@Nullable String>()).new Fn<String>() {",
+            "      // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method",
+            "      public String apply(String s) { return s; }",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void explicitlyTypedAnonymousClassAsReceiver() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  interface Fn<P extends @Nullable Object, R extends @Nullable Object> {",
+            "    R apply(P p);",
+            "  }",
+            "  static abstract class FnClass<P extends @Nullable Object, R extends @Nullable Object> {",
+            "    abstract R apply(P p);",
+            "  }",
+            "  static void anonymousClasses() {",
+            "    String s1 = (new Fn<String, @Nullable String>() {",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    }).apply(\"hi\");",
+            "    // BUG: Diagnostic contains: dereferenced expression s1",
+            "    s1.hashCode();",
+            "    String s2 = (new FnClass<String, @Nullable String>() {",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    }).apply(\"hi\");",
+            "    // BUG: Diagnostic contains: dereferenced expression s2",
+            "    s2.hashCode();",
+            "    (new Fn<String, String>() {",
+            "      public String apply(String s) { return \"hi\"; }",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter",
+            "    }).apply(null);",
+            "    (new FnClass<String, String>() {",
+            "      public String apply(String s) { return \"hi\"; }",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter",
+            "    }).apply(null);",
+            "    (new Fn<@Nullable String, String>() {",
+            "      public String apply(@Nullable String s) { return \"hi\"; }",
+            "    }).apply(null);",
+            "    (new FnClass<@Nullable String, String>() {",
+            "      public String apply(@Nullable String s) { return \"hi\"; }",
+            "    }).apply(null);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  /** Diamond anonymous classes are not supported yet; tests are for future reference */
+  @Test
+  public void overrideDiamondAnonymousClass() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  interface Fn<P extends @Nullable Object, R extends @Nullable Object> {",
+            "    R apply(P p);",
+            "  }",
+            "  static abstract class FnClass<P extends @Nullable Object, R extends @Nullable Object> {",
+            "    abstract R apply(P p);",
+            "  }",
+            "  static void anonymousClasses() {",
+            "    Fn<@Nullable String, String> fn1 = new Fn<>() {",
+            "      // TODO: should report a bug here",
+            "      public String apply(String s) { return s; }",
+            "    };",
+            "    FnClass<@Nullable String, String> fn2 = new FnClass<>() {",
+            "      // TODO: should report a bug here",
+            "      public String apply(String s) { return s; }",
+            "    };",
+            "    Fn<String, @Nullable String> fn3 = new Fn<>() {",
+            "      // TODO: this is a false positive",
+            "      // BUG: Diagnostic contains: method returns @Nullable, but superclass method",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    };",
+            "    FnClass<String, @Nullable String> fn4 = new FnClass<>() {",
+            "      // TODO: this is a false positive",
+            "      // BUG: Diagnostic contains: method returns @Nullable, but superclass method",
+            "      public @Nullable String apply(String s) { return null; }",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void nullableGenericTypeVariableReturnType() {
     makeHelper()