Skip to content

Commit

Permalink
Apply replacements in forward order instead of reverse
Browse files Browse the repository at this point in the history 
This avoids quadratic re-copying, and also simplifies finding the first edited line.

PiperOrigin-RevId: 439899685
  • Loading branch information
@amalloy
amalloy authored and Error Prone Team committed Apr 6, 2022
1 parent 3498bb9 commit 186334b
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 71 deletions.
124 changes: 53 additions & 71 deletions check_api/src/main/java/com/google/errorprone/fixes/AppliedFix.java
View file
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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()));
}

/**
Expand All @@ -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();
}
}
8 changes: 8 additions & 0 deletions check_api/src/main/java/com/google/errorprone/fixes/Replacements.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down

0 comments on commit 186334b

Please sign in to comment.