When inspecting instantiations of anonymous classes for CheckReturnVa…

…lue'd

ness, look at the @crv annotation of the constructor being called, instead of
the synthetic constructor invented locally.

is considered @CanIgnoreReturnValue or @CheckReturnValue (by a direct or indirect annotation),
consider the anonymous class construction to be identically-annotated.

RELNOTES=When creating an anonymous class where the superclass's constructor
PiperOrigin-RevId: 439834216

core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java

@@ -17,6 +17,7 @@
 package com.google.errorprone.bugpatterns;
 
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
 import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
 
 import com.google.auto.value.AutoValue;
@@ -32,7 +33,9 @@
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ExpressionStatementTree;
 import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.tools.javac.code.Symbol;
@@ -96,13 +99,42 @@ public CheckReturnValue(ErrorProneFlags flags) {
   @Override
   public Matcher<ExpressionTree> specializedMatcher() {
     return (tree, state) -> {
-      Optional<MethodSymbol> sym = methodSymbol(tree);
+      Optional<MethodSymbol> sym = methodToInspect(tree);
       return sym.flatMap(CheckReturnValue::firstAnnotation)
           .map(found -> found.annotation().equals(CHECK_RETURN_VALUE))
           .orElse(checkAllConstructors && sym.map(MethodSymbol::isConstructor).orElse(false));
     };
   }
 
+  private static Optional<MethodSymbol> methodToInspect(ExpressionTree tree) {
+    // If we're in the middle of calling an anonymous class, we want to actually look at the
+    // corresponding constructor of the supertype (e.g.: if I extend a class with a @CIRV
+    // constructor that I delegate to, then my anonymous class's constructor should *also* be
+    // considered @CIRV).
+    if (tree instanceof NewClassTree) {
+      ClassTree anonymousClazz = ((NewClassTree) tree).getClassBody();
+      if (anonymousClazz != null) {
+        // There should be a single defined constructor in the anonymous class body
+        var constructor =
+            anonymousClazz.getMembers().stream()
+                .filter(MethodTree.class::isInstance)
+                .map(MethodTree.class::cast)
+                .filter(mt -> getSymbol(mt).isConstructor())
+                .findFirst();
+
+        // and its first statement should be a super() call to the method in question.
+        return constructor
+            .map(MethodTree::getBody)
+            .map(block -> block.getStatements().get(0))
+            .map(ExpressionStatementTree.class::cast)
+            .map(ExpressionStatementTree::getExpression)
+            .map(MethodInvocationTree.class::cast)
+            .map(ASTHelpers::getSymbol);
+      }
+    }
+    return methodSymbol(tree);
+  }
+
   private static Optional<MethodSymbol> methodSymbol(ExpressionTree tree) {
     Symbol sym = ASTHelpers.getSymbol(tree);
     return sym instanceof MethodSymbol ? Optional.of((MethodSymbol) sym) : Optional.empty();

core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java

@@ -705,6 +705,89 @@ public void constructor_superCall() {
         .doTest();
   }
 
+  @Test
+  public void constructor_anonymousClassInheritsCIRV() {
+    compilationHelperLookingAtAllConstructors()
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  @com.google.errorprone.annotations.CanIgnoreReturnValue",
+            "  public Test() {}",
+            "  public static void foo() {",
+            "    new Test() {};",
+            "    new Test() {{ System.out.println(\"Lookie, instance initializer\"); }};",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void constructor_anonymousClassInheritsCRV() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  @com.google.errorprone.annotations.CheckReturnValue",
+            "  public Test() {}",
+            "  public static void foo() {",
+            "    // BUG: Diagnostic contains: ",
+            "    new Test() {};",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void constructor_hasOuterInstance() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  class Inner {",
+            "    @com.google.errorprone.annotations.CheckReturnValue",
+            "    public Inner() {}",
+            "  }",
+            "  public static void foo() {",
+            "    // BUG: Diagnostic contains: ",
+            "    new Test().new Inner() {};",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void constructor_anonymousClassInheritsCRV_syntheticConstructor() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  @com.google.errorprone.annotations.CheckReturnValue",
+            "  static class Nested {}",
+            "  public static void foo() {",
+            "    // BUG: Diagnostic contains: ",
+            "    new Nested() {};", // The "called" constructor is synthetic, but within @CRV Nested
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void constructor_inheritsFromCrvInterface() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  @com.google.errorprone.annotations.CheckReturnValue",
+            "  static interface IFace {}",
+            "  public static void foo() {",
+            //  TODO(b/226203690): It's arguable that this might need to be @CRV?
+            //   The superclass of the anonymous class is Object, not IFace, but /shrug
+            "    new IFace() {};",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void constructor_throwingContexts() {
     compilationHelper

core/src/test/java/com/google/errorprone/bugpatterns/testdata/CheckReturnValuePositiveCases.java

@@ -77,22 +77,7 @@ public void testBeforeAndAfterRule() {
   }
 
   public void constructor() {
-    /*
-     * We may or may not want to treat this as a bug. On the one hand, the
-     * subclass might be "using" the superclass, so it might not be being
-     * "ignored." (Plus, it would be a pain to produce a valid suggested fix
-     * that incorporates any subclass constructor body, which might even contain
-     * calls to methods in the class.) On the other hand, the more likely
-     * scenario may be a class like IteratorTester, which requires (a) that the
-     * user subclass it to implement a method and (b) that the user call test()
-     * on the constructed object. There, it would be nice if IteratorTester
-     * could be annotated with @CheckReturnValue to mean "anyone who creates an
-     * anonymous subclasses of this should still do something with that
-     * subclass." But perhaps that's an abuse of @CheckReturnValue.
-     *
-     * Anyway, these tests are here to ensure that subclasses don't don't crash
-     * the compiler.
-     */
+    // BUG: Diagnostic contains: Ignored return value
     new MyObject() {};
 
     class MySubObject1 extends MyObject {}