Improve MethodReferenceUsageCheck, document further improvements

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java

@@ -4,17 +4,26 @@ import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.errorprone.BugPattern.LinkType.NONE;
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 import static com.google.errorprone.BugPattern.StandardTags.STYLE;
+import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.Matchers.hasAnnotation;
+import static java.util.function.Predicate.not;
 
 import com.google.auto.service.AutoService;
 import com.google.common.base.VerifyException;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Streams;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
+import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.refaster.annotation.AfterTemplate;
+import com.google.errorprone.refaster.annotation.BeforeTemplate;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.BlockTree;
 import com.sun.source.tree.ExpressionStatementTree;
@@ -23,15 +32,21 @@ import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.ParenthesizedTree;
 import com.sun.source.tree.ReturnTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
 import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreePath;
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
 import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.code.Types;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Stream;
 import javax.lang.model.element.Name;
 
 /**
@@ -49,6 +64,8 @@ import javax.lang.model.element.Name;
 // `not(some::reference)`.
 // XXX: This check is extremely inefficient due to its reliance on `SuggestedFixes.compilesWithFix`.
 // Palantir's `LambdaMethodReference` check seems to suffer a similar issue at this time.
+// XXX: Does this check also do `() -> something.someConst` -> `something::someConst`?
+// XXX: Don't rewrite `() -> field.m()` to `field::m` for non-final fields.
 @AutoService(BugChecker.class)
 @BugPattern(
     summary = "Prefer method references over lambda expressions",
@@ -57,59 +74,140 @@ import javax.lang.model.element.Name;
     tags = STYLE)
 public final class MethodReferenceUsage extends BugChecker implements LambdaExpressionTreeMatcher {
   private static final long serialVersionUID = 1L;
+  private static final Matcher<Tree> REFASTER_TEMPLATE_METHOD =
+      anyOf(
+          hasAnnotation(AfterTemplate.class.getName()),
+          hasAnnotation(BeforeTemplate.class.getName()));
+  private static final String VERIFY_SUGGESTIONS_FLAG = "MethodReferenceUsage:VerifySuggestions";
+  /**
+   * Tells whether this class is loaded in inside a {@link SuggestedFixes#compilesWithFix(Fix,
+   * VisitorState, ImmutableList, boolean)} invocation. This allows {@link
+   * #tryCompileWithFix(VisitorState, SuggestedFix)} to prevent a very expensive chain of nested
+   * compilations.
+   */
+  // XXX: Review whether, given the most recent improvements, we need the `compilesWithFix` fallback
+  // at all. (Likely at least one open point remains: handling of generic types.)
+  private static final boolean IS_NESTED_INVOCATION =
+      StackWalker.getInstance()
+          .walk(
+              frames -> {
+                String className = MethodReferenceUsage.class.getCanonicalName();
+                return frames.filter(f -> f.getClassName().equals(className)).limit(2).count() > 1;
+              });
+
+  private final boolean verifySuggestions;
+
+  /** Instantiates the default {@link MethodReferenceUsage}. */
+  public MethodReferenceUsage() {
+    this(ErrorProneFlags.empty());
+  }
+
+  /**
+   * Instantiates a customized {@link MethodReferenceUsage}.
+   *
+   * @param flags Any provided command line flags.
+   */
+  public MethodReferenceUsage(ErrorProneFlags flags) {
+    verifySuggestions = flags.getBoolean(VERIFY_SUGGESTIONS_FLAG).orElse(Boolean.FALSE);
+  }
 
   @Override
   public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
