Skip to content

Commit

Permalink
Suggest fixes for some DeeplyNested findings
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 635622996
  • Loading branch information
@cushon
cushon authored and Error Prone Team committed May 21, 2024
1 parent a2682da commit ae77a5f
Show file tree
Hide file tree
Showing 2 changed files with 278 additions and 6 deletions.
143 changes: 137 additions & 6 deletions core/src/main/java/com/google/errorprone/bugpatterns/DeeplyNested.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -43,25 +69,130 @@ public class DeeplyNested extends BugChecker implements CompilationUnitTreeMatch

@Override
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
Tree result =
new SuppressibleTreePathScanner<Tree, Integer>(state) {
TreePath result =
new SuppressibleTreePathScanner<TreePath, Integer>(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<Tree> 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<ExpressionTree> 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<ErrorProneToken> 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 <builder>.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 = <builder>` to declare a helper method named create<builder>
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");
}
}
141 changes: 141 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/DeeplyNestedTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<Integer> createXs() {",
" return ImmutableList.<Integer>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<Integer> createXs() {",
" ImmutableList.Builder<Integer> builder = ImmutableList.<Integer>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<Integer> XS = ",
" ImmutableList.<Integer>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<Integer> XS = createXs();",
" private static ImmutableList<Integer> createXs() {",
" ImmutableList.Builder<Integer> builder = ImmutableList.<Integer>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<Integer> XS = ",
" new ImmutableList.Builder<Integer>()",
" .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<Integer> XS = createXs();",
" private static ImmutableList<Integer> createXs() {",
" ImmutableList.Builder<Integer> builder = new ImmutableList.Builder<Integer>();",
" 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
Expand Down

0 comments on commit ae77a5f

Please sign in to comment.