Introduce DirectReturn check (#513)

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<StatementTree> VARIABLE_RETURN = returnStatement(isVariable());
+  private static final Matcher<ExpressionTree> 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<? extends StatementTree> 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<ExpressionTree> 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.
+   *
+   * <p>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();
+  }
+}

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<MethodInvocationTree> MOCKITO_MOCK_OR_SPY =
+  private static final Matcher<MethodInvocationTree> 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);
-  }
 }

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<MethodTree> 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<MethodTree> 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);
+  }
 }