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 new file mode 100644 index 0000000000..ee0091eeeb --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -0,0 +1,131 @@ +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.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; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.BlockTreeMatcher; +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.AssignmentTree; +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; +import com.sun.source.tree.VariableTree; +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; + +/** + * A {@link BugChecker} that flags unnecessary local variable assignments preceding a return + * statement. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Variable assignment is redundant; value can be returned directly", + link = BUG_PATTERNS_BASE_URL + "DirectReturn", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) +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() {} + + @Override + public Description matchBlock(BlockTree tree, VisitorState state) { + List statements = tree.getStatements(); + if (statements.size() < 2) { + return Description.NO_MATCH; + } + + StatementTree finalStatement = statements.get(statements.size() - 1); + if (!VARIABLE_RETURN.matches(finalStatement, state)) { + return Description.NO_MATCH; + } + + Symbol variableSymbol = ASTHelpers.getSymbol(((ReturnTree) finalStatement).getExpression()); + StatementTree precedingStatement = statements.get(statements.size() - 2); + + return tryMatchAssignment(variableSymbol, precedingStatement) + .filter(resultExpr -> canInlineToReturnStatement(resultExpr, state)) + .map( + resultExpr -> + describeMatch( + precedingStatement, + SuggestedFix.builder() + .replace( + precedingStatement, + String.format("return %s;", SourceCode.treeToString(resultExpr, state))) + .delete(finalStatement) + .build())) + .orElse(Description.NO_MATCH); + } + + private static Optional tryMatchAssignment(Symbol targetSymbol, Tree tree) { + if (tree instanceof ExpressionStatementTree) { + return tryMatchAssignment(targetSymbol, ((ExpressionStatementTree) tree).getExpression()); + } + + if (tree instanceof AssignmentTree) { + AssignmentTree assignment = (AssignmentTree) tree; + return targetSymbol.equals(ASTHelpers.getSymbol(assignment.getVariable())) + ? Optional.of(assignment.getExpression()) + : Optional.empty(); + } + + if (tree instanceof VariableTree) { + VariableTree declaration = (VariableTree) tree; + return declaration.getModifiers().getAnnotations().isEmpty() + && targetSymbol.equals(ASTHelpers.getSymbol(declaration)) + ? Optional.ofNullable(declaration.getInitializer()) + : Optional.empty(); + } + + 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 4aa3af46e6..af2e51d8a0 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 ccfd3353bb..b0d4bcaddc 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 new file mode 100644 index 0000000000..be455b123f --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java @@ -0,0 +1,174 @@ +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 DirectReturnTest { + @Test + void identification() { + CompilationTestHelper.newInstance(DirectReturn.class, getClass()) + .addSourceLines( + "A.java", + "import static org.mockito.Mockito.mock;", + "import static org.mockito.Mockito.spy;", + "", + "import java.util.function.Supplier;", + "", + "class A {", + " private String field;", + "", + " void emptyMethod() {}", + "", + " void voidMethod() {", + " toString();", + " return;", + " }", + "", + " String directReturnOfParam(String param) {", + " return param;", + " }", + "", + " String assignmentToField() {", + " field = toString();", + " return field;", + " }", + "", + " 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:", + " variable = toString();", + " return variable;", + " }", + "", + " String unusedAssignmentToLocalVariable(String param) {", + " String variable = null;", + " variable = toString();", + " return param;", + " }", + "", + " String redundantVariableDeclaration() {", + " // BUG: Diagnostic contains:", + " String variable = toString();", + " 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 salientSpyTypeVariableDeclaration() {", + " String variable = spy(\"name\");", + " return variable;", + " }", + "", + " String unusedVariableDeclaration(String param) {", + " String variable = toString();", + " return param;", + " }", + "", + " String assignmentToAnnotatedVariable() {", + " @SuppressWarnings(\"HereBeDragons\")", + " String variable = toString();", + " return variable;", + " }", + "", + " String complexReturnStatement() {", + " String variable = toString();", + " return variable + toString();", + " }", + "", + " String assignmentInsideIfClause() {", + " String variable = null;", + " if (true) {", + " variable = toString();", + " }", + " return variable;", + " }", + "", + " String redundantAssignmentInsideElseClause() {", + " String variable = toString();", + " if (true) {", + " return variable;", + " } else {", + " // BUG: Diagnostic contains:", + " variable = \"foo\";", + " return variable;", + " }", + " }", + "", + " Supplier redundantAssignmentInsideLambda() {", + " return () -> {", + " // BUG: Diagnostic contains:", + " String variable = toString();", + " return variable;", + " };", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(DirectReturn.class, getClass()) + .addInputLines( + "A.java", + "class A {", + " String m1() {", + " String variable = null;", + " variable = toString();", + " return variable;", + " }", + "", + " String m2() {", + " String variable = toString();", + " return variable;", + " }", + "}") + .addOutputLines( + "A.java", + "class A {", + " String m1() {", + " String variable = null;", + " return toString();", + " }", + "", + " String m2() {", + " return toString();", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} 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 44496738bf..19f3db045d 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,70 @@ 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 topLevelMethod() {", + " // BUG: Diagnostic contains: topLevelMethod", + " toString();", + " // BUG: Diagnostic contains: topLevelMethod", + " return toString();", + " }", + "", + " Stream anotherMethod() {", + " // BUG: Diagnostic contains: anotherMethod", + " return Stream.of(1)", + " .map(", + " n -> {", + " toString();", + " return toString();", + " });", + " }", + "", + " void recursiveMethod(Runnable r) {", + " // BUG: Diagnostic contains: recursiveMethod", + " recursiveMethod(", + " new Runnable() {", + " @Override", + " public void run() {", + " // BUG: Diagnostic contains: run", + " toString();", + " }", + " });", + " }", + "}") + .doTest(); + } + + @Test + void areSameType() { + CompilationTestHelper.newInstance(AreSameTypeTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "class A {", + " void negative1(String a, Integer b) {}", + "", + " void negative2(Integer a, Number 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 +155,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 +173,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(m.getName().toString()).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; + } + } }