diff --git a/check_api/src/main/java/com/google/errorprone/fixes/AppliedFix.java b/check_api/src/main/java/com/google/errorprone/fixes/AppliedFix.java index 370e5b74289..436ea15cf92 100644 --- a/check_api/src/main/java/com/google/errorprone/fixes/AppliedFix.java +++ b/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> 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 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 replacements = + ascending(suggestedFix.getReplacements(endPositions)); + if (replacements.isEmpty()) { + return null; + } - Set 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 descending(Set set) { + private static ImmutableSet ascending(Set 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 lineOffsets(String input) { - ImmutableSortedMap.Builder 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(); - } } diff --git a/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java b/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java index 1f60d51342b..b3ff921353f 100644 --- a/check_api/src/main/java/com/google/errorprone/fixes/Replacements.java +++ b/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 descending() { return new LinkedHashSet<>(replacements.values()); } + /** Non-overlapping replacements, sorted in ascending order by position. */ + public ImmutableSet 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(); }