+    MethodTree enclosingMethod = state.findEnclosing(MethodTree.class);
+    if (enclosingMethod != null && REFASTER_TEMPLATE_METHOD.matches(enclosingMethod, state)) {
+      /*
+       * Within Refaster template methods variable references may stand in for complex expressions.
+       * Additionally, `@Placeholder` methods cannot be used as method handles. For both these
+       * reasons it is not safe to replace lambda expressions with method references inside Refaster
+       * templates.
+       */
+      // XXX: This is too strict; we should more explicitly handle `@Placeholder` and method
+      // parameters. For example, `() -> Optional.empty()` _should_ be flagged.
+      return Description.NO_MATCH;
+    }
+
+    if (isPassedToSameArgCountMethodOverload(state)) {
+      // XXX: When a lambda expression is passed to an overloaded method, replacing it with a method
+      // reference may introduce an ambiguity about the method that is intended to be invoked. An
+      // example is a pair of overloads accepting a `Runnable` and `Supplier<T>` respectively, where
+      // the lambda expression in question returns a value: in this case the first overload is
+      // selected, but when converted to a method reference the intended target is no longer clear.
+      // Right now any lambda expression passed to an method with an overload accepting the same
+      // number of arguments is ignored. Improve this detection logic.
+      return Description.NO_MATCH;
+    }
+
     /*
      * Lambda expressions can be used in several places where method references cannot, either
      * because the latter are not syntactically valid or ambiguous. Rather than encoding all these
      * edge cases we try to compile the code with the suggested fix, to see whether this works.
      */
-    return constructMethodRef(tree, tree.getBody())
+    // XXX: Update the comment to reflect actual `tryCompileWithFix` usage.
+    return constructMethodRef(tree, state, tree.getBody())
         .map(SuggestedFix.Builder::build)
-        .filter(
-            fix ->
-                SuggestedFixes.compilesWithFix(
-                    fix, state, ImmutableList.of(), /* onlyInSameCompilationUnit= */ true))
+        .filter(fix -> !verifySuggestions || tryCompileWithFix(state, fix))
         .map(fix -> describeMatch(tree, fix))
         .orElse(Description.NO_MATCH);
   }
 
+  private static boolean isPassedToSameArgCountMethodOverload(VisitorState state) {
+    TreePath parent = state.getPath().getParentPath();
+    if (parent == null) {
+      return false;
+    }
+
+    Symbol symbol = ASTHelpers.getSymbol(parent.getLeaf());
+    if (!(symbol instanceof MethodSymbol)) {
+      return false;
+    }
+
+    MethodSymbol method = (MethodSymbol) symbol;
+    return getOverloads(method).anyMatch(m -> m.params().size() == method.params.size());
+  }
+
   private static Optional<SuggestedFix.Builder> constructMethodRef(
-      LambdaExpressionTree lambdaExpr, Tree subTree) {
+      LambdaExpressionTree lambdaExpr, VisitorState state, Tree subTree) {
     switch (subTree.getKind()) {
       case BLOCK:
-        return constructMethodRef(lambdaExpr, (BlockTree) subTree);
+        return constructMethodRef(lambdaExpr, state, (BlockTree) subTree);
       case EXPRESSION_STATEMENT:
-        return constructMethodRef(lambdaExpr, ((ExpressionStatementTree) subTree).getExpression());
+        return constructMethodRef(
+            lambdaExpr, state, ((ExpressionStatementTree) subTree).getExpression());
       case METHOD_INVOCATION:
-        return constructMethodRef(lambdaExpr, (MethodInvocationTree) subTree);
+        return constructMethodRef(lambdaExpr, state, (MethodInvocationTree) subTree);
       case PARENTHESIZED:
-        return constructMethodRef(lambdaExpr, ((ParenthesizedTree) subTree).getExpression());
+        // XXX: Add test!
+        return constructMethodRef(lambdaExpr, state, ((ParenthesizedTree) subTree).getExpression());
       case RETURN:
-        return constructMethodRef(lambdaExpr, ((ReturnTree) subTree).getExpression());
+        // XXX: This case isn't tested. Reachable?
+        // XXX: Should be possible with `{ return x; }`.
+        return constructMethodRef(lambdaExpr, state, ((ReturnTree) subTree).getExpression());
       default:
+        // XXX: Explicitly handle known cases and throw an exception otherwise?
         return Optional.empty();
     }
   }
 
   private static Optional<SuggestedFix.Builder> constructMethodRef(
-      LambdaExpressionTree lambdaExpr, BlockTree subTree) {
+      LambdaExpressionTree lambdaExpr, VisitorState state, BlockTree subTree) {
+    // XXX: Add test with >1 statement.
     return Optional.of(subTree.getStatements())
         .filter(statements -> statements.size() == 1)
-        .flatMap(statements -> constructMethodRef(lambdaExpr, statements.get(0)));
+        .flatMap(statements -> constructMethodRef(lambdaExpr, state, statements.get(0)));
   }
 
   // XXX: Replace nested `Optional` usage.
   @SuppressWarnings("NestedOptionals")
   private static Optional<SuggestedFix.Builder> constructMethodRef(
-      LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) {
-    return matchArguments(lambdaExpr, subTree)
-        .flatMap(expectedInstance -> constructMethodRef(lambdaExpr, subTree, expectedInstance));
+      LambdaExpressionTree lambdaExpr, VisitorState state, MethodInvocationTree subTree) {
+    return matchParameters(lambdaExpr, subTree)
+        .flatMap(
+            expectedInstance -> constructMethodRef(lambdaExpr, state, subTree, expectedInstance));
   }
 
   private static Optional<SuggestedFix.Builder> constructMethodRef(
       LambdaExpressionTree lambdaExpr,
+      VisitorState state,
       MethodInvocationTree subTree,
       Optional<Name> expectedInstance) {
     ExpressionTree methodSelect = subTree.getMethodSelect();
@@ -119,35 +217,54 @@ public final class MethodReferenceUsage extends BugChecker implements LambdaExpr
           /* Direct method call; there is no matching "implicit parameter". */
           return Optional.empty();
         }
