Skip to content

Commit

Permalink
Add matchers for incompatible type matchers
Browse files Browse the repository at this point in the history
We discovered that users run into issues with using the wrong Mockito
matcher for arguments. Examples include `any(Integer.class)` instead of
`anyInt()` and `anyInt()` instead of `anyFloat()`. Users then run into
cryptic run-time errors that are difficult to understand.

These ErrorProne checkers make these a compile warning, to warn the user
before hand. They also provide the appropriate fixes that can be
directly applied.
  • Loading branch information
TimvdLippe committed Nov 27, 2019
1 parent dc6eadc commit 4b3cedf
Show file tree
Hide file tree
Showing 5 changed files with 608 additions and 0 deletions.
@@ -0,0 +1,129 @@
/*
* Copyright (c) 2017 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.errorprone.bugpatterns;

import static com.google.errorprone.matchers.Description.NO_MATCH;

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.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.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Type.ArrayType;
import java.util.List;
import javax.lang.model.type.TypeKind;

/** Base for {@link BugChecker}s that detect issues with any() matchers and primitive types. */
public abstract class AbstractMockitoAnyForPrimitiveType extends BugChecker
implements MethodInvocationTreeMatcher {

protected abstract Matcher<? super MethodInvocationTree> matcher();

protected abstract String formatMessage(
String expectedTypeAsString, Type matcherType, String replacementName);

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

MethodSymbol method = ASTHelpers.getSymbol(tree);
Type matcherType = method.getReturnType();

// It is expected that the call to anyX() is itself the argument to another call which is
// the one being mocked, e.g. something like this:
// when(mock.call(..., anyInt(), ...))...
TreePath path = state.getPath();
Tree parentTree = path.getParentPath().getLeaf();
if (!(parentTree instanceof MethodInvocationTree)) {
// Ignore calls that are not arguments to another method call.
// TODO: Report this as a problem because it makes little sense.
// TODO: Support casting.
return NO_MATCH;
}

MethodInvocationTree parentCall = (MethodInvocationTree) parentTree;
MethodSymbol parentMethod = ASTHelpers.getSymbol(parentCall);

// Find the index of the argument in the parent call.
int argumentIndex = -1;
List<? extends ExpressionTree> parentArguments = parentCall.getArguments();
for (int i = 0; i < parentArguments.size(); i++) {
ExpressionTree argumentTree = parentArguments.get(i);
if (argumentTree == tree) {
argumentIndex = i;
break;
}
}
if (argumentIndex == -1) {
throw new IllegalStateException(
"Cannot find argument " + tree + " in argument list from " + parentTree);
}

Type parameterType = getParameterType(parentMethod, argumentIndex);

TypeKind parameterTypeKind = parameterType.getKind();
if (parameterTypeKind.isPrimitive() && parameterTypeKind != matcherType.getKind()) {
String expectedTypeAsString = parameterType.toString();
String replacementName =
"any"
+ Character.toUpperCase(expectedTypeAsString.charAt(0))
+ expectedTypeAsString.substring(1);

String message = formatMessage(expectedTypeAsString, matcherType, replacementName);

SuggestedFix.Builder fixBuilder = SuggestedFix.builder();

ExpressionTree methodSelect = tree.getMethodSelect();
String replacement;
if (methodSelect instanceof MemberSelectTree) {
MemberSelectTree qualifier = (MemberSelectTree) methodSelect;
replacement = state.getSourceForNode(qualifier.getExpression()) + "." + replacementName;
} else {
replacement = replacementName;
String staticImport = method.owner + "." + replacementName;
fixBuilder.addStaticImport(staticImport);
}

SuggestedFix fix = fixBuilder.replace(tree, replacement + "()").build();

return buildDescription(tree).setMessage(message).addFix(fix).build();
}

return NO_MATCH;
}

