diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DeeplyNested.java b/core/src/main/java/com/google/errorprone/bugpatterns/DeeplyNested.java index 3ac55d780d6..54febe9afb0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DeeplyNested.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DeeplyNested.java @@ -16,16 +16,42 @@ package com.google.errorprone.bugpatterns; +import static com.google.common.base.CaseFormat.UPPER_CAMEL; +import static com.google.common.base.CaseFormat.UPPER_UNDERSCORE; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.fixes.SuggestedFixes.prettyType; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getResultType; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSameType; +import static com.google.errorprone.util.ASTHelpers.isStatic; +import com.google.common.collect.Iterators; +import com.google.common.collect.PeekingIterator; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ErrorProneToken; import com.sun.source.tree.CompilationUnitTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.ReturnTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.parser.Tokens; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import javax.inject.Inject; /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ @@ -43,25 +69,130 @@ public class DeeplyNested extends BugChecker implements CompilationUnitTreeMatch @Override public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { - Tree result = - new SuppressibleTreePathScanner(state) { + TreePath result = + new SuppressibleTreePathScanner(state) { @Override - public Tree scan(Tree tree, Integer depth) { + public TreePath scan(Tree tree, Integer depth) { if (depth > maxDepth) { - return tree; + return getCurrentPath(); } return super.scan(tree, depth + 1); } @Override - public Tree reduce(Tree r1, Tree r2) { + public TreePath reduce(TreePath r1, TreePath r2) { return r1 != null ? r1 : r2; } }.scan(state.getPath(), 0); if (result != null) { - return describeMatch(result); + return describeMatch(result.getLeaf(), buildFix(result, state)); } return NO_MATCH; } + + private static Fix buildFix(TreePath path, VisitorState state) { + PeekingIterator it = Iterators.peekingIterator(path.iterator()); + + // Find enclosing chained calls that return a Builder type + while (it.hasNext() && !builderResult(it.peek())) { + it.next(); + } + if (!it.hasNext()) { + return SuggestedFix.emptyFix(); + } + ExpressionTree receiver = null; + while (it.hasNext() && builderResult(it.peek())) { + receiver = (ExpressionTree) it.peek(); + it.next(); + } + + // Look for an enclosing terminal .build() call + it.next(); // skip past the enclosing MemberSelectTree + if (!terminalBuilder(it.peek())) { + return SuggestedFix.emptyFix(); + } + + // Look for the enclosing tree, e.g. a field that is being initialized, or a return + it.next(); + Tree enclosing = it.next(); + + // Descend back into the tree to final all chained calls on the same builder + Type builderType = getResultType(receiver); + List chain = new ArrayList<>(); + while (receiver != null && isSameType(getResultType(receiver), builderType, state)) { + chain.add(receiver); + if (receiver instanceof NewClassTree) { + break; + } + receiver = ASTHelpers.getReceiver(receiver); + } + Collections.reverse(chain); + + // Emit a fix + SuggestedFix.Builder fix = SuggestedFix.builder(); + StringBuilder replacement = new StringBuilder(); + // Create a variable to hold the builder + replacement.append( + String.format( + "%s builder = %s;", + prettyType(state, fix, builderType), state.getSourceForNode(chain.get(0)))); + // Update all subsequence chained calls to use the variable as their receiver + for (int i = 1; i < chain.size(); i++) { + int start = state.getEndPosition(chain.get(i - 1)); + int end = state.getEndPosition(chain.get(i)); + List tokens = state.getOffsetTokens(start, end); + int dot = + tokens.stream().filter(t -> t.kind() == Tokens.TokenKind.DOT).findFirst().get().pos(); + replacement.append( + String.format( + "%sbuilder%s;", + state.getSourceCode().subSequence(start, dot), + state.getSourceCode().subSequence(dot, end))); + } + + if (enclosing instanceof ReturnTree) { + // update `return .build();` to use a variable in the same scope + replacement.append("return builder.build();"); + fix.replace(enclosing, replacement.toString()); + return fix.build(); + } else if (enclosing instanceof VariableTree && isStatic(getSymbol(enclosing))) { + // update `static FOO = ` to declare a helper method named create + VariableTree variableTree = (VariableTree) enclosing; + String factory = + String.format( + "create%s", + UPPER_UNDERSCORE.converterTo(UPPER_CAMEL).convert(variableTree.getName().toString())); + fix.replace(variableTree.getInitializer(), String.format("%s()", factory)); + fix.postfixWith( + variableTree, + String.format( + "private static %s %s() { %s return builder.build(); }", + prettyType(state, fix, getType(variableTree)), factory, replacement)); + return fix.build(); + } else { + // TODO: support other patterns + return SuggestedFix.emptyFix(); + } + } + + private static boolean builderResult(Tree leaf) { + if (!(leaf instanceof ExpressionTree)) { + return false; + } + Type resultType = getResultType((ExpressionTree) leaf); + if (resultType == null) { + return false; + } + return resultType.asElement().getSimpleName().contentEquals("Builder"); + } + + private static boolean terminalBuilder(Tree leaf) { + if (!(leaf instanceof MethodInvocationTree)) { + return false; + } + ExpressionTree select = ((MethodInvocationTree) leaf).getMethodSelect(); + return select instanceof MemberSelectTree + && ((MemberSelectTree) select).getIdentifier().toString().startsWith("build"); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DeeplyNestedTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DeeplyNestedTest.java index 18332d5aa34..009127f700e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/DeeplyNestedTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/DeeplyNestedTest.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; import org.junit.Test; import org.junit.runner.RunWith; @@ -27,6 +28,146 @@ public class DeeplyNestedTest { private final CompilationTestHelper testHelper = CompilationTestHelper.newInstance(DeeplyNested.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(DeeplyNested.class, getClass()); + + @Test + public void refactoringReturn() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " static ImmutableList createXs() {", + " return ImmutableList.builder()", + " .add(1)", + " .add(2)", + " .add(3)", + " .add(4)", + " // comment", + " .add(5)", + " .add(6)", + " .add(7)", + " .add(8)", + " .add(9)", + " .add(10)", + " .build();", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " static ImmutableList createXs() {", + " ImmutableList.Builder builder = ImmutableList.builder();", + " builder.add(1);", + " builder.add(2);", + " builder.add(3);", + " builder.add(4);", + " // comment", + " builder.add(5);", + " builder.add(6);", + " builder.add(7);", + " builder.add(8);", + " builder.add(9);", + " builder.add(10);", + " return builder.build();", + " }", + "}") + .setArgs("-XepOpt:DeeplyNested:MaxDepth=10") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void refactoringField() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " static final ImmutableList XS = ", + " ImmutableList.builder()", + " .add(1)", + " .add(2)", + " .add(3)", + " .add(4)", + " .add(5)", + " .add(6)", + " .add(7)", + " .add(8)", + " .add(9)", + " .add(10)", + " .build();", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " static final ImmutableList XS = createXs();", + " private static ImmutableList createXs() {", + " ImmutableList.Builder builder = ImmutableList.builder();", + " builder.add(1);", + " builder.add(2);", + " builder.add(3);", + " builder.add(4);", + " builder.add(5);", + " builder.add(6);", + " builder.add(7);", + " builder.add(8);", + " builder.add(9);", + " builder.add(10);", + " return builder.build();", + " }", + "}") + .setArgs("-XepOpt:DeeplyNested:MaxDepth=10") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void refactoringNewBuilder() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " static final ImmutableList XS = ", + " new ImmutableList.Builder()", + " .add(1)", + " .add(2)", + " .add(3)", + " .add(4)", + " .add(5)", + " .add(6)", + " .add(7)", + " .add(8)", + " .add(9)", + " .add(10)", + " .build();", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "class Test {", + " static final ImmutableList XS = createXs();", + " private static ImmutableList createXs() {", + " ImmutableList.Builder builder = new ImmutableList.Builder();", + " builder.add(1);", + " builder.add(2);", + " builder.add(3);", + " builder.add(4);", + " builder.add(5);", + " builder.add(6);", + " builder.add(7);", + " builder.add(8);", + " builder.add(9);", + " builder.add(10);", + " return builder.build();", + " }", + "}") + .setArgs("-XepOpt:DeeplyNested:MaxDepth=10") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + @Test public void positive() { testHelper