+        // XXX: Here too test for ambiguous method references.
         Symbol sym = ASTHelpers.getSymbol(methodSelect);
         if (!sym.isStatic()) {
           return constructFix(lambdaExpr, "this", methodSelect);
         }
         return constructFix(lambdaExpr, sym.owner, methodSelect);
       case MEMBER_SELECT:
-        return constructMethodRef(lambdaExpr, (MemberSelectTree) methodSelect, expectedInstance);
+        return constructMethodRef(
+            lambdaExpr, state, (MemberSelectTree) methodSelect, expectedInstance);
       default:
         throw new VerifyException("Unexpected type of expression: " + methodSelect.getKind());
     }
   }
 
   private static Optional<SuggestedFix.Builder> constructMethodRef(
-      LambdaExpressionTree lambdaExpr, MemberSelectTree subTree, Optional<Name> expectedInstance) {
+      LambdaExpressionTree lambdaExpr,
+      VisitorState state,
+      MemberSelectTree subTree,
+      Optional<Name> expectedInstance) {
     if (subTree.getExpression().getKind() != Kind.IDENTIFIER) {
-      // XXX: Could be parenthesized. Handle. Also in other classes.
+      // XXX: Could be parenthesized. Handle. Also in other classes. Maybe consult
+      // `SideEffectAnalysis`?
+      // XXX: This branch isn't tested. Fix. Maybe something like `foo.bar().baz()`.
       /*
        * Only suggest a replacement if the method select's expression provably doesn't have
        * side-effects. Otherwise the replacement may not be behavior preserving.
        */
+      // XXX: So do this ^.
       return Optional.empty();
     }
 
-    Name lhs = ((IdentifierTree) subTree.getExpression()).getName();
+    // XXX: Check whether this cast is safe in all cases.
+    MethodSymbol method = (MethodSymbol) ASTHelpers.getSymbol(subTree);
+    Symbol lhsSymbol = ASTHelpers.getSymbol(subTree.getExpression());
+    if (method.isStatic() && lhsSymbol instanceof VarSymbol) {
+      return Optional.empty();
+    }
+
+    if (hasAmbiguousMethodReference(method, state)) {
+      return Optional.empty();
+    }
+
+    Name lhs = lhsSymbol.name;
     if (expectedInstance.isEmpty()) {
       return constructFix(lambdaExpr, lhs, subTree.getIdentifier());
     }
 
-    Type lhsType = ASTHelpers.getType(subTree.getExpression());
+    Type lhsType = lhsSymbol.type;
     if (lhsType == null || !expectedInstance.orElseThrow().equals(lhs)) {
       return Optional.empty();
     }
@@ -158,9 +275,66 @@ public final class MethodReferenceUsage extends BugChecker implements LambdaExpr
     return constructFix(lambdaExpr, lhsType.tsym, subTree.getIdentifier());
   }
 
