Fix bug where nested MissingBraces violations' suggested fixes resu…

…lt 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=#3797 from keithl-stripe:keithl-missing-braces-patch de82c53 PiperOrigin-RevId: 564840815
google · Sep 19, 2023 · ce6fdc0 · ce6fdc0
1 parent 39d2884
commit ce6fdc0
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 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
@@ -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
@@ -40,6 +40,8 @@ default String getShortDescription() {
     return "";
   }
 
+  Replacements.CoalescePolicy getCoalescePolicy();
+
   Set<Replacement> getReplacements(EndPosTable endPositions);
 
   Collection<String> 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
@@ -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<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();
   }
@@ -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() {}
@@ -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
@@ -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
@@ -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();
+  }
 }