From 120e418000786505c76bdb1c7d19cafb0a34593f Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 11 Mar 2023 17:16:04 +0100 Subject: [PATCH] Handle mock statements --- .../errorprone/bugpatterns/DirectReturn.java | 30 +++++ .../MockitoMockClassReference.java | 22 ++-- .../bugpatterns/util/MoreASTHelpers.java | 33 ++++++ .../bugpatterns/DirectReturnTest.java | 39 ++++++- .../bugpatterns/util/MoreASTHelpersTest.java | 109 +++++++++++++++++- 5 files changed, 216 insertions(+), 17 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java index 268c1550e0c..ee0091eeebc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -3,8 +3,14 @@ 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.returnStatement; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.toType; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -20,6 +26,7 @@ import com.sun.source.tree.BlockTree; import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.Tree; @@ -27,6 +34,7 @@ import com.sun.tools.javac.code.Symbol; import java.util.List; import java.util.Optional; +import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** @@ -43,6 +51,10 @@ public final class DirectReturn extends BugChecker implements BlockTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher VARIABLE_RETURN = returnStatement(isVariable()); + private static final Matcher MOCKITO_MOCK_OR_SPY_WITH_IMPLICIT_TYPE = + allOf( + not(toType(MethodInvocationTree.class, argument(0, isSameType(Class.class.getName())))), + staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy")); /** Instantiates a new {@link DirectReturn} instance. */ public DirectReturn() {} @@ -63,6 +75,7 @@ public Description matchBlock(BlockTree tree, VisitorState state) { StatementTree precedingStatement = statements.get(statements.size() - 2); return tryMatchAssignment(variableSymbol, precedingStatement) + .filter(resultExpr -> canInlineToReturnStatement(resultExpr, state)) .map( resultExpr -> describeMatch( @@ -98,4 +111,21 @@ private static Optional tryMatchAssignment(Symbol targetSymbol, return Optional.empty(); } + + /** + * Tells whether inlining the given expression to the associated return statement can be done + * safely. + * + *

Inlining is generally safe, but in rare cases the operation may have a functional impact. + * The sole case considered here is the inlining of a Mockito mock or spy construction without an + * explicit type. In such a case the type created depends on context, such as the method's return + * type. + */ + private static boolean canInlineToReturnStatement( + ExpressionTree expressionTree, VisitorState state) { + return !MOCKITO_MOCK_OR_SPY_WITH_IMPLICIT_TYPE.matches(expressionTree, state) + || MoreASTHelpers.findMethodExitedOnReturn(state) + .filter(m -> MoreASTHelpers.areSameType(expressionTree, m.getReturnType(), state)) + .isPresent(); + } } 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 index 4aa3af46e67..af2e51d8a00 100644 --- 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 @@ -21,12 +21,11 @@ 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; +import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers; /** * A {@link BugChecker} that flags the use of {@link org.mockito.Mockito#mock(Class)} and {@link @@ -49,7 +48,7 @@ public final class MockitoMockClassReference extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher MOCKITO_MOCK_OR_SPY = + private static final Matcher MOCKITO_MOCK_OR_SPY_WITH_HARDCODED_TYPE = allOf( argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))), staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy")); @@ -59,7 +58,8 @@ public MockitoMockClassReference() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!MOCKITO_MOCK_OR_SPY.matches(tree, state) || !isTypeDerivableFromContext(tree, state)) { + if (!MOCKITO_MOCK_OR_SPY_WITH_HARDCODED_TYPE.matches(tree, state) + || !isTypeDerivableFromContext(tree, state)) { return Description.NO_MATCH; } @@ -72,19 +72,15 @@ private static boolean isTypeDerivableFromContext(MethodInvocationTree tree, Vis switch (parent.getKind()) { case VARIABLE: return !ASTHelpers.hasNoExplicitType((VariableTree) parent, state) - && areSameType(tree, parent, state); + && MoreASTHelpers.areSameType(tree, parent, state); case ASSIGNMENT: - return areSameType(tree, parent, state); + return MoreASTHelpers.areSameType(tree, parent, state); case RETURN: - Tree context = state.findEnclosing(LambdaExpressionTree.class, MethodTree.class); - return context instanceof MethodTree - && areSameType(tree, ((MethodTree) context).getReturnType(), state); + return MoreASTHelpers.findMethodExitedOnReturn(state) + .filter(m -> MoreASTHelpers.areSameType(tree, m.getReturnType(), state)) + .isPresent(); 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/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java index ccfd3353bbb..b0d4bcaddc0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java @@ -5,8 +5,12 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; +import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import java.util.Optional; /** * A collection of helper methods for working with the AST. @@ -46,4 +50,33 @@ public static ImmutableList findMethods(CharSequence methodName, Vis public static boolean methodExistsInEnclosingClass(CharSequence methodName, VisitorState state) { return !findMethods(methodName, state).isEmpty(); } + + /** + * Returns the {@link MethodTree} from which control flow would exit if there would be a {@code + * return} statement at the given {@link VisitorState}'s current {@link VisitorState#getPath() + * path}. + * + * @param state The {@link VisitorState} from which to derive the AST location of interest. + * @return A {@link MethodTree}, unless the {@link VisitorState}'s path does not point to an AST + * node located inside a method, or if the (hypothetical) {@code return} statement would exit + * a lambda expression instead. + */ + public static Optional findMethodExitedOnReturn(VisitorState state) { + return Optional.ofNullable(state.findEnclosing(LambdaExpressionTree.class, MethodTree.class)) + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast); + } + + /** + * Tells whether the given trees are of the same type, after type erasure. + * + * @param treeA The first tree of interest. + * @param treeB The second tree of interest. + * @param state The {@link VisitorState} describing the context in which the given trees were + * found. + * @return Whether the specified trees have the same erased types. + */ + public 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/DirectReturnTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java index ccfda3179b0..05cf0d3fa97 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java @@ -11,6 +11,9 @@ void identification() { CompilationTestHelper.newInstance(DirectReturn.class, getClass()) .addSourceLines( "A.java", + "import static org.mockito.Mockito.mock;", + "import static org.mockito.Mockito.spy;", + "", "class A {", " private String field;", "", @@ -30,12 +33,29 @@ void identification() { " return field;", " }", "", - " String redundantAssignmentToParam(String param) {", + " Object redundantAssignmentToParam(String param) {", " // BUG: Diagnostic contains:", " param = toString();", " return param;", " }", "", + " String redundantMockAssignmentToParam(String param) {", + " // BUG: Diagnostic contains:", + " param = mock();", + " return param;", + " }", + "", + " Object redundantMockWithExplicitTypeAssignmentToParam(String param) {", + " // BUG: Diagnostic contains:", + " param = mock(String.class);", + " return param;", + " }", + "", + " Object salientMockAssignmentToParam(String param) {", + " param = mock();", + " return param;", + " }", + "", " String redundantAssignmentToLocalVariable() {", " String variable = null;", " // BUG: Diagnostic contains:", @@ -55,6 +75,23 @@ void identification() { " return variable;", " }", "", + " String redundantSpyVariableDeclaration() {", + " // BUG: Diagnostic contains:", + " String variable = spy();", + " return variable;", + " }", + "", + " Object redundantSpyWithExplicitTypeVariableDeclaration() {", + " // BUG: Diagnostic contains:", + " String variable = spy(String.class);", + " return variable;", + " }", + "", + " Object salienSpyTypeVariableDeclaration() {", + " String variable = spy(\"name\");", + " return variable;", + " }", + "", " String unusedVariableDeclaration(String param) {", " String variable = toString();", " return param;", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpersTest.java index 44496738bfc..51ebf913f27 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpersTest.java @@ -8,9 +8,16 @@ import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ExpressionStatementTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; import com.google.errorprone.matchers.Description; +import com.sun.source.tree.ExpressionStatementTree; 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; import java.util.function.BiFunction; import org.junit.jupiter.api.Test; @@ -67,7 +74,56 @@ void methodExistsInEnclosingClass() { .doTest(); } - private static String createDiagnosticsMessage( + @Test + void findMethodExitedOnReturn() { + CompilationTestHelper.newInstance(FindMethodReturnTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import java.util.stream.Stream;", + "", + "class A {", + " {", + " toString();", + " }", + "", + " String m() {", + " // BUG: Diagnostic contains:", + " toString();", + " // BUG: Diagnostic contains:", + " return toString();", + " }", + "", + " Stream m2() {", + " // BUG: Diagnostic contains:", + " return Stream.of(1)", + " .map(", + " n -> {", + " toString();", + " return toString();", + " });", + " }", + "}") + .doTest(); + } + + @Test + void areSameType() { + CompilationTestHelper.newInstance(AreSameTypeTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "class A {", + " void negative1(String a, Integer b) {}", + "", + " // BUG: Diagnostic contains:", + " void positive1(String a, String b) {}", + "", + " // BUG: Diagnostic contains:", + " void positive2(Iterable a, Iterable b) {}", + "}") + .doTest(); + } + + private static String createMethodSearchDiagnosticsMessage( BiFunction valueFunction, VisitorState state) { return Maps.toMap(ImmutableSet.of("foo", "bar", "baz"), key -> valueFunction.apply(key, state)) .toString(); @@ -85,7 +141,7 @@ public static final class FindMethodsTestChecker extends BugChecker implements M public Description matchMethod(MethodTree tree, VisitorState state) { return buildDescription(tree) .setMessage( - createDiagnosticsMessage( + createMethodSearchDiagnosticsMessage( (methodName, s) -> MoreASTHelpers.findMethods(methodName, s).size(), state)) .build(); } @@ -103,8 +159,55 @@ public static final class MethodExistsTestChecker extends BugChecker @Override public Description matchMethod(MethodTree tree, VisitorState state) { return buildDescription(tree) - .setMessage(createDiagnosticsMessage(MoreASTHelpers::methodExistsInEnclosingClass, state)) + .setMessage( + createMethodSearchDiagnosticsMessage( + MoreASTHelpers::methodExistsInEnclosingClass, state)) .build(); } } + + /** + * A {@link BugChecker} that delegates to {@link + * MoreASTHelpers#findMethodExitedOnReturn(VisitorState)}. + */ + @BugPattern(summary = "Interacts with `MoreASTHelpers` for testing purposes", severity = ERROR) + public static final class FindMethodReturnTestChecker extends BugChecker + implements ExpressionStatementTreeMatcher, ReturnTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchExpressionStatement(ExpressionStatementTree tree, VisitorState state) { + return flagMethodReturnLocation(tree, state); + } + + @Override + public Description matchReturn(ReturnTree tree, VisitorState state) { + return flagMethodReturnLocation(tree, state); + } + + private Description flagMethodReturnLocation(Tree tree, VisitorState state) { + return MoreASTHelpers.findMethodExitedOnReturn(state) + .map(m -> buildDescription(tree).setMessage(state.getSourceForNode(m)).build()) + .orElse(Description.NO_MATCH); + } + } + + /** + * A {@link BugChecker} that delegates to {@link MoreASTHelpers#areSameType(Tree, Tree, + * VisitorState)}. + */ + @BugPattern(summary = "Interacts with `MoreASTHelpers` for testing purposes", severity = ERROR) + public static final class AreSameTypeTestChecker extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + List parameters = tree.getParameters(); + return parameters.stream() + .skip(1) + .allMatch(p -> MoreASTHelpers.areSameType(p, parameters.get(0), state)) + ? describeMatch(tree) + : Description.NO_MATCH; + } + } }