From ce6fdc015b71f7aacf5dcd8c407439d5ae07683d Mon Sep 17 00:00:00 2001 From: Keith Lea Date: Tue, 12 Sep 2023 14:52:14 -0700 Subject: [PATCH] Fix bug where nested `MissingBraces` violations' suggested fixes result in broken code @holtherndon-stripe came across this issue while applying `MissingBraces` to Stripe's code base of 4 million lines of Java code. See unit test for sample code. I'm not sure about a few things: - Why does the coalescing code suppress duplicate inserts? It seems a bit odd that this is the default. I wonder which checkers' fixes need this, and why. - Is `Fix` the right level of abstraction? Or should the coalescing policy be per `Replacement`? Fixes #3797 FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/google/error-prone/pull/3797 from keithl-stripe:keithl-missing-braces-patch de82c5307143d870e260d0f0d6c56af00366952d PiperOrigin-RevId: 564840815 --- .../apply/DescriptionBasedDiff.java | 2 +- .../java/com/google/errorprone/fixes/Fix.java | 2 ++ .../google/errorprone/fixes/SuggestedFix.java | 12 ++++++-- .../errorprone/bugpatterns/MissingBraces.java | 8 ++++- .../bugpatterns/MissingBracesTest.java | 29 +++++++++++++++++++ 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java b/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java index 3df86f4abdd5..8f9b5827be75 100644 --- a/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java +++ b/check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java @@ -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; diff --git a/check_api/src/main/java/com/google/errorprone/fixes/Fix.java b/check_api/src/main/java/com/google/errorprone/fixes/Fix.java index 261434e986e8..d447818392df 100644 --- a/check_api/src/main/java/com/google/errorprone/fixes/Fix.java +++ b/check_api/src/main/java/com/google/errorprone/fixes/Fix.java @@ -40,6 +40,8 @@ default String getShortDescription() { return ""; } + Replacements.CoalescePolicy getCoalescePolicy(); + Set getReplacements(EndPosTable endPositions); Collection getImportsToAdd(); diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java index 365dff1c01d5..5b8f96893ce8 100644 --- a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java +++ b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java @@ -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; @@ -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), @@ -90,8 +92,7 @@ public Set 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(); } @@ -169,6 +170,7 @@ public static class Builder { private final List fixes = new ArrayList<>(); private final Set importsToAdd = new LinkedHashSet<>(); private final Set importsToRemove = new LinkedHashSet<>(); + private CoalescePolicy coalescePolicy = CoalescePolicy.EXISTING_FIRST; private String shortDescription = ""; protected Builder() {} @@ -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); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingBraces.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingBraces.java index 7f1fc5c95954..9e3f27c93afa 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingBraces.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingBraces.java @@ -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; @@ -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())); } } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MissingBracesTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MissingBracesTest.java index 961e30a6872b..dd2ca60b7acf 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/MissingBracesTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MissingBracesTest.java @@ -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 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 items) {", + " for (String item : items) {", + " if (item != null) {", + " return item;", + " }", + " }", + " return null;", + " }", + "}") + .doTest(); + } }