Skip to content

Commit

Permalink
Improve Replacement handling
Browse files Browse the repository at this point in the history
* Allow adjacent but non-overlapping replacements.
* Disallow overlapping replacements in SuggestedFix#getReplacements

RELNOTES: N/A
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=120188283
  • Loading branch information
cushon committed Apr 25, 2016
1 parent 505514f commit c4dc5da
Show file tree
Hide file tree
Showing 5 changed files with 228 additions and 78 deletions.
Expand Up @@ -16,24 +16,18 @@

package com.google.errorprone.apply;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.collect.Range;
import com.google.common.collect.RangeMap;
import com.google.common.collect.TreeRangeMap;
import com.google.errorprone.DescriptionListener;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.Replacement;
import com.google.errorprone.fixes.Replacements;
import com.google.errorprone.matchers.Description;

import com.sun.tools.javac.tree.EndPosTable;
import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.LinkedHashSet;
import java.util.Set;

/**
Expand All @@ -45,12 +39,13 @@
* @author lowasser@google.com (Louis Wasserman)
*/
public final class DescriptionBasedDiff implements DescriptionListener, Diff {

private final String sourcePath;
private final JCCompilationUnit compilationUnit;
private final Set<String> importsToAdd;
private final Set<String> importsToRemove;
private final EndPosTable endPositions;
private final RangeMap<Integer, Replacement> replacements;
private final Replacements replacements = new Replacements();

public static DescriptionBasedDiff create(JCCompilationUnit compilationUnit) {
return new DescriptionBasedDiff(compilationUnit);
Expand All @@ -59,10 +54,9 @@ public static DescriptionBasedDiff create(JCCompilationUnit compilationUnit) {
private DescriptionBasedDiff(JCCompilationUnit compilationUnit) {
this.compilationUnit = checkNotNull(compilationUnit);
this.sourcePath = compilationUnit.getSourceFile().toUri().getPath();
this.importsToAdd = new HashSet<>();
this.importsToRemove = new HashSet<>();
this.importsToAdd = new LinkedHashSet<>();
this.importsToRemove = new LinkedHashSet<>();
this.endPositions = compilationUnit.endPositions;
this.replacements = TreeRangeMap.create();
}

@Override
Expand All @@ -71,8 +65,7 @@ public String getRelevantFileName() {
}

public boolean isEmpty() {
return importsToAdd.isEmpty() && importsToRemove.isEmpty()
&& replacements.asMapOfRanges().isEmpty();
return importsToAdd.isEmpty() && importsToRemove.isEmpty() && replacements.isEmpty();
}

@Override
Expand All @@ -83,57 +76,24 @@ public void onDescribed(Description description) {
importsToAdd.addAll(fix.getImportsToAdd());
importsToRemove.addAll(fix.getImportsToRemove());
for (Replacement replacement : fix.getReplacements(endPositions)) {
addReplacement(replacement);
replacements.add(replacement);
}
}
}

private void addReplacement(Replacement replacement) {
checkNotNull(replacement);
Range<Integer> range;
if (replacement.startPosition() == replacement.endPosition()) {
/*
* 1) Inserting Range.closedOpen(N,N) into a RangeMap is a no-op, leading to pure insertions
* being discarded. Therefore, we create a fake range [n,n+1), which has a non-zero
* measure and therefore won't be discarded.
* 2) The RangeMap is only used for ordering insertions, not for removing or inserting text.
* The Replacement still contains the right range (N,N), so no extra characters will be
* deleted.
* 3) This does not work when insertions overlap with replacements or other insertions
* (and will raise an Exception below in that case).
* There is no general solution for this case, e.g. when inserting '(' and '[' at the same
* position, we can't tell whether to insert "([" or "[(". Therefore, insertions cannot
* overlap with replacements, and there can only be one insertion at each position.
* This class is only used in tests, and overlapping replacements do not occur.
*/
range = Range.closedOpen(replacement.startPosition(), replacement.endPosition() + 1);
} else {
range = Range.closedOpen(replacement.startPosition(), replacement.endPosition());
}
RangeMap<Integer, Replacement> overlaps = replacements.subRangeMap(range);
checkArgument(overlaps.asMapOfRanges().isEmpty(), "Replacement %s overlaps with %s",
replacement, overlaps);

replacements.put(range, replacement);
}

@Override
public void applyDifferences(SourceFile sourceFile) throws DiffNotApplicableException {
/*
* We want to apply replacements in reverse order of start position, and we know that imports
* come before all the other replacements.
*/
List<Replacement> replacementsInOrder = new ArrayList<>(replacements.asMapOfRanges().values());
Collections.reverse(replacementsInOrder);
if (!importsToAdd.isEmpty() || !importsToRemove.isEmpty()) {
ImportStatements importStatements = ImportStatements.create(compilationUnit);
importStatements.addAll(importsToAdd);
importStatements.removeAll(importsToRemove);
replacementsInOrder.add(Replacement.create(importStatements.getStartPos(),
importStatements.getEndPos(), importStatements.toString()));
replacements.add(
Replacement.create(
importStatements.getStartPos(),
importStatements.getEndPos(),
importStatements.toString()));
}

for (Replacement replacement : replacementsInOrder) {
for (Replacement replacement : replacements.descending()) {
sourceFile.replaceChars(replacement.startPosition(), replacement.endPosition(),
replacement.replaceWith());
}
Expand Down
42 changes: 28 additions & 14 deletions core/src/main/java/com/google/errorprone/fixes/Replacement.java
Expand Up @@ -19,25 +19,39 @@
import static com.google.common.base.Preconditions.checkArgument;

import com.google.auto.value.AutoValue;
import com.google.common.collect.Range;

/**
* A single replaced section of a source file. When multiple replacements are to be made in a file,
* these should be applied in reverse order of startPosition.
* @author alexeagle@google.com (Alex Eagle)
*/
/** A replaced section of a source file. */
@AutoValue
public abstract class Replacement {

/**
* Creates a {@link Replacement}. Start and end positions are represented as code unit indices
* in a Unicode 16-bit string.
*
* @param startPosition the beginning of the replacement
* @param endPosition the end of the replacement, exclusive
* @param replaceWith the replacement text
*/
public static Replacement create(int startPosition, int endPosition, String replaceWith) {
checkArgument(
0 <= startPosition && startPosition <= endPosition,
"Illegal range [%s, %s)",
startPosition,
endPosition);
return new AutoValue_Replacement(startPosition, endPosition, replaceWith);
checkArgument(startPosition >= 0, "invalid startPosition: %s", startPosition);
return new AutoValue_Replacement(Range.closedOpen(startPosition, endPosition), replaceWith);
}

/** The beginning of the replacement range. */
public int startPosition() {
return range().lowerEndpoint();
}

// positions are character offset from beginning of the source file
public abstract int startPosition();
public abstract int endPosition();
/** The end of the replacement range, exclusive. */
public int endPosition() {
return range().upperEndpoint();
}

/** The {@link Range} to be replaced. */
public abstract Range<Integer> range();

/** The source text to appear in the output. */
public abstract String replaceWith();
}

89 changes: 89 additions & 0 deletions core/src/main/java/com/google/errorprone/fixes/Replacements.java
@@ -0,0 +1,89 @@
/*
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.fixes;

import static com.google.common.base.Preconditions.checkArgument;

import com.google.common.base.Joiner;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.Ordering;
import com.google.common.collect.Range;
import com.google.common.collect.RangeMap;
import com.google.common.collect.TreeRangeMap;

import java.util.Collection;
import java.util.Comparator;
import java.util.LinkedHashSet;
import java.util.Set;
import java.util.TreeMap;

/** A collection of {@link Replacement}s to be made to a source file. */
public class Replacements {

/**
* We apply replacements in reverse order of start position, so that replacements that
* change the length of the input don't affect the position of earlier replacements.
*/
private static final Comparator<Range<Integer>> DESCENDING =
new Comparator<Range<Integer>>() {
@Override
public int compare(Range<Integer> o1, Range<Integer> o2) {
return ComparisonChain.start()
.compare(o1.lowerEndpoint(), o2.lowerEndpoint(), Ordering.natural().reverse())
.compare(o1.upperEndpoint(), o2.upperEndpoint(), Ordering.natural().reverse())
.result();
}
};

private final TreeMap<Range<Integer>, Replacement> replacements = new TreeMap<>(DESCENDING);
private final RangeMap<Integer, Replacement> overlaps = TreeRangeMap.create();

public Replacements add(Replacement replacement) {
if (replacements.containsKey(replacement.range())) {
Replacement existing = replacements.get(replacement.range());
if (!existing.equals(replacement)) {
throw new IllegalArgumentException(
String.format("%s conflicts with existing replacement %s", replacement, existing));
}
} else {
checkOverlaps(replacement);
}
replacements.put(replacement.range(), replacement);
return this;
}

private void checkOverlaps(Replacement replacement) {
Collection<Replacement> overlap =
overlaps.subRangeMap(replacement.range()).asMapOfRanges().values();
checkArgument(
overlap.isEmpty(),
"replacement '%s' overlaps with existing replacements: %s",
replacement.replaceWith(),
Joiner.on(", ").join(overlap));
overlaps.put(replacement.range(), replacement);
}

/** Non-overlapping replacements, sorted in descending order by position. */
public Set<Replacement> descending() {
// TODO(cushon): refactor SuggestedFix#getReplacements and just return a Collection,
return new LinkedHashSet<>(replacements.values());
}

public boolean isEmpty() {
return replacements.isEmpty();
}
}
12 changes: 2 additions & 10 deletions core/src/main/java/com/google/errorprone/fixes/SuggestedFix.java
Expand Up @@ -43,11 +43,9 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.Deque;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;

import javax.annotation.Nullable;
import javax.lang.model.element.ElementKind;
Expand Down Expand Up @@ -103,17 +101,11 @@ public Set<Replacement> getReplacements(EndPosTable endPositions) {
throw new IllegalArgumentException(
"Cannot produce correct replacements without endPositions.");
}
TreeSet<Replacement> replacements = new TreeSet<>(
new Comparator<Replacement>() {
@Override
public int compare(Replacement o1, Replacement o2) {
return Integer.compare(o2.startPosition(), o1.startPosition());
}
});
Replacements replacements = new Replacements();
for (FixOperation fix : fixes) {
replacements.add(fix.getReplacement(endPositions));
}
return replacements;
return replacements.descending();
}

/** {@link Builder#replace(Tree, String)} */
Expand Down

0 comments on commit c4dc5da

Please sign in to comment.