Fix overlapping replacement issues and broken tests.

2 files changed, 235 insertions, 71 deletions

diff --git a/errorprone/src/main/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheck.java b/errorprone/src/main/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheck.java
index 70e1cbd57..af5cbac7b 100644
--- a/errorprone/src/main/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheck.java
+++ b/errorprone/src/main/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheck.java
@@ -52,6 +52,7 @@ import com.sun.tools.javac.code.Type.UnknownType;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.tree.JCTree;
 import com.sun.tools.javac.tree.JCTree.JCAssign;
+import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
 import com.sun.tools.javac.tree.JCTree.JCExpression;
 import com.sun.tools.javac.tree.JCTree.JCFieldAccess;
 import com.sun.tools.javac.tree.JCTree.JCIdent;
@@ -61,9 +62,12 @@ import com.sun.tools.javac.tree.JCTree.JCNewClass;
 import com.sun.tools.javac.tree.JCTree.JCTypeCast;
 import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
 import com.sun.tools.javac.tree.TreeMaker;
+import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition;
 import com.sun.tools.javac.util.Name;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
@@ -108,20 +112,16 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
       return NO_MATCH;
     }
 
-    final ShadowInliner shadowInliner = new ShadowInliner();
+    final ShadowInliner shadowInliner = new ShadowInliner(
+        (JCCompilationUnit) state.getPath().getCompilationUnit());
     shadowInliner.scan(tree, state);
 
-    for (Runnable runnable : shadowInliner.possibleFixes.values()) {
-      runnable.run();
-    }
-
-    Fix fix = shadowInliner.fixBuilder.build();
+    Fix fix = shadowInliner.possibleFixes.getFix();
     return fix.isEmpty() ? NO_MATCH : describeMatch(tree, fix);
   }
 
   static class ShadowInliner extends TreeScanner<Void, VisitorState> {
-    private final SuggestedFix.Builder fixBuilder;
-    private final Map<Tree, Runnable> possibleFixes;
+    private final PossibleFixes possibleFixes;
 
     private final Set<String> knownFields = new HashSet<>();
     private final Set<String> knownLocalVars = new HashSet<>();
@@ -129,12 +129,11 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
 
     private boolean inShadowClass;
 
-    ShadowInliner() {
-      this(SuggestedFix.builder(), new HashMap<>());
+    ShadowInliner(JCCompilationUnit compilationUnit) {
+      this(new PossibleFixes(SuggestedFix.builder(), compilationUnit));
     }
 
-    ShadowInliner(SuggestedFix.Builder fixBuilder, Map<Tree, Runnable> possibleFixes) {
-      this.fixBuilder = fixBuilder;
+    ShadowInliner(PossibleFixes possibleFixes) {
       this.possibleFixes = possibleFixes;
     }
 
@@ -169,7 +168,8 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
             // replace shadow variable declaration with shadowed type and name
             if (!newVarName.equals(shadowOfArg.toString())) {
               Type shadowedType = getUpperBound(shadowOfArgType, state);
-              String shadowedTypeName = SuggestedFixes.prettyType(state, fixBuilder, shadowedType);
+              String shadowedTypeName =
+                  SuggestedFixes.prettyType(state, possibleFixes.fixBuilder, shadowedType);
               String newAssignment =
                   shadowedTypeName + " " + newVarName + " = " + shadowOfArg + ";";
 
@@ -177,15 +177,13 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
               if (shadowOfArg instanceof JCMethodInvocation) {
                 JCExpression jcExpression = ((JCMethodInvocation) shadowOfArg).meth;
                 if (jcExpression instanceof JCFieldAccess) {
-                  possibleFixes.remove(((JCFieldAccess) jcExpression).selected);
+                  possibleFixes.removeFixFor(((JCFieldAccess) jcExpression).selected);
                 }
               }
 
-              possibleFixes.remove(parent);
-              fixBuilder.replace(parent, newAssignment);
+              possibleFixes.fixByReplacing(parent, newAssignment);
             } else {
-              possibleFixes.remove(parent);
-              fixBuilder.delete(parent);
+              possibleFixes.fixByDeleting(parent);
             }
 
             // replace shadow variable reference with `nonShadowInstance` or
@@ -204,26 +202,24 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
                   JCTree replaceNode;
                   if (parent instanceof JCFieldAccess && !callDirectlyOnFramework) {
                     JCFieldAccess fieldAccess = (JCFieldAccess) parent;
-                    replaceNode =
-                        fieldAccess.selected =
-                            createSyntheticShadowAccess(
-                                identifierTree, fieldAccess, shadowOfCall, newVarName, symbol,
-                                state);
+                    JCMethodInvocation newShadowOfCall =
+                        createSyntheticShadowAccess(shadowOfCall, newVarName, symbol, state);
+
+                    replaceFieldSelected(fieldAccess, newShadowOfCall, state);
+                    replaceNode = newShadowOfCall;
                   } else {
                     identifierTree.name = state.getName(newVarName);
                     identifierTree.sym.name = state.getName(newVarName);
                     replaceNode = identifierTree;
                   }
 
+                  String replaceWith =
+                      callDirectlyOnFramework
+                          ? newVarName
+                          : shadowOfCall.getMethodSelect() + "(" + newVarName + ")";
+
                   possibleFixes.put(
-                      replaceNode,
-                      () -> {
-                        fixBuilder.replace(
-                            identifierTree,
-                            callDirectlyOnFramework
-                                ? newVarName
-                                : shadowOfCall.getMethodSelect() + "(" + newVarName + ")");
-                      });
+                      replaceNode, possibleFixes.new ReplacementFix(identifierTree, replaceWith));
                 }
                 return super.visitIdentifier(identifierTree, aVoid);
               }
