Handle mock statements

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

@@ -3,8 +3,14 @@
 import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
+import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.argument;
+import static com.google.errorprone.matchers.Matchers.isSameType;
 import static com.google.errorprone.matchers.Matchers.isVariable;
+import static com.google.errorprone.matchers.Matchers.not;
 import static com.google.errorprone.matchers.Matchers.returnStatement;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.matchers.Matchers.toType;
 import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
 
 import com.google.auto.service.AutoService;
@@ -20,13 +26,15 @@
 import com.sun.source.tree.BlockTree;
 import com.sun.source.tree.ExpressionStatementTree;
 import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.ReturnTree;
 import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import com.sun.tools.javac.code.Symbol;
 import java.util.List;
 import java.util.Optional;
+import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers;
 import tech.picnic.errorprone.bugpatterns.util.SourceCode;
 
 /**
@@ -43,6 +51,10 @@
 public final class DirectReturn extends BugChecker implements BlockTreeMatcher {
   private static final long serialVersionUID = 1L;
   private static final Matcher<StatementTree> VARIABLE_RETURN = returnStatement(isVariable());
+  private static final Matcher<ExpressionTree> MOCKITO_MOCK_OR_SPY_WITH_IMPLICIT_TYPE =
+      allOf(
+          not(toType(MethodInvocationTree.class, argument(0, isSameType(Class.class.getName())))),
+          staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));
 
   /** Instantiates a new {@link DirectReturn} instance. */
   public DirectReturn() {}