+  /**
+   * Tells whether the given method has an overload that would lead to an ambiguous method
+   * reference.
+   */
+  private static boolean hasAmbiguousMethodReference(MethodSymbol method, VisitorState state) {
+    return getOverloads(method)
+        .anyMatch(m -> haveAmbiguousMethodReferences(m, method, state.getTypes()));
+  }
+
+  /** Returns any overloads of the given method defined on the same class. */
+  // XXX: This probably doesn't return overloads defined by supertypes. Review and extend if
+  // necessary.
+  private static Stream<MethodSymbol> getOverloads(MethodSymbol method) {
+    return Streams.stream(ASTHelpers.enclosingClass(method).members().getSymbolsByName(method.name))
+        .filter(MethodSymbol.class::isInstance)
+        .map(MethodSymbol.class::cast)
+        .filter(not(method::equals));
+  }
+
+  /** Tells whether method references to the given methods would be mutually ambiguous. */
+  // XXX: This doesn't necessarily identify all ambiguous cases; carefully read the JLS and update
+  // this logic if necessary.
+  private static boolean haveAmbiguousMethodReferences(
+      MethodSymbol method1, MethodSymbol method2, Types types) {
+    if (method1.isStatic() == method2.isStatic()) {
+      return false;
+    }
+
+    if (method1.isStatic()) {
+      return haveAmbiguousMethodReferences(method2, method1, types);
+    }
+
+    com.sun.tools.javac.util.List<VarSymbol> params1 = method1.params();
+    com.sun.tools.javac.util.List<VarSymbol> params2 = method2.params();
+    if (params1.size() != params2.size() - 1) {
+      return false;
+    }
+
+    // XXX: Here and below: perhaps `isAssignable` is more appropriate than `isConvertible`.
+    if (!types.isConvertible(method1.owner.asType(), params2.get(0).asType())) {
+      return false;
+    }
+
+    for (int i = 0; i < params1.size(); i++) {
+      if (!types.isConvertible(params1.get(0).asType(), params2.get(i + 1).asType())) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  /**
+   * Attempts to match the given method invocation's arguments against the rightmost parameters of
+   * the provided lambda expression, in order; if successful with zero or one lambda parameter
+   * unaccounted for, then said parameter is returned.
+   */
   // XXX: Refactor or replace inner `Optional` with a custom type.
   @SuppressWarnings("NestedOptionals")
-  private static Optional<Optional<Name>> matchArguments(
+  private static Optional<Optional<Name>> matchParameters(
       LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) {
     ImmutableList<Name> expectedArguments = getVariables(lambdaExpr);
     List<? extends ExpressionTree> args = subTree.getArguments();
@@ -190,7 +364,12 @@ public final class MethodReferenceUsage extends BugChecker implements LambdaExpr
     Name sName = target.getSimpleName();
     Optional<SuggestedFix.Builder> fix = constructFix(lambdaExpr, sName, methodName);
 
-    if (!"java.lang".equals(target.packge().toString())) {
+    if (!"java.lang".equals(target.packge().getQualifiedName().toString())) {
+      // XXX: Check whether the type can be imported. If not, skip the suggestion. (In other words:
+      // if another type with simple name `sName` is already imported, then this suggested fix would
+      // introduce a compilation failure.)
+      // XXX: Make sure `SuggestedFixes.qualifyType` handles `java.lang` and the double-import case,
+      // then use that method.
       Name fqName = target.getQualifiedName();
       if (!sName.equals(fqName)) {
         return fix.map(b -> b.addImport(fqName.toString()));
@@ -200,8 +379,15 @@ public final class MethodReferenceUsage extends BugChecker implements LambdaExpr
     return fix;
   }
 
+  // XXX: As-is this method shouldn't return an `Optional`.
   private static Optional<SuggestedFix.Builder> constructFix(
       LambdaExpressionTree lambdaExpr, Object target, Object methodName) {
     return Optional.of(SuggestedFix.builder().replace(lambdaExpr, target + "::" + methodName));
   }
+
+  private static boolean tryCompileWithFix(VisitorState state, SuggestedFix fix) {
+    return !IS_NESTED_INVOCATION
+        && SuggestedFixes.compilesWithFix(
+            fix, state, ImmutableList.of(), /* onlyInSameCompilationUnit= */ true);
+  }
 }

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsageTest.java

@@ -11,12 +11,16 @@ final class MethodReferenceUsageTest {
   private final BugCheckerRefactoringTestHelper refactoringTestHelper =
       BugCheckerRefactoringTestHelper.newInstance(MethodReferenceUsage.class, getClass());
 
+  // XXX: Disable the `replacement` test and verify using PIT that this test covers all
+  // identification cases.
   @Test
   void identification() {
     compilationTestHelper
         .addSourceLines(
             "A.java",
             "import com.google.common.collect.Streams;",
+            "import com.google.errorprone.refaster.annotation.AfterTemplate;",
+            "import com.google.errorprone.refaster.annotation.BeforeTemplate;",
             "import java.util.HashMap;",
             "import java.util.Map;",
             "import java.util.function.IntConsumer;",
@@ -261,6 +265,18 @@ final class MethodReferenceUsageTest {
             "        });",
             "  }",
             "",
+            "  @AfterTemplate",
+            "  void refasterBeforeTemplateFunctionCalls() {",
+            "    s.forEach(v -> String.valueOf(v));",
+            "    s.forEach((v) -> { String.valueOf(v); });",
+            "  }",
+            "",
+            "  @BeforeTemplate",
+            "  void refasterAfterTemplateFunctionCalls() {",
+            "    m.forEach((k, v) -> m.put(k, v));",
+            "    m.forEach((Integer k, Integer v) -> { m.put(k, v); });",
+            "  }",
+            "",
             "  void assortedOtherEdgeCases() {",
             "    s.forEach(v -> String.valueOf(v.toString()));",
             "    TernaryOp o1 = (a, b, c) -> String.valueOf(a);",

pom.xml

@@ -129,9 +129,10 @@
         default value. -->
         <build.number>LOCAL</build.number>
         <!-- Properties using which additional Error Prone flags can be
-        specified. Used by the `patch` and `self-check` profiles. -->
+        specified. The first may be used to specify flags on the command line;
+        the second is used by the `patch` profile. -->
+        <error-prone.extra-args />
         <error-prone.patch-args />
-        <error-prone.self-check-args />
         <!-- The Maven `groupId` under which Error Prone dependencies are
         published. By default we use an official Error Prone release. This
         property allows the `error-prone-fork` profile below to build the
@@ -1350,16 +1351,14 @@
             <!-- Applies the Error Prone checks defined by this project to the
             code base itself. Assumes that a prior build has already installed
             the project in the local Maven repository. -->
+            <!-- XXX: Find a way to assert that test code (both inline
+            `BugChecker` test code and the Refaster test files) does not
+            exhibit anti-patterns other than those associated with the
+            check/template under test. Ideally all test cases are
+            realistic. -->
             <id>self-check</id>
             <properties>
-                <!-- XXX: `MethodReferenceUsage` is an extremely expensive
-                check due to its use of `SuggestedFixes.compilesWithFix`. Maybe
-                we should drop it altogether? -->
-                <!-- XXX: Find a way to assert that test code (both inline
-                `BugChecker` test code and the Refaster test files) does not
-                exhibit anti-patterns other than those associated with the
-                check/template under test. Ideally all test cases are realistic. -->
-                <error-prone.self-check-args>-XepAllSuggestionsAsWarnings -Xep:MethodReferenceUsage:OFF</error-prone.self-check-args>
+                <error-prone.self-check-args>-XepAllSuggestionsAsWarnings</error-prone.self-check-args>
             </properties>
             <build>
                 <plugins>
@@ -1552,7 +1551,7 @@
                                     -XepOpt:Nullness:Conservative=false
                                     <!-- Append additional custom arguments. -->
                                     ${error-prone.patch-args}
-                                    ${error-prone.self-check-args}
+                                    ${error-prone.extra-args}
                                 </arg>
                                 <!-- The Error Prone plugin makes certain
                                 assumptions about the state of the AST at the