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.

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

Fix bug where nested MissingBraces violations' suggested fixes result in broken code #3797

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void handleFix(Fix fix) {
importsToRemove.addAll(fix.getImportsToRemove());
for (Replacement replacement : fix.getReplacements(endPositions)) {
try {
replacements.add(replacement, Replacements.CoalescePolicy.EXISTING_FIRST);
replacements.add(replacement, fix.getCoalescePolicy());
} catch (IllegalArgumentException iae) {
if (!ignoreOverlappingFixes) {
throw iae;
Expand Down
2 changes: 2 additions & 0 deletions check_api/src/main/java/com/google/errorprone/fixes/Fix.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ default String getShortDescription() {
return "";
}

Replacements.CoalescePolicy getCoalescePolicy();

Set<Replacement> getReplacements(EndPosTable endPositions);

Collection<String> getImportsToAdd();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.fixes.Replacements.CoalescePolicy;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
Expand All @@ -46,6 +47,7 @@ public abstract class SuggestedFix implements Fix {

private static SuggestedFix create(SuggestedFix.Builder builder) {
return new AutoValue_SuggestedFix(
builder.coalescePolicy,
ImmutableList.copyOf(builder.fixes),
ImmutableSet.copyOf(builder.importsToAdd),
ImmutableSet.copyOf(builder.importsToRemove),
Expand Down Expand Up @@ -90,8 +92,7 @@ public Set<Replacement> getReplacements(EndPosTable endPositions) {
}
Replacements replacements = new Replacements();
for (FixOperation fix : fixes()) {
replacements.add(
fix.getReplacement(endPositions), Replacements.CoalescePolicy.EXISTING_FIRST);
replacements.add(fix.getReplacement(endPositions), getCoalescePolicy());
}
return replacements.ascending();
}
Expand Down Expand Up @@ -169,6 +170,7 @@ public static class Builder {
private final List<FixOperation> fixes = new ArrayList<>();
private final Set<String> importsToAdd = new LinkedHashSet<>();
private final Set<String> importsToRemove = new LinkedHashSet<>();
private CoalescePolicy coalescePolicy = CoalescePolicy.EXISTING_FIRST;
private String shortDescription = "";

protected Builder() {}
Expand Down Expand Up @@ -199,6 +201,12 @@ public Builder setShortDescription(String shortDescription) {
return this;
}

@CanIgnoreReturnValue
public Builder setCoalescePolicy(CoalescePolicy coalescePolicy) {
this.coalescePolicy = coalescePolicy;
return this;
}

@CanIgnoreReturnValue
public Builder replace(Tree node, String replaceWith) {
checkNotSyntheticConstructor(node);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.errorprone.bugpatterns;

import static com.google.errorprone.fixes.Replacements.CoalescePolicy.KEEP_ONLY_IDENTICAL_INSERTS;
import static com.google.errorprone.matchers.Description.NO_MATCH;

import com.google.errorprone.BugPattern;
Expand Down Expand Up @@ -93,7 +94,12 @@ void check(StatementTree tree, VisitorState state) {
if (tree != null && !(tree instanceof BlockTree)) {
state.reportMatch(
describeMatch(
tree, SuggestedFix.builder().prefixWith(tree, "{").postfixWith(tree, "}").build()));
tree,
SuggestedFix.builder()
.prefixWith(tree, "{")
.postfixWith(tree, "}")
.setCoalescePolicy(KEEP_ONLY_IDENTICAL_INSERTS)
.build()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,33 @@ public void negative() {
"}")
.doTest();
}

@Test
public void suggestedFixForNestedProblemsWithOverlappingBracePostfixInsertions() {
BugCheckerRefactoringTestHelper.newInstance(MissingBraces.class, getClass())
.addInputLines(
"Test.java",
"import java.util.List;",
"class Test {",
" private String findNotNull(List<String> items) {",
" for (String item : items)",
" if (item != null) return item;",
" return null;",
" }",
"}")
.addOutputLines(
"Test.java",
"import java.util.List;",
"class Test {",
" private String findNotNull(List<String> items) {",
" for (String item : items) {",
" if (item != null) {",
" return item;",
" }",
" }",
" return null;",
" }",
"}")
.doTest();
}
}