Introduce MockitoMockClassReference check

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

@@ -0,0 +1,90 @@
+package tech.picnic.errorprone.bugpatterns;
+
+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.staticMethod;
+import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+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;
+
+/**
+ * A {@link BugChecker} that flags the use of {@link org.mockito.Mockito#mock(Class)} and {@link
+ * org.mockito.Mockito#spy(Class)} where instead the type to be mocked or spied can be derived from
+ * context.
+ */
+// XXX: This check currently does not flag method invocation arguments. When adding support for
+// this, consider that in some cases the type to be mocked or spied must be specified explicitly so
+// as to disambiguate between method overloads.
+// XXX: This check currently does not flag (implicit or explicit) lambda return expressions.
+// XXX: This check currently does not drop suppressions that become obsolete after the
+// suggested fix is applied; consider adding support for this.
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary = "Don't unnecessarily pass a type to Mockito's `mock(Class)` and `spy(Class)` methods",
+    link = BUG_PATTERNS_BASE_URL + "MockitoMockClassReference",
+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = SIMPLIFICATION)
+public final class MockitoMockClassReference extends BugChecker
+    implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY =
+      allOf(
+          argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))),
+          staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));
+
+  /** Instantiates a new {@link MockitoMockClassReference} instance. */
+  public MockitoMockClassReference() {}
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!MOCKITO_MOCK_OR_SPY.matches(tree, state) || !isTypeDerivableFromContext(tree, state)) {
+      return Description.NO_MATCH;
+    }
+
+    List<? extends ExpressionTree> arguments = tree.getArguments();
+    return describeMatch(tree, SuggestedFixes.removeElement(arguments.get(0), arguments, state));
+  }
+
+  private static boolean isTypeDerivableFromContext(MethodInvocationTree tree, VisitorState state) {
+    Tree parent = state.getPath().getParentPath().getLeaf();
+    switch (parent.getKind()) {
+      case VARIABLE:
+        return !ASTHelpers.hasNoExplicitType((VariableTree) parent, state)
+            && areSameType(tree, parent, state);
+      case ASSIGNMENT:
+        return areSameType(tree, parent, state);
+      case RETURN:
+        Tree context = state.findEnclosing(LambdaExpressionTree.class, MethodTree.class);
+        return context instanceof MethodTree
+            && areSameType(tree, ((MethodTree) context).getReturnType(), state);
+      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/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

@@ -0,0 +1,136 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class MockitoMockClassReferenceTest {
+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(MockitoMockClassReference.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "import static org.mockito.Mockito.mock;",
+            "import static org.mockito.Mockito.spy;",
+            "import static org.mockito.Mockito.withSettings;",
+            "",
+            "import java.util.List;",
+            "import java.util.Objects;",
+            "import org.mockito.invocation.InvocationOnMock;",
+            "",
+            "class A {",
+            "  {",
+            "    Double d = Objects.requireNonNullElseGet(null, () -> mock(Double.class));",
+            "    Double d2 =",
+            "        Objects.requireNonNullElseGet(",
+            "            null,",
+            "            () -> {",
+            "              return mock(Double.class);",
+            "            });",
+            "  }",
+            "",
+            "  void m() {",
+            "    Number variableMock = 42;",
+            "    // BUG: Diagnostic contains:",
+            "    variableMock = mock(Number.class);",
+            "    // BUG: Diagnostic contains:",
+            "    variableMock = mock(Number.class, \"name\");",
+            "    // BUG: Diagnostic contains:",
+            "    variableMock = mock(Number.class, InvocationOnMock::callRealMethod);",
+            "    // BUG: Diagnostic contains:",
+            "    variableMock = mock(Number.class, withSettings());",
+            "    variableMock = mock(Integer.class);",
+            "    variableMock = 42;",
+            "    // BUG: Diagnostic contains:",
+            "    List rawMock = mock(List.class);",
+            "    // BUG: Diagnostic contains:",
+            "    List<String> genericMock = mock(List.class);",
+            "    var varMock = mock(Integer.class);",
+            "    Class<? extends Number> numberType = Integer.class;",
+            "    Number variableTypeMock = mock(numberType);",
+            "    Object subtypeMock = mock(Integer.class);",
+            "",
+            "    Number variableSpy = 42;",
+            "    // BUG: Diagnostic contains:",
+            "    variableSpy = spy(Number.class);",
+            "    variableSpy = spy(Integer.class);",
+            "    variableSpy = 42;",
+            "    // BUG: Diagnostic contains:",
+            "    List rawSpy = spy(List.class);",
+            "    // BUG: Diagnostic contains:",
+            "    List<String> genericSpy = spy(List.class);",
+            "    var varSpy = spy(Integer.class);",
+            "    Number variableTypeSpy = spy(numberType);",
+            "    Object subtypeSpy = spy(Integer.class);",
+            "    Object objectSpy = spy(new Object());",
+            "",
+            "    Objects.hash(mock(Integer.class));",
+            "    Integer i = mock(mock(Integer.class));",
+            "    String s = new String(mock(String.class));",
+            "  }",
+            "",
+            "  Double getDoubleMock() {",
+            "    return Objects.requireNonNullElseGet(",
+            "        null,",
+            "        () -> {",
+            "          return mock(Double.class);",
+            "        });",
+            "  }",
+            "",
+            "  Integer getIntegerMock() {",
+            "    // BUG: Diagnostic contains:",
+            "    return mock(Integer.class);",
+            "  }",
+            "",
+            "  <T> T getGenericMock(Class<T> clazz) {",
+            "    return mock(clazz);",
+            "  }",
+            "",
+            "  Number getSubTypeMock() {",
+            "    return mock(Integer.class);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  void replacement() {
+    BugCheckerRefactoringTestHelper.newInstance(MockitoMockClassReference.class, getClass())
+        .addInputLines(
+            "A.java",
+            "import static org.mockito.Mockito.mock;",
+            "import static org.mockito.Mockito.spy;",
+            "import static org.mockito.Mockito.withSettings;",
+            "",
+            "import org.mockito.invocation.InvocationOnMock;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Number simpleMock = mock(Number.class);",
+            "    Number namedMock = mock(Number.class, \"name\");",
+            "    Number customAnswerMock = mock(Number.class, InvocationOnMock::callRealMethod);",
+            "    Number customSettingsMock = mock(Number.class, withSettings());",
+            "    Number simpleSpy = spy(Number.class);",
+            "  }",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "import static org.mockito.Mockito.mock;",
+            "import static org.mockito.Mockito.spy;",
+            "import static org.mockito.Mockito.withSettings;",
+            "",
+            "import org.mockito.invocation.InvocationOnMock;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Number simpleMock = mock();",
+            "    Number namedMock = mock(\"name\");",
+            "    Number customAnswerMock = mock(InvocationOnMock::callRealMethod);",
+            "    Number customSettingsMock = mock(withSettings());",
+            "    Number simpleSpy = spy();",
+            "  }",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
+}

refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java

@@ -40,9 +40,9 @@
 // through `RefasterTest`, but ideally it is covered by tests in this class, closer to the code that
 // implements the relevant logic.) See the comment in `#context()` below.
 final class AnnotatedCompositeCodeTransformerTest {
-  private static final DiagnosticPosition DUMMY_POSITION = mock(DiagnosticPosition.class);
-  private static final Fix DUMMY_FIX = mock(Fix.class);
-  private static final TreePath DUMMY_PATH = mock(TreePath.class);
+  private static final DiagnosticPosition DUMMY_POSITION = mock();
+  private static final Fix DUMMY_FIX = mock();
+  private static final TreePath DUMMY_PATH = mock();
   private static final String DEFAULT_PACKAGE = "";
   private static final String CUSTOM_PACKAGE = "com.example";
   private static final String SIMPLE_CLASS_NAME = "MyRefasterRule";
@@ -149,7 +149,7 @@ private static CodeTransformer delegateCodeTransformer(
       ImmutableSet<? extends Annotation> annotations,
       Context expectedContext,
       Description returnedDescription) {
-    CodeTransformer codeTransformer = mock(CodeTransformer.class);
+    CodeTransformer codeTransformer = mock();
 
     when(codeTransformer.annotations()).thenReturn(indexAnnotations(annotations));
     doAnswer(
@@ -182,7 +182,7 @@ private static Description description(
   private static Context context() {
     // XXX: Use `ErrorProneOptions#processArgs` to test the
     // `AnnotatedCompositeCodeTransformer#overrideSeverity` logic.
-    Context context = mock(Context.class);
+    Context context = mock();
     when(context.get(ErrorProneOptions.class)).thenReturn(ErrorProneOptions.empty());
     return context;
   }