@@ -266,12 +262,11 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
                 TreePath assignmentPath = TreePath.getPath(compilationUnit, assignment);
                 Tree assignmentParent = assignmentPath.getParentPath().getLeaf();
                 if (assignmentParent instanceof ExpressionStatementTree) {
-                  possibleFixes.remove(assignmentParent);
-                  fixBuilder.delete(assignmentParent);
+                  possibleFixes.fixByDeleting(assignmentParent);
                 }
               } else {
-                possibleFixes.remove(assignment);
-                fixBuilder.replace(assignment, newFieldName + " = " + shadowOfArg.toString());
+                possibleFixes.fixByReplacing(
+                    assignment, newFieldName + " = " + shadowOfArg.toString());
               }
               break;
             }
@@ -289,31 +284,32 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
                     && !isMethodParam(ASTHelpers.getSymbol(shadowOfArg), state.getPath());
 
             if (useExistingField) {
-              fixVar(fieldSymbol, state, fixBuilder).delete();
+              fixVar(fieldSymbol, state, possibleFixes).delete();
 
               ExpressionStatementTree enclosingNode =
                   ASTHelpers.findEnclosingNode(
-                      TreePath.getPath(compilationUnit, parent), ExpressionStatementTree.class);
+                      TreePath.getPath(compilationUnit, assignment), ExpressionStatementTree.class);
               if (enclosingNode != null) {
-                fixBuilder.delete(enclosingNode);
+                possibleFixes.fixByDeleting(enclosingNode);
               }
             } else {
               Type shadowedType = getUpperBound(shadowOfArgType, state);
-              String shadowedTypeName = SuggestedFixes.prettyType(state, fixBuilder, shadowedType);
-              fixVar(fieldSymbol, state, fixBuilder)
+              String shadowedTypeName =
+                  SuggestedFixes.prettyType(state, possibleFixes.fixBuilder, shadowedType);
+              fixVar(fieldSymbol, state, possibleFixes)
                   .setName(newFieldName)
                   .setTypeName(shadowedTypeName)
                   .setRenameUses(false)
                   .modify();
 
               String thisStr = "";
-              if (((JCAssign) parent).lhs.toString().startsWith("this.")
+              if (assignment.lhs.toString().startsWith("this.")
                   || (shadowOfArgDomicile == ElementKind.LOCAL_VARIABLE && namesAreSame)) {
                 thisStr = "this.";
               }
 
-              possibleFixes.remove(parent);
-              fixBuilder.replace(parent, thisStr + newFieldName + " = " + shadowOfArg);
+              possibleFixes.fixByReplacing(
+                  assignment, thisStr + newFieldName + " = " + shadowOfArg);
             }
 
             TreePath containingBlock = findParentOfKind(state, Kind.BLOCK);
@@ -349,14 +345,11 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
                       JCTree replaceNode = (JCTree) subject;
                       Tree container = subjectPath.getParentPath().getLeaf();
                       if (container instanceof JCFieldAccess) {
-                        replaceNode =
-                            createSyntheticShadowAccess(
-                                replaceNode,
-                                (JCFieldAccess) container,
-                                shadowOfCall,
-                                newFieldName,
-                                symbol,
-                                state);
+                        JCFieldAccess fieldAccess = (JCFieldAccess) container;
+                        JCMethodInvocation newShadowOfCall =
+                            createSyntheticShadowAccess(shadowOfCall, newFieldName, symbol, state);
+                        replaceFieldSelected(fieldAccess, newShadowOfCall, state);
+                        replaceNode = newShadowOfCall;
                       }
 
                       String replaceWith =
@@ -364,7 +357,7 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
                               ? fieldRef
                               : shadowOfCall.getMethodSelect() + "(" + fieldRef + ")";
                       possibleFixes.put(
-                          replaceNode, () -> fixBuilder.replace(subject, replaceWith));
+                          replaceNode, possibleFixes.new ReplacementFix(subject, replaceWith));
                     }
                   }
                 }
