Apply replacements in forward order instead of reverse

This avoids quadratic re-copying, and also simplifies finding the first edited line.

PiperOrigin-RevId: 439899685

Author: amalloy

check_api/src/main/java/com/google/errorprone/fixes/AppliedFix.java

@@ -18,17 +18,10 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 
-import com.google.common.base.Supplier;
-import com.google.common.base.Suppliers;
-import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.sun.tools.javac.tree.EndPosTable;
-import java.io.IOException;
-import java.io.LineNumberReader;
-import java.io.StringReader;
-import java.util.HashSet;
 import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 
 /**
@@ -57,12 +50,10 @@ public boolean isRemoveLine() {
   public static class Applier {
     private final CharSequence source;
     private final EndPosTable endPositions;
-    private final Supplier<ImmutableSortedMap<Integer, Integer>> lineOffsets;
 
     public Applier(CharSequence source, EndPosTable endPositions) {
       this.source = source;
       this.endPositions = endPositions;
-      this.lineOffsets = Suppliers.memoize(() -> lineOffsets(source.toString()));
     }
 
     /**
@@ -71,86 +62,77 @@ public Applier(CharSequence source, EndPosTable endPositions) {
      */
     @Nullable
     public AppliedFix apply(Fix suggestedFix) {
-      StringBuilder replaced = new StringBuilder(source);
-
-      // We have to apply the replacements in descending order, since otherwise the positions in
-      // subsequent replacements are invalidated by earlier replacements.
-      Set<Replacement> replacements = descending(suggestedFix.getReplacements(endPositions));
+      // We apply the replacements in ascending order here. Descending is simpler, since applying a
+      // replacement can't change the index for future replacements, but it leads to quadratic
+      // copying behavior as we constantly shift the tail of the file around in our StringBuilder.
+      ImmutableSet<Replacement> replacements =
+          ascending(suggestedFix.getReplacements(endPositions));
+      if (replacements.isEmpty()) {
+        return null;
+      }
 
-      Set<Integer> modifiedLines = new HashSet<>();
+      StringBuilder replaced = new StringBuilder();
+      int positionInOriginal = 0;
       for (Replacement repl : replacements) {
         checkArgument(
             repl.endPosition() <= source.length(),
             "End [%s] should not exceed source length [%s]",
             repl.endPosition(),
             source.length());
-        replaced.replace(repl.startPosition(), repl.endPosition(), repl.replaceWith());
-
-        // Find the line number(s) being modified
-        modifiedLines.add(lineOffsets.get().floorEntry(repl.startPosition()).getValue());
-      }
 
-      // Not sure this is really the right behavior, but otherwise we can end up with an infinite
-      // loop below.
-      if (modifiedLines.isEmpty()) {
-        return null;
+        // Write the unmodified content leading up to this change
+        replaced.append(source, positionInOriginal, repl.startPosition());
+        // And the modified content for this change
+        replaced.append(repl.replaceWith());
+        // Then skip everything from source between start and end
+        positionInOriginal = repl.endPosition();
       }
+      // Flush out any remaining content after the final change
+      replaced.append(source, positionInOriginal, source.length());
 
-      LineNumberReader lineNumberReader =
-          new LineNumberReader(new StringReader(replaced.toString()));
-      String snippet = null;
-      boolean isRemoveLine = false;
-      try {
-        while (!modifiedLines.contains(lineNumberReader.getLineNumber())) {
-          lineNumberReader.readLine();
-        }
-        // TODO: this is over-simplified; need a failing test case
-        snippet = lineNumberReader.readLine();
-        if (snippet == null) {
-          // The file's last line was removed.
-          snippet = "";
-        } else {
-          snippet = snippet.trim();
-          // snip comment from line
-          if (snippet.contains("//")) {
-            snippet = snippet.substring(0, snippet.indexOf("//")).trim();
-          }
-        }
-        if (snippet.isEmpty()) {
-          isRemoveLine = true;
-          snippet = "to remove this line";
-        }
-      } catch (IOException e) {
-        // impossible since source is in-memory
+      // Find the changed line containing the first edit
+      String snippet = firstEditedLine(replaced, Iterables.get(replacements, 0));
+      if (snippet.isEmpty()) {
+        return new AppliedFix("to remove this line", /* isRemoveLine= */ true);
       }
-      return new AppliedFix(snippet, isRemoveLine);
+      return new AppliedFix(snippet, /* isRemoveLine= */ false);
     }
 
     /** Get the replacements in an appropriate order to apply correctly. */
-    private static Set<Replacement> descending(Set<Replacement> set) {
+    private static ImmutableSet<Replacement> ascending(Set<Replacement> set) {
       Replacements replacements = new Replacements();
       set.forEach(replacements::add);
-      return replacements.descending();
+      return replacements.ascending();
+    }
+
+    /**
+     * Finds the full text of the first line that's changed. In this case "line" means "bracketed by
+     * \n characters". We don't handle \r\n specially, because the strings that javac provides to
+     * Error Prone have already been transformed from platform line endings to newlines (and even if
+     * it didn't, the dangling \r characters would be handled by a trim() call).
+     */
+    private static String firstEditedLine(StringBuilder content, Replacement firstEdit) {
+      // We subtract 1 here because we want to find the first newline *before* the edit, not one
+      // at its beginning.
+      int startOfFirstEditedLine = content.lastIndexOf("\n", firstEdit.startPosition() - 1);
+      int endOfFirstEditedLine = content.indexOf("\n", firstEdit.startPosition());
+      if (startOfFirstEditedLine == -1) {
+        startOfFirstEditedLine = 0; // Change to start of file with no preceding newline
+      }
+      if (endOfFirstEditedLine == -1) {
+        // Change to last line of file
+        endOfFirstEditedLine = content.length();
+      }
+      String snippet = content.substring(startOfFirstEditedLine, endOfFirstEditedLine);
+      snippet = snippet.trim();
+      if (snippet.contains("//")) {
+        snippet = snippet.substring(0, snippet.indexOf("//")).trim();
+      }
+      return snippet;
     }
   }
 
   public static Applier fromSource(CharSequence source, EndPosTable endPositions) {
     return new Applier(source, endPositions);
   }
-
-  private static final Pattern NEWLINE = Pattern.compile("\\R");
-
-  /** Returns the start offsets of the lines in the input. */
-  private static ImmutableSortedMap<Integer, Integer> lineOffsets(String input) {
-    ImmutableSortedMap.Builder<Integer, Integer> lines = ImmutableSortedMap.naturalOrder();
-    int line = 0;
-    int idx = 0;
-    lines.put(idx, line++);
-    Matcher matcher = NEWLINE.matcher(input);
-    while (matcher.find(idx)) {
-      idx = matcher.end();
-      lines.put(idx, line++);
-    }
-    return lines.buildOrThrow();
-  }
 }

check_api/src/main/java/com/google/errorprone/fixes/Replacements.java

@@ -20,6 +20,7 @@
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Range;
 import com.google.common.collect.RangeMap;
@@ -148,6 +149,13 @@ public Set<Replacement> descending() {
     return new LinkedHashSet<>(replacements.values());
   }
 
+  /** Non-overlapping replacements, sorted in ascending order by position. */
+  public ImmutableSet<Replacement> ascending() {
+    // TODO(amalloy): Encourage using this instead of descending()
+    // Applying replacements in forward order is substantially more efficient, and only a bit harder
+    return ImmutableSet.copyOf(replacements.descendingMap().values());
+  }
+
   public boolean isEmpty() {
     return replacements.isEmpty();
   }