@@ -63,6 +75,7 @@ public Description matchBlock(BlockTree tree, VisitorState state) {
     StatementTree precedingStatement = statements.get(statements.size() - 2);
 
     return tryMatchAssignment(variableSymbol, precedingStatement)
+        .filter(resultExpr -> canInlineToReturnStatement(resultExpr, state))
         .map(
             resultExpr ->
                 describeMatch(
@@ -98,4 +111,21 @@ private static Optional<ExpressionTree> tryMatchAssignment(Symbol targetSymbol,
 
     return Optional.empty();
   }
+
+  /**
+   * Tells whether inlining the given expression to the associated return statement can be done
+   * safely.
+   *
+   * <p>Inlining is generally safe, but in rare cases the operation may have a functional impact.
+   * The sole case considered here is the inlining of a Mockito mock or spy construction without an
+   * explicit type. In such a case the type created depends on context, such as the method's return
+   * type.
+   */
+  private static boolean canInlineToReturnStatement(
+      ExpressionTree expressionTree, VisitorState state) {
+    return !MOCKITO_MOCK_OR_SPY_WITH_IMPLICIT_TYPE.matches(expressionTree, state)
+        || MoreASTHelpers.findMethodExitedOnReturn(state)
+            .filter(m -> MoreASTHelpers.areSameType(expressionTree, m.getReturnType(), state))
+            .isPresent();
+  }
 }

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

@@ -21,12 +21,11 @@
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
-import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
-import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import java.util.List;
+import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers;
 
 /**
  * A {@link BugChecker} that flags the use of {@link org.mockito.Mockito#mock(Class)} and {@link
@@ -49,7 +48,7 @@
 public final class MockitoMockClassReference extends BugChecker
     implements MethodInvocationTreeMatcher {
   private static final long serialVersionUID = 1L;
-  private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY =
+  private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY_WITH_HARDCODED_TYPE =
       allOf(
           argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))),
           staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));
@@ -59,7 +58,8 @@ public MockitoMockClassReference() {}
 
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
-    if (!MOCKITO_MOCK_OR_SPY.matches(tree, state) || !isTypeDerivableFromContext(tree, state)) {
+    if (!MOCKITO_MOCK_OR_SPY_WITH_HARDCODED_TYPE.matches(tree, state)
+        || !isTypeDerivableFromContext(tree, state)) {
       return Description.NO_MATCH;
     }
 
@@ -72,19 +72,15 @@ private static boolean isTypeDerivableFromContext(MethodInvocationTree tree, Vis
     switch (parent.getKind()) {
       case VARIABLE:
         return !ASTHelpers.hasNoExplicitType((VariableTree) parent, state)
-            && areSameType(tree, parent, state);
+            && MoreASTHelpers.areSameType(tree, parent, state);
       case ASSIGNMENT:
-        return areSameType(tree, parent, state);
+        return MoreASTHelpers.areSameType(tree, parent, state);
       case RETURN:
-        Tree context = state.findEnclosing(LambdaExpressionTree.class, MethodTree.class);
-        return context instanceof MethodTree
-            && areSameType(tree, ((MethodTree) context).getReturnType(), state);
+        return MoreASTHelpers.findMethodExitedOnReturn(state)
+            .filter(m -> MoreASTHelpers.areSameType(tree, m.getReturnType(), state))
+            .isPresent();
       default:
         return false;
     }
   }
-
-  private static boolean areSameType(Tree treeA, Tree treeB, VisitorState state) {
-    return ASTHelpers.isSameType(ASTHelpers.getType(treeA), ASTHelpers.getType(treeB), state);
-  }
 }

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

@@ -5,8 +5,12 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.Tree;
+import java.util.Optional;
 
 /**
  * A collection of helper methods for working with the AST.
@@ -46,4 +50,33 @@ public static ImmutableList<MethodTree> findMethods(CharSequence methodName, Vis
   public static boolean methodExistsInEnclosingClass(CharSequence methodName, VisitorState state) {
     return !findMethods(methodName, state).isEmpty();
   }
+
+  /**
+   * Returns the {@link MethodTree} from which control flow would exit if there would be a {@code
+   * return} statement at the given {@link VisitorState}'s current {@link VisitorState#getPath()
+   * path}.
+   *
+   * @param state The {@link VisitorState} from which to derive the AST location of interest.
+   * @return A {@link MethodTree}, unless the {@link VisitorState}'s path does not point to an AST
+   *     node located inside a method, or if the (hypothetical) {@code return} statement would exit
+   *     a lambda expression instead.
+   */
+  public static Optional<MethodTree> findMethodExitedOnReturn(VisitorState state) {
+    return Optional.ofNullable(state.findEnclosing(LambdaExpressionTree.class, MethodTree.class))
+        .filter(MethodTree.class::isInstance)
+        .map(MethodTree.class::cast);
+  }
+
+  /**
+   * Tells whether the given trees are of the same type, after type erasure.
+   *
+   * @param treeA The first tree of interest.
+   * @param treeB The second tree of interest.
+   * @param state The {@link VisitorState} describing the context in which the given trees were
+   *     found.
+   * @return Whether the specified trees have the same erased types.
+   */
+  public static boolean areSameType(Tree treeA, Tree treeB, VisitorState state) {
+    return ASTHelpers.isSameType(ASTHelpers.getType(treeA), ASTHelpers.getType(treeB), state);
+  }
 }

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

@@ -11,6 +11,9 @@ void identification() {
     CompilationTestHelper.newInstance(DirectReturn.class, getClass())
         .addSourceLines(
             "A.java",
+            "import static org.mockito.Mockito.mock;",
+            "import static org.mockito.Mockito.spy;",
+            "",
             "class A {",
             "  private String field;",
             "",
@@ -30,12 +33,29 @@ void identification() {
             "    return field;",
             "  }",
             "",
-            "  String redundantAssignmentToParam(String param) {",
+            "  Object redundantAssignmentToParam(String param) {",
             "    // BUG: Diagnostic contains:",
             "    param = toString();",
             "    return param;",
             "  }",
             "",
+            "  String redundantMockAssignmentToParam(String param) {",
+            "    // BUG: Diagnostic contains:",
+            "    param = mock();",
+            "    return param;",
+            "  }",
+            "",
+            "  Object redundantMockWithExplicitTypeAssignmentToParam(String param) {",
+            "    // BUG: Diagnostic contains:",
+            "    param = mock(String.class);",
+            "    return param;",
+            "  }",
+            "",
+            "  Object salientMockAssignmentToParam(String param) {",
+            "    param = mock();",
+            "    return param;",
+            "  }",
+            "",
             "  String redundantAssignmentToLocalVariable() {",
             "    String variable = null;",
             "    // BUG: Diagnostic contains:",
@@ -55,6 +75,23 @@ void identification() {
             "    return variable;",
             "  }",
             "",
+            "  String redundantSpyVariableDeclaration() {",
+            "    // BUG: Diagnostic contains:",
+            "    String variable = spy();",
+            "    return variable;",
+            "  }",
+            "",
+            "  Object redundantSpyWithExplicitTypeVariableDeclaration() {",
+            "    // BUG: Diagnostic contains:",
+            "    String variable = spy(String.class);",
+            "    return variable;",
+            "  }",
+            "",
+            "  Object salienSpyTypeVariableDeclaration() {",
+            "    String variable = spy(\"name\");",
+            "    return variable;",
+            "  }",
+            "",
             "  String unusedVariableDeclaration(String param) {",
             "    String variable = toString();",
             "    return param;",

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

@@ -8,9 +8,16 @@
 import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.ExpressionStatementTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher;
 import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.ExpressionStatementTree;
 import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+import java.util.List;
 import java.util.function.BiFunction;
 import org.junit.jupiter.api.Test;
 
@@ -67,7 +74,56 @@ void methodExistsInEnclosingClass() {
         .doTest();
   }
 
-  private static String createDiagnosticsMessage(
+  @Test
+  void findMethodExitedOnReturn() {
+    CompilationTestHelper.newInstance(FindMethodReturnTestChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "import java.util.stream.Stream;",
+            "",
+            "class A {",
+            "  {",
+            "    toString();",
+            "  }",
+            "",
+            "  String m() {",
+            "    // BUG: Diagnostic contains:",
+            "    toString();",
+            "    // BUG: Diagnostic contains:",
+            "    return toString();",
+            "  }",
+            "",
+            "  Stream<String> m2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return Stream.of(1)",
+            "        .map(",
+            "            n -> {",
+            "              toString();",
+            "              return toString();",
+            "            });",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  void areSameType() {
+    CompilationTestHelper.newInstance(AreSameTypeTestChecker.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  void negative1(String a, Integer b) {}",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  void positive1(String a, String b) {}",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  void positive2(Iterable<String> a, Iterable<Integer> b) {}",
+            "}")
+        .doTest();
+  }
+
+  private static String createMethodSearchDiagnosticsMessage(
       BiFunction<String, VisitorState, Object> valueFunction, VisitorState state) {
     return Maps.toMap(ImmutableSet.of("foo", "bar", "baz"), key -> valueFunction.apply(key, state))
         .toString();
@@ -85,7 +141,7 @@ public static final class FindMethodsTestChecker extends BugChecker implements M
     public Description matchMethod(MethodTree tree, VisitorState state) {
       return buildDescription(tree)
           .setMessage(
-              createDiagnosticsMessage(
+              createMethodSearchDiagnosticsMessage(
                   (methodName, s) -> MoreASTHelpers.findMethods(methodName, s).size(), state))
           .build();
     }
@@ -103,8 +159,55 @@ public static final class MethodExistsTestChecker extends BugChecker
     @Override
     public Description matchMethod(MethodTree tree, VisitorState state) {
       return buildDescription(tree)
-          .setMessage(createDiagnosticsMessage(MoreASTHelpers::methodExistsInEnclosingClass, state))
+          .setMessage(
+              createMethodSearchDiagnosticsMessage(
+                  MoreASTHelpers::methodExistsInEnclosingClass, state))
           .build();
     }
   }
+
+  /**
+   * A {@link BugChecker} that delegates to {@link
+   * MoreASTHelpers#findMethodExitedOnReturn(VisitorState)}.
+   */
+  @BugPattern(summary = "Interacts with `MoreASTHelpers` for testing purposes", severity = ERROR)
+  public static final class FindMethodReturnTestChecker extends BugChecker
+      implements ExpressionStatementTreeMatcher, ReturnTreeMatcher {
+    private static final long serialVersionUID = 1L;
+
+    @Override
+    public Description matchExpressionStatement(ExpressionStatementTree tree, VisitorState state) {
+      return flagMethodReturnLocation(tree, state);
+    }
+
+    @Override
+    public Description matchReturn(ReturnTree tree, VisitorState state) {
+      return flagMethodReturnLocation(tree, state);
+    }
+
+    private Description flagMethodReturnLocation(Tree tree, VisitorState state) {
+      return MoreASTHelpers.findMethodExitedOnReturn(state)
+          .map(m -> buildDescription(tree).setMessage(state.getSourceForNode(m)).build())
+          .orElse(Description.NO_MATCH);
+    }
+  }
+
+  /**
+   * A {@link BugChecker} that delegates to {@link MoreASTHelpers#areSameType(Tree, Tree,
+   * VisitorState)}.
+   */
+  @BugPattern(summary = "Interacts with `MoreASTHelpers` for testing purposes", severity = ERROR)
+  public static final class AreSameTypeTestChecker extends BugChecker implements MethodTreeMatcher {
+    private static final long serialVersionUID = 1L;
+
+    @Override
+    public Description matchMethod(MethodTree tree, VisitorState state) {
+      List<? extends VariableTree> parameters = tree.getParameters();
+      return parameters.stream()
+              .skip(1)
+              .allMatch(p -> MoreASTHelpers.areSameType(p, parameters.get(0), state))
+          ? describeMatch(tree)
+          : Description.NO_MATCH;
+    }
+  }
 }