@@ -377,7 +370,7 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
           {
             if (shouldCallDirectlyOnFramework(state.getPath())) {
               if (!isInSyntheticShadowAccess(state)) {
-                fixBuilder.replace(shadowOfCall, shadowOfArg.toString());
+                possibleFixes.fixByReplacing(shadowOfCall, shadowOfArg.toString());
               }
             }
           }
@@ -393,6 +386,29 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
       }
     }
 
+    private void replaceFieldSelected(JCFieldAccess fieldAccess, JCMethodInvocation newShadowOfCall,
+        VisitorState state) {
+      int priorStartPosition = fieldAccess.selected.getStartPosition();
+      int priorEndPosition = getEndPosition(state, fieldAccess.selected);
+
+      fieldAccess.selected = newShadowOfCall;
+
+      newShadowOfCall.pos = priorStartPosition;
+      if (newShadowOfCall.meth instanceof JCIdent) {
+        ((JCIdent) newShadowOfCall.meth).pos = priorStartPosition;
+      }
+      setEndPosition(state, newShadowOfCall, priorEndPosition);
+    }
+
+    private int getEndPosition(VisitorState state, JCTree node) {
+      return state.getEndPosition(node);
+    }
+
+    private void setEndPosition(VisitorState state, JCTree tree, int endPosition) {
+      JCCompilationUnit compilationUnit = (JCCompilationUnit) state.getPath().getCompilationUnit();
+      compilationUnit.endPositions.storeEnd(tree, endPosition);
+    }
+
     @Override
     public Void visitClass(ClassTree classTree, VisitorState visitorState) {
       boolean priorInShadowClass = inShadowClass;
@@ -430,8 +446,9 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
         ClassSymbol owner = (ClassSymbol) methodSelect.sym.owner;
 
         ClassType shadowedClass = determineShadowedClassName(owner, nowState);
-        String shadowedTypeName = SuggestedFixes.prettyType(state, fixBuilder, shadowedClass);
-        fixBuilder.replace(methodSelect.selected, shadowedTypeName);
+        String shadowedTypeName =
+            SuggestedFixes.prettyType(state, possibleFixes.fixBuilder, shadowedClass);
+        possibleFixes.fixByReplacing(methodSelect.selected, shadowedTypeName);
       }
 
       if (!inShadowClass && shadowOfMatcher.matches(tree, nowState)) {
@@ -452,8 +469,6 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
   }
 
   private static JCMethodInvocation createSyntheticShadowAccess(
-      JCTree replaceNode,
-      JCFieldAccess fieldAccess,
       MethodInvocationTree shadowOfCall,
       String newFieldName,
       Symbol originalSymbol,
@@ -463,14 +478,17 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
     Symbol newSymbol = createSymbol(originalSymbol, state.getName(newFieldName),
         ((JCExpression) shadowOfCall.getArguments().get(0)).type);
 
+    JCExpression methodSelect = (JCExpression) shadowOfCall.getMethodSelect();
+    if (methodSelect instanceof JCIdent) {
+      // clone so start pos can be changed...
+      methodSelect = treeMaker.Ident(((JCIdent) shadowOfCall.getMethodSelect()).sym);
+    }
     JCMethodInvocation callShadowOf =
         treeMaker.Apply(
             null,
-            (JCExpression) shadowOfCall.getMethodSelect(),
+            methodSelect,
             com.sun.tools.javac.util.List.of(createIdent(treeMaker, newSymbol)));
     callShadowOf.type = ((JCMethodInvocation) shadowOfCall).type;
-    fieldAccess.selected = callShadowOf;
-    callShadowOf.pos = replaceNode.pos;
     return callShadowOf;
   }
 
