Skip to content

Commit

Permalink
Suggestions (2)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202
Stephan202 committed Feb 19, 2023
1 parent f2837e5 commit 3c77c53
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 88 deletions.
38 changes: 17 additions & 21 deletions ...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java
View file
Expand Up @@ -5,40 +5,41 @@
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.argumentCount;
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.common.collect.Iterables;
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.SuggestedFix;
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.AssignmentTree;
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.ReturnTree;
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 could be derived from the variable or
* field with an explicit type.
* 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.
@AutoService(BugChecker.class)
@BugPattern(
summary =
"Don't unnecessarily pass the type reference to Mockito's `mock(Class)` and spy(Class)`",
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,
Expand All @@ -48,7 +49,6 @@ public final class MockitoMockClassReference extends BugChecker
private static final long serialVersionUID = 1L;
private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY =
allOf(
argumentCount(1),
argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))),
staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));

Expand All @@ -57,30 +57,26 @@ public MockitoMockClassReference() {}

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!MOCKITO_MOCK_OR_SPY.matches(tree, state)) {
if (!MOCKITO_MOCK_OR_SPY.matches(tree, state) || !isTypeDerivableFromContext(tree, state)) {
return Description.NO_MATCH;
}

return isReturnTypeDerivableFromContext(state)
? describeMatch(tree, SuggestedFix.delete(Iterables.getOnlyElement(tree.getArguments())))
: Description.NO_MATCH;
List<? extends ExpressionTree> arguments = tree.getArguments();
return describeMatch(tree, SuggestedFixes.removeElement(arguments.get(0), arguments, state));
}

private static boolean isReturnTypeDerivableFromContext(VisitorState 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(parent, ((VariableTree) parent).getInitializer(), state);
&& areSameType(tree, parent, state);
case ASSIGNMENT:
return areSameType(parent, ((AssignmentTree) parent).getExpression(), state);
return areSameType(tree, parent, state);
case RETURN:
Tree context = state.findEnclosing(LambdaExpressionTree.class, MethodTree.class);
return context instanceof MethodTree
&& areSameType(
((MethodTree) context).getReturnType(),
((ReturnTree) parent).getExpression(),
state);
&& areSameType(tree, ((MethodTree) context).getReturnType(), state);
default:
return false;
}
Expand Down
105 changes: 38 additions & 67 deletions ...ntrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java
View file
Expand Up @@ -13,17 +13,13 @@ void identification() {
"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 {",
" // BUG: Diagnostic contains:",
" String memberMock = mock(String.class);",
" // BUG: Diagnostic contains:",
" String memberSpy = mock(String.class);",
"",
" {",
" Double d = Objects.requireNonNullElseGet(null, () -> mock(Double.class));",
" Double d2 =",
Expand All @@ -35,40 +31,43 @@ void identification() {
" }",
"",
" void m() {",
" Integer variableMock;",
" 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 nonGenericallyTypedMock = mock(List.class);",
" List rawMock = mock(List.class);",
" // BUG: Diagnostic contains:",
" List<String> genericallyTypedMock = mock(List.class);",
" var nonExplicitlyTypedMock = mock(Integer.class);",
" Class<? extends Number> variableType = Integer.class;",
" Number variableTypedMock = mock(variableType);",
" Integer namedMock = mock(Integer.class, \"name\");",
" 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:",
" String equalTypedSpy = spy(String.class);",
" variableSpy = spy(Number.class);",
" variableSpy = spy(Integer.class);",
" variableSpy = 42;",
" // BUG: Diagnostic contains:",
" List nonGenericallyTypedSpy = spy(List.class);",
" List rawSpy = spy(List.class);",
" // BUG: Diagnostic contains:",
" List<String> genericallyTypedSpy = spy(List.class);",
" var nonExplicitlyTypedSpy = spy(Integer.class);",
" Number subtypeSpy = spy(Integer.class);",
" 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));",
" Integer i2 = mock(Integer.class, InvocationOnMock::callRealMethod);",
" String s = new String(mock(String.class));",
" Double d = Objects.requireNonNullElseGet(null, () -> mock(Double.class));",
" Double d2 =",
" Objects.requireNonNullElseGet(",
" null,",
" () -> {",
" return mock(Double.class);",
" });",
" }",
"",
" Double getDoubleMock() {",
Expand Down Expand Up @@ -102,62 +101,34 @@ void replacement() {
"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 org.mockito.invocation.InvocationOnMock;",
"",
"class A {",
" String memberMock = mock(String.class);",
"",
" void m() {",
" Runnable runnableMock;",
" runnableMock = mock(Runnable.class);",
" List<String> listOfStringsMock = mock(List.class);",
" List genericListMock = mock(List.class);",
"",
" var unknownTypeMock = mock(Integer.class);",
" Integer namedMock = mock(Integer.class, \"name\");",
"",
" Runnable runnableSpy = spy(Runnable.class);",
" List<String> listOfStringsSpy = spy(List.class);",
" List genericListSpy = spy(List.class);",
"",
" var unknownTypeSpy = spy(Integer.class);",
" Object objectSpy = spy(new Object());",
" }",
"",
" Integer getIntegerMock() {",
" return mock(Integer.class);",
" 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 java.util.List;",
"import org.mockito.invocation.InvocationOnMock;",
"",
"class A {",
" String memberMock = mock();",
"",
" void m() {",
" Runnable runnableMock;",
" runnableMock = mock();",
" List<String> listOfStringsMock = mock();",
" List genericListMock = mock();",
"",
" var unknownTypeMock = mock(Integer.class);",
" Integer namedMock = mock(Integer.class, \"name\");",
"",
" Runnable runnableSpy = spy();",
" List<String> listOfStringsSpy = spy();",
" List genericListSpy = spy();",
"",
" var unknownTypeSpy = spy(Integer.class);",
" Object objectSpy = spy(new Object());",
" }",
"",
" Integer getIntegerMock() {",
" return mock();",
" Number simpleMock = mock();",
" Number namedMock = mock(\"name\");",
" Number customAnswerMock = mock(InvocationOnMock::callRealMethod);",
" Number customSettingsMock = mock(withSettings());",
" Number simpleSpy = spy();",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
Expand Down

0 comments on commit 3c77c53

Please sign in to comment.