Modify AbstractReturnValueIgnored

to detect cases where the function is called in a method reference
targeting a void return type.

RELNOTES: Applies CheckReturnValue, ReturnValueIgnored,
FutureReturnValueIgnored, and PromiseReturnValueIgnored to detect cases
where the function is called in an expression-type lambda or method
reference targeting a void return type.

MOE_MIGRATED_REVID=165011260

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -234,6 +234,12 @@ public static MethodSymbol getSymbol(MethodInvocationTree tree) {
     return (MethodSymbol) sym;
   }
 
+  /** Gets the symbol for a member reference. */
+  public static MethodSymbol getSymbol(MemberReferenceTree tree) {
+    Symbol sym = ((JCMemberReference) tree).sym;
+    return sym instanceof MethodSymbol ? (MethodSymbol) sym : null;
+  }
+
   /** Given an ExpressionTree, removes any enclosing parentheses. */
   public static ExpressionTree stripParentheses(ExpressionTree tree) {
     while (tree instanceof ParenthesizedTree) {

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

@@ -33,6 +33,7 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
 import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.fixes.SuggestedFix;
@@ -43,6 +44,8 @@
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.LambdaExpressionTree;
+import com.sun.source.tree.MemberReferenceTree;
+import com.sun.source.tree.MemberReferenceTree.ReferenceMode;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.Tree;
@@ -51,15 +54,17 @@
 import com.sun.tools.javac.tree.JCTree.JCFieldAccess;
 import com.sun.tools.javac.tree.JCTree.JCIdent;
 import com.sun.tools.javac.tree.JCTree.JCLambda;
+import com.sun.tools.javac.tree.JCTree.JCMemberReference;
 import com.sun.tools.javac.tree.JCTree.JCMethodInvocation;
+import javax.lang.model.type.TypeKind;
 
 /**
  * An abstract base class to match method invocations in which the return value is not used.
  *
  * @author eaftan@google.com (Eddie Aftandilian)
  */
 public abstract class AbstractReturnValueIgnored extends BugChecker
-    implements MethodInvocationTreeMatcher {
+    implements MethodInvocationTreeMatcher, MemberReferenceTreeMatcher {
 
   @Override
   public Description matchMethodInvocation(
@@ -79,13 +84,46 @@ public Description matchMethodInvocation(
     return Description.NO_MATCH;
   }
 
+  @Override
+  public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) {
+    if (allOf(
+            (t, s) -> t.getMode() == ReferenceMode.INVOKE,
+            AbstractReturnValueIgnored::isVoidReturningMethodReferenceExpression,
+            // Skip cases where the method we're referencing really does return void. We're only
+            // looking for cases where the referenced method does not return void, but it's being
+            // used on a void-returning functional interface.
+            not((t, s) -> ASTHelpers.isVoidType(ASTHelpers.getSymbol(tree).getReturnType(), s)),
+            not((t, s) -> isThrowingFunctionalInterface(s, ((JCMemberReference) t).type)),
+            specializedMatcher())
+        .matches(tree, state)) {
+      return describeMatch(tree);
+    }
+
+    return Description.NO_MATCH;
+  }
+
+  private static boolean isVoidReturningMethodReferenceExpression(
+      MemberReferenceTree tree, VisitorState state) {
+    return functionalInterfaceReturnsExactlyVoid(((JCMemberReference) tree).type, state);
+  }
+
   private static boolean isVoidReturningLambdaExpression(Tree tree, VisitorState state) {
     if (!(tree instanceof LambdaExpressionTree)) {
       return false;
     }
 
-    return ASTHelpers.isVoidType(
-        state.getTypes().findDescriptorType(((JCLambda) tree).type).getReturnType(), state);
+    return functionalInterfaceReturnsExactlyVoid(((JCLambda) tree).type, state);
+  }
+
+  /**
+   * Checks that the return value of a functional interface is void. Note, we do not use
+   * ASTHelpers.isVoidType here, return values of Void are actually type-checked. Only
+   * void-returning functions silently ignore return values of any type.
+   */
+  private static boolean functionalInterfaceReturnsExactlyVoid(
+      Type interfaceType, VisitorState state) {
+    return state.getTypes().findDescriptorType(interfaceType).getReturnType().getKind()
+        == TypeKind.VOID;
   }
 
   private static boolean methodCallInDeclarationOfExceptionType(VisitorState state) {
@@ -107,8 +145,10 @@ private static boolean methodCallInDeclarationOfExceptionType(VisitorState state
       // Huh. Shouldn't happen.
       return false;
     }
-    Type clazzType = ASTHelpers.getType(tree);
+    return isThrowingFunctionalInterface(state, ASTHelpers.getType(tree));
+  }
 
+  private static boolean isThrowingFunctionalInterface(VisitorState state, Type clazzType) {
     return CLASSES_CONSIDERED_THROWING
         .stream()
         .anyMatch(t -> ASTHelpers.isSubtype(clazzType, state.getTypeFromString(t), state));
@@ -134,7 +174,7 @@ private static boolean methodCallInDeclarationOfExceptionType(VisitorState state
    * Match whatever additional conditions concrete subclasses want to match (a list of known
    * side-effect-free methods, has a @CheckReturnValue annotation, etc.).
    */
-  public abstract Matcher<? super MethodInvocationTree> specializedMatcher();
+  public abstract Matcher<? super ExpressionTree> specializedMatcher();
 
   private static Matcher<IdentifierTree> identifierHasName(final String name) {
     return (item, state) -> item.getName().contentEquals(name);

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

@@ -33,7 +33,7 @@
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ClassTree;
-import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Symbol.ClassSymbol;
@@ -79,11 +79,15 @@ private static Optional<Boolean> checkPackage(MethodSymbol method, VisitorState
     return shouldCheckReturnValue(enclosingPackage(method), state);
   }
 
-  private static final Matcher<MethodInvocationTree> MATCHER =
-      new Matcher<MethodInvocationTree>() {
+  private static final Matcher<ExpressionTree> MATCHER =
+      new Matcher<ExpressionTree>() {
         @Override
-        public boolean matches(MethodInvocationTree tree, VisitorState state) {
-          MethodSymbol method = ASTHelpers.getSymbol(tree);
+        public boolean matches(ExpressionTree tree, VisitorState state) {
+          Symbol sym = ASTHelpers.getSymbol(tree);
+          if (!(sym instanceof MethodSymbol)) {
+            return false;
+          }
+          MethodSymbol method = (MethodSymbol) sym;
           Optional<Boolean> result = shouldCheckReturnValue(method, state);
           if (result.isPresent()) {
             return result.get();
@@ -108,7 +112,7 @@ public boolean matches(MethodInvocationTree tree, VisitorState state) {
    * {@code @CheckReturnValue} annotation.
    */
   @Override
-  public Matcher<MethodInvocationTree> specializedMatcher() {
+  public Matcher<ExpressionTree> specializedMatcher() {
     return MATCHER;
   }
 

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

@@ -29,7 +29,7 @@
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
-import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import com.sun.tools.javac.code.Type;
 import java.util.Objects;
@@ -74,13 +74,17 @@ public final class FutureReturnValueIgnored extends AbstractReturnValueIgnored {
               .onExactClass("java.util.concurrent.CompletableFuture")
               .withNameMatching(Pattern.compile("completeAsync|orTimeout|completeOnTimeout")));
 
-  private static final Matcher<MethodInvocationTree> MATCHER =
-      new Matcher<MethodInvocationTree>() {
+  private static final Matcher<ExpressionTree> MATCHER =
+      new Matcher<ExpressionTree>() {
         @Override
-        public boolean matches(MethodInvocationTree tree, VisitorState state) {
+        public boolean matches(ExpressionTree tree, VisitorState state) {
           Type futureType =
               Objects.requireNonNull(state.getTypeFromString("java.util.concurrent.Future"));
-          MethodSymbol sym = ASTHelpers.getSymbol(tree);
+          Symbol untypedSymbol = ASTHelpers.getSymbol(tree);
+          if (!(untypedSymbol instanceof MethodSymbol)) {
+            return false;
+          }
+          MethodSymbol sym = (MethodSymbol) untypedSymbol;
           if (hasAnnotation(sym, CanIgnoreReturnValue.class, state)) {
             return false;
           }
@@ -106,7 +110,7 @@ public boolean matches(MethodInvocationTree tree, VisitorState state) {
       };
 
   @Override
-  public Matcher<? super MethodInvocationTree> specializedMatcher() {
+  public Matcher<? super ExpressionTree> specializedMatcher() {
     return MATCHER;
   }
 }

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

@@ -28,8 +28,8 @@
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
-import com.sun.source.tree.MethodInvocationTree;
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import com.sun.tools.javac.code.Type;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -76,11 +76,15 @@ public class ReturnValueIgnored extends AbstractReturnValueIgnored {
   /**
    * Methods in {@link java.util.function} are pure, and their returnvalues should not be discarded.
    */
-  private static final Matcher<MethodInvocationTree> FUNCTIONAL_METHOD =
+  private static final Matcher<ExpressionTree> FUNCTIONAL_METHOD =
       (tree, state) -> {
-        Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(tree);
-        return symbol != null
-            && symbol.owner.packge().getQualifiedName().contentEquals("java.util.function");
+        Symbol symbol = ASTHelpers.getSymbol(tree);
+        return symbol instanceof MethodSymbol
+            && ((MethodSymbol) symbol)
+                .owner
+                .packge()
+                .getQualifiedName()
+                .contentEquals("java.util.function");
       };
 
   /**
@@ -91,7 +95,7 @@ public class ReturnValueIgnored extends AbstractReturnValueIgnored {
       instanceMethod().onDescendantOf("java.util.stream.BaseStream");
 
   @Override
-  public Matcher<? super MethodInvocationTree> specializedMatcher() {
+  public Matcher<? super ExpressionTree> specializedMatcher() {
     return anyOf(RETURNS_SAME_TYPE, FUNCTIONAL_METHOD, STREAM_METHOD);
   }
 

core/src/main/java/com/google/errorprone/bugpatterns/android/RectIntersectReturnValueIgnored.java

@@ -25,6 +25,7 @@
 import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
 
 /** @author avenet@google.com (Arnaud J. Venet) */
@@ -44,7 +45,7 @@
 )
 public final class RectIntersectReturnValueIgnored extends AbstractReturnValueIgnored {
   @Override
-  public Matcher<? super MethodInvocationTree> specializedMatcher() {
+  public Matcher<? super ExpressionTree> specializedMatcher() {
     return instanceMethod().onExactClass("android.graphics.Rect").named("intersect");
   }
 

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

@@ -461,6 +461,7 @@ public void ignoreInThrowingRunnables() throws Exception {
             "       }",
             "     });",
             "   org.junit.Assert.assertThrows(IllegalStateException.class, () -> foo.f());",
+            "   org.junit.Assert.assertThrows(IllegalStateException.class, foo::f);",
             "   org.junit.Assert.assertThrows(IllegalStateException.class, () -> {",
             "      int bah = foo.f();",
             "      foo.f(); ",

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

@@ -36,6 +36,17 @@ private int mustCheck() {
     return 5;
   }
 
+  private void callRunnable(Runnable runnable) {
+    runnable.run();
+  }
+
+  private void testNonCheckedCallsWithMethodReferences() {
+    Object obj = new String();
+    callRunnable(String::new);
+    callRunnable(this::test2);
+    callRunnable(obj::toString);
+  }
+
   private void callSupplier(Supplier<Integer> supplier) {
     supplier.get();
   }

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

@@ -57,8 +57,7 @@ public void testResolvedToVoidLambda() {
   }
 
   public void testResolvedToVoidMethodReference() {
-    // TODO(b/62960293): This should be an error too, but it's tricky to adapt existing code to
-    // catch it.
+    // BUG: Diagnostic contains: Ignored return value
     callRunnable(this.intValue::increment);
   }
 

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

@@ -19,6 +19,7 @@
 import java.math.BigInteger;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.function.Function;
 
 /** @author alexeagle@google.com (Alex Eagle) */
 public class ReturnValueIgnoredNegativeCases {
@@ -44,4 +45,13 @@ public void methodDoesntMatch() {
   public void methodDoesntMatch2() {
     final String b = a.toString().trim();
   }
+
+  public void acceptFunctionOfVoid(Function<Integer, Void> arg) {
+    arg.apply(5);
+  }
+
+  public void passReturnValueCheckedMethodReferenceToFunctionVoid() {
+    Function<Integer, Void> fn = (i -> null);
+    acceptFunctionOfVoid(fn::apply);
+  }
 }