@@ -642,23 +660,23 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
    * Renames the given {@link Symbol} and its usages in the current compilation unit to {@code
    * newName}.
    */
-  static VariableFixer fixVar(Symbol symbol, VisitorState state, Builder fixBuilder) {
-    return new VariableFixer(symbol, state, fixBuilder);
+  static VariableFixer fixVar(Symbol symbol, VisitorState state, PossibleFixes possibleFixes) {
+    return new VariableFixer(symbol, state, possibleFixes);
   }
 
   private static class VariableFixer {
 
     private final Symbol symbol;
     private final VisitorState state;
-    private final SuggestedFix.Builder fixBuilder;
+    private final PossibleFixes possibleFixes;
     private boolean renameUses = true;
     private String newName;
     private String newTypeName;
 
-    public VariableFixer(Symbol symbol, VisitorState state, Builder fixBuilder) {
+    public VariableFixer(Symbol symbol, VisitorState state, PossibleFixes possibleFixes) {
       this.symbol = symbol;
       this.state = state;
-      this.fixBuilder = fixBuilder;
+      this.possibleFixes = possibleFixes;
     }
 
     VariableFixer setName(String newName) {
@@ -685,7 +703,7 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
             // For a lambda parameter without explicit type, it will return null.
             String source = state.getSourceForNode(variableTree.getType());
             if (newTypeName != null) {
-              fixBuilder.replace(variableTree.getType(), newTypeName);
+              possibleFixes.fixByReplacing(variableTree.getType(), newTypeName);
             }
 
             if (newName != null && !newName.equals(name)) {
@@ -693,7 +711,7 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
               int pos =
                   ((JCTree) variableTree).getStartPosition()
                       + state.getSourceForNode(variableTree).indexOf(name, typeLength);
-              fixBuilder.replace(pos, pos + name.length(), newName);
+              possibleFixes.fixByReplacing(pos, pos + name.length(), newName);
             }
           }
 
@@ -708,7 +726,7 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
                   @Override
                   public void visitIdent(JCTree.JCIdent tree) {
                     if (symbol.equals(getSymbol(tree))) {
-                      fixBuilder.replace(tree, newName);
+                      possibleFixes.fixByReplacing(tree, newName);
                     }
                   }
                 });
@@ -720,7 +738,7 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
         @Override
         public Void visitVariable(VariableTree variableTree, Void v) {
           if (getSymbol(variableTree).equals(symbol)) {
-            fixBuilder.delete(variableTree);
+            possibleFixes.fixByDeleting(variableTree);
           }
 
           return super.visitVariable(variableTree, v);
@@ -755,4 +773,150 @@ public final class ShadowUsageCheck extends BugChecker implements ClassTreeMatch
   private static Tree findParent(Tree node, VisitorState state) {
     return TreePath.getPath(state.getPath().getCompilationUnit(), node).getParentPath().getLeaf();
   }
+
+  static class PossibleFixes {
+
+    private final Builder fixBuilder;
+    private final JCCompilationUnit compilationUnit;
+    private final Map<Tree, PossibleFix> map = new HashMap<>();
+    private final List<PossibleFix> list = new ArrayList<>();
+
+    public PossibleFixes(Builder builder, JCCompilationUnit compilationUnit) {
+      fixBuilder = builder;
+      this.compilationUnit = compilationUnit;
+    }
+
+    public void put(Tree tree, PossibleFix possibleFix) {
+      if (tree != null) {
+        PossibleFix priorFix = map.put(tree, possibleFix);
+        if (priorFix != null) {
+          list.remove(priorFix);
+        }
+      }
+
+      list.add(possibleFix);
+    }
+
+    public void removeFixFor(Tree tree) {
+      PossibleFix possibleFix = map.remove(tree);
+      list.remove(possibleFix);
+    }
+
+    public void fixByReplacing(Tree tree, String value) {
+      put(tree, new ReplacementFix(tree, value));
+    }
+
+    public void fixByDeleting(Tree tree) {
+      put(tree, new DeletionFix(tree));
+    }
+
+    public void fixByReplacing(int startPos, int endPos, String replaceWith) {
+      put(null, new PositionalReplacementFix(startPos, endPos, replaceWith));
+    }
+
+    public Fix getFix() {
+      for (PossibleFix possibleFix : list) {
+        possibleFix.applyFix(fixBuilder);
+      }
+
+      return fixBuilder.build();
+    }
+
+    abstract class PossibleFix {
+      protected final Tree tree;
+      protected final int startPosition;
+      protected final int endPosition;
+      private final Throwable trace;
+
+      PossibleFix(Tree tree) {
+        this.tree = tree;
+
+        DiagnosticPosition position = (DiagnosticPosition) tree;
+        this.startPosition = position.getStartPosition();
+        this.endPosition = position.getEndPosition(compilationUnit.endPositions);
+
+        this.trace = new RuntimeException();
+      }
+
+      PossibleFix(int startPosition, int endPosition) {
+        this.tree = null;
+
+        this.startPosition = startPosition;
+        this.endPosition = endPosition;
+
+        this.trace = new RuntimeException();
+      }
+
+      abstract void applyFix(Builder fixBuilder);
+
+      @Override
+      public String toString() {
+        return "PossibleFix{"
+            + "tree="
+            + tree
+            + ", startPosition="
+            + startPosition
+            + ", endPosition="
+            + endPosition
+            + '}';
+      }
+    }
+
+    class ReplacementFix extends PossibleFix {
+
+      private final String replaceWith;
+
+      public ReplacementFix(Tree tree, String replaceWith) {
+        super(tree);
+        this.replaceWith = replaceWith;
+      }
+
+      @Override
+      void applyFix(Builder fixBuilder) {
+        fixBuilder.replace(tree, replaceWith);
+      }
+
+      @Override
+      public String toString() {
+        return super.toString() + " [replace with " + replaceWith + "]";
+      }
+    }
+
+    private class PositionalReplacementFix extends PossibleFix {
+
+      private final String replaceWith;
+
+      public PositionalReplacementFix(int startPos, int endPos, String replaceWith) {
+        super(startPos, endPos);
+        this.replaceWith = replaceWith;
+      }
+
+      @Override
+      void applyFix(Builder fixBuilder) {
+        fixBuilder.replace(startPosition, endPosition, replaceWith);
+      }
+
+      @Override
+      public String toString() {
+        return super.toString() + " [replace with " + replaceWith + "]";
+      }
+    }
+
+    class DeletionFix extends PossibleFix {
+
+      public DeletionFix(Tree tree) {
+        super(tree);
+      }
+
+      @Override
+      void applyFix(Builder fixBuilder) {
+        fixBuilder.delete(tree);
+      }
+
+      @Override
+      public String toString() {
+        return super.toString() + " [delete]";
+      }
+    }
+  }
 }
diff --git a/errorprone/src/test/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheckTest.java b/errorprone/src/test/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheckTest.java
index 3adde59bf..977726846 100644
--- a/errorprone/src/test/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheckTest.java
+++ b/errorprone/src/test/java/org/robolectric/errorprone/bugpatterns/ShadowUsageCheckTest.java
@@ -496,7 +496,7 @@ public class ShadowUsageCheckTest {
             "  @Test void theTest() {",
             "    linearLayout = new LinearLayout(RuntimeEnvironment.application);",
             "    linearLayout.getLayoutAnimation().start();",
-            "    this.linearLayout.getLayoutAnimation().start();",
+            "    linearLayout.getLayoutAnimation().start();",
             "    shadowOf(linearLayout).getGravity();",
             "    shadowOf(this.linearLayout).getGravity();",
             "  }",
@@ -550,7 +550,7 @@ public class ShadowUsageCheckTest {
             "    linearLayout = new LinearLayout(RuntimeEnvironment.application);",
             "    linearLayout2 = new LinearLayout(RuntimeEnvironment.application);",
             "    linearLayout2.getLayoutAnimation().start();",
-            "    this.linearLayout2.getLayoutAnimation().start();",
+            "    linearLayout2.getLayoutAnimation().start();",
             "    shadowOf(linearLayout2).getGravity();",
             "    shadowOf(this.linearLayout2).getGravity();",
             "  }",