Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce MockitoMockClassReference check #454

Merged
merged 12 commits into from
Mar 6, 2023
Merged

Introduce MockitoMockClassReference check #454

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d742d6c
Introduce MockitoMockClassReference bug check
Badbond Dec 20, 2022
87956a2
Add check for compatible variable tree types and extend test cases
Badbond Jan 7, 2023
20be36d
Add test for unimplemented edge-cases
Badbond Jan 7, 2023
54973a6
Self-apply and add XXX
Badbond Jan 7, 2023
70a4cb1
Add another XXX
Badbond Jan 7, 2023
cabec75
Suggestions
rickie Jan 9, 2023
fedd8ef
Implement return type matches and do not flag variable class arguments
Badbond Jan 15, 2023
e1b5f72
Suggestion to restructure the check
rickie Jan 27, 2023
2c1d096
Suggestions
Stephan202 Feb 18, 2023
fd70908
Suggestions (2)
Stephan202 Feb 19, 2023
180ddf6
Add XXX
rickie Mar 6, 2023
f0c7563
Suggestion
rickie Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 17 additions & 21 deletions ...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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()))),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be caveats when the passed argument is a variable that can lead into non-compilable/working-with-Mockito code, e.g. the following is odd and won't work:

-<T> T getMock(class<T> clazz) {
-  return mock(clazz);
+<T> T getMock() {
+  return mock();
}


@Test
void mockType() {
-  assertThat(getMock(BigInteger.class)).isInstanceOf(BigInteger.class);
+  assertThat((BigInteger) getMock()).isInstanceOf(BigInteger.class);
}

There might be cases where we could still replace the variable, but I couldn't find an elegant way for it without going into the caveats. For example the trivial:

-Class<BigInteger> clazz = BigInteger.class;
-BigInteger mock = mock(clazz);
+BigInteger mock = mock();
}

Feel free to play around and see if you find an elegant way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's fair to leave this out of scope.

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
Open in desktop
Original file line number Diff line number Diff line change
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;",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: always check for more edge cases that we can test 😛.

"",
"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