Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest fixes for some DeeplyNested findings #4405

Merged
merged 1 commit into from
May 21, 2024
Merged

Suggest fixes for some DeeplyNested findings #4405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 137 additions & 6 deletions core/src/main/java/com/google/errorprone/bugpatterns/DeeplyNested.java
View file
Open in desktop
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
Open in desktop
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
Loading