/**
* Get the type of the parameter for a supplied argument.
*
* @param method the method symbol that is being called.
* @param argumentIndex the index of the argument, can be greater than the number of parameters
* for a var arg method.
* @return the type of the associated parameter.
*/
private Type getParameterType(MethodSymbol method, int argumentIndex) {
List<VarSymbol> parameters = method.getParameters();
Type parameterType;
int parameterCount = parameters.size();
if (argumentIndex >= parameterCount && method.isVarArgs()) {
VarSymbol varArgParameter = parameters.get(parameterCount - 1);
parameterType = ((ArrayType) varArgParameter.asType()).getComponentType();
} else {
parameterType = parameters.get(argumentIndex).asType();
}
return parameterType;
}
}
@@ -0,0 +1,60 @@
/*
* Copyright (c) 2017 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers.MethodNameMatcher;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.tools.javac.code.Type;

/**
* Matches on usage of {@code Mockito.argThat(Matcher)} with a matcher that does not extend
* ArgumentMatcher.
*/
@AutoService(BugChecker.class)
@BugPattern(
name = "MockitoAnyClassWithPrimitiveType",
summary = "Matcher inconsistency: use of any(Class) to match a primitive argument",
severity = WARNING,
linkType = CUSTOM,
link = "site.mockito.org/usage/bugpattern/MockitoAnyClassWithPrimitiveType",
explanation =
"Mockito relies on Java type checking to ensure that parameter matchers are"
+ " type safe but there are some discrepancies between what the Java type checker"
+ " allows and what Mockito expects. e.g. Java will allow anyInt() to be used as a"
+ " matcher for a long parameter because an int can be widened to a long. This"
+ " checker highlights those incorrect usages and suggests replacements. In Mockito"
+ " 1.x this was not really an issue because the anyX() methods did not do runtime"
+ " type checking of the arguments but in Mockito 2.x they do.")
public class MockitoAnyClassWithPrimitiveType extends AbstractMockitoAnyForPrimitiveType {

private static final String[] CLASS_NAMES = {
"org.mockito.Mockito", "org.mockito.ArgumentMatchers", "org.mockito.Matchers"
};

// Match against the any() or any(Class) methods.
private static final MethodNameMatcher GENERIC_ANY =
Matchers.staticMethod().onClassAny(CLASS_NAMES).named("any");

@Override
protected Matcher<? super MethodInvocationTree> matcher() {
return GENERIC_ANY;
}

@Override
protected String formatMessage(
String expectedTypeAsString, Type matcherType, String replacementName) {
return String.format(
"Matcher mismatch: use %s() matcher to match primitive %s arguments",
replacementName, expectedTypeAsString);
}
}
@@ -0,0 +1,67 @@
/*
* Copyright (c) 2017 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.tools.javac.code.Type;
import java.util.regex.Pattern;

/**
* Matches on usage of {@code Mockito.argThat(Matcher)} with a matcher that does not extend
* ArgumentMatcher.
*/
@AutoService(BugChecker.class)
@BugPattern(
name = "MockitoAnyIncorrectPrimitiveType",
summary = "Matcher mismatch: incorrect use of any() or anyX() to match a primitive argument",
severity = ERROR,
linkType = CUSTOM,
link = "site.mockito.org/usage/bugpattern/MockitoAnyIncorrectPrimitiveType",
explanation =
"Mockito relies on Java type checking to ensure that parameter matchers are"
+ " type safe but there are some discrepancies between what the Java type checker"
+ " allows and what Mockito expects. e.g. Java will allow anyInt() to be used as a"
+ " matcher for a long parameter because an int can be widened to a long. This"
+ " checker highlights those incorrect usages and suggests replacements. In Mockito"
+ " 1.x this was not really an issue because the anyX() methods did not do runtime"
+ " type checking of the arguments but in Mockito 2.x they do."
+ " Java will also allow any() to be used within a primitive but any() returns null and"
+ " the compiler wraps that call in unboxing which leads to a NPE.")
public class MockitoAnyIncorrectPrimitiveType extends AbstractMockitoAnyForPrimitiveType {

// Match against any() or any of the any<x>() methods.
private static final Pattern METHOD_NAME_PATTERN =
Pattern.compile("any(Boolean|Byte|Char|Double|Float|Int|Long|Short)?");

private static final String[] CLASS_NAMES = {
"org.mockito.ArgumentMatchers", "org.mockito.Mockito", "org.mockito.Matchers"
};

private static final Matcher<ExpressionTree> METHOD_MATCHER =
staticMethod().onClassAny(CLASS_NAMES).withNameMatching(METHOD_NAME_PATTERN).withParameters();

@Override
protected Matcher<? super MethodInvocationTree> matcher() {
return METHOD_MATCHER;
}

@Override
protected String formatMessage(
String expectedTypeAsString, Type matcherType, String replacementName) {
return String.format(
"Matcher mismatch: expected matcher for parameter of type '%s',"
+ " found matcher for parameter of type '%s'",
expectedTypeAsString, matcherType);
}
}

0 comments on commit 4b3cedf

Please sign in to comment.