diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java new file mode 100644 index 00000000..4aa3af46 --- /dev/null +++ b/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 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 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); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java new file mode 100644 index 00000000..8fc34e35 --- /dev/null +++ b/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 genericMock = mock(List.class);", + " var varMock = mock(Integer.class);", + " Class 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 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 getGenericMock(Class 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); + } +} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java index 51d46815..47f30b78 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/AnnotatedCompositeCodeTransformerTest.java @@ -40,9 +40,9 @@ import tech.picnic.errorprone.refaster.annotation.Severity; // 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 @@ final class AnnotatedCompositeCodeTransformerTest { ImmutableSet annotations, Context expectedContext, Description returnedDescription) { - CodeTransformer codeTransformer = mock(CodeTransformer.class); + CodeTransformer codeTransformer = mock(); when(codeTransformer.annotations()).thenReturn(indexAnnotations(annotations)); doAnswer( @@ -182,7 +182,7 @@ final class AnnotatedCompositeCodeTransformerTest { 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; }