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 00000000000..268c1550e0c --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -0,0 +1,101 @@ +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.isVariable; +import static com.google.errorprone.matchers.Matchers.returnStatement; +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.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.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()); + + /** 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) + .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(); + } +} 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 00000000000..ccfda3179b0 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java @@ -0,0 +1,127 @@ +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", + "class A {", + " private String field;", + "", + " void emptyMethod() {}", + "", + " void voidMethod() {", + " toString();", + " return;", + " }", + "", + " String directReturnOfParam(String param) {", + " return param;", + " }", + "", + " String assignmentToField() {", + " field = toString();", + " return field;", + " }", + "", + " String redundantAssignmentToParam(String param) {", + " // BUG: Diagnostic contains:", + " param = toString();", + " 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 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;", + " }", + " }", + "}") + .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); + } +}