Generalize VariableRenamer to rename variables due to name collisions that might be introduced by other passes.

PiperOrigin-RevId: 826489947

Author: rluble

translator/src/main/java/com/google/devtools/j2objc/ast/Name.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.j2objc.ast;
 
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import javax.lang.model.element.Element;
 import javax.lang.model.type.TypeMirror;
 
@@ -50,6 +51,7 @@ public Element getElement() {
     return element;
   }
 
+  @CanIgnoreReturnValue
   public Name setElement(Element newElement) {
     element = newElement;
     return this;

translator/src/main/java/com/google/devtools/j2objc/pipeline/TranslationProcessor.java

@@ -210,16 +210,18 @@ public static void applyMutations(
     new AbstractMethodRewriter(unit, deadCodeMap).run();
     ticker.tick("AbstractMethodRewriter");
 
-    new VariableRenamer(unit).run();
-    ticker.tick("VariableRenamer");
-
+    // Before: VariableRenamer - VariableRenamer renames variables in a scope-aware manner.
     new InstanceOfPatternRewriter(unit).run();
     ticker.tick("InstanceOfPatternRewriter");
 
+    // Before: VariableRenamer - VariableRenamer renames variables in a scope-aware manner.
     // Rewrite enhanced for loops into correct C code.
     new EnhancedForRewriter(unit).run();
     ticker.tick("EnhancedForRewriter");
 
+    new VariableRenamer(unit).run();
+    ticker.tick("VariableRenamer");
+
     // Before: Autoboxer - Must generate implementations so autoboxing can be applied to result.
     new LambdaRewriter(unit).run();
     ticker.tick("LambdaRewriter");

translator/src/main/java/com/google/devtools/j2objc/translate/EnhancedForRewriter.java

@@ -78,9 +78,7 @@ public void endVisit(EnhancedForStatement node) {
     } else if (loopVariable.asType().getKind().isPrimitive()) {
       boxLoopVariable(node, expressionType, loopVariable);
     } else {
-      VariableElement newLoopVariable = GeneratedVariableElement.mutableCopy(loopVariable)
-          .setTypeQualifiers("__strong");
-      node.getParameter().setVariableElement(newLoopVariable);
+      addStrongQualifierToLoopVariable(node);
     }
   }
 
@@ -233,4 +231,23 @@ private Block makeBlock(Statement stmt) {
     block.addStatement(stmt);
     return block;
   }
+
+  private void addStrongQualifierToLoopVariable(EnhancedForStatement node) {
+    VariableElement oldLoopVariable = node.getParameter().getVariableElement();
+    VariableElement newLoopVariable =
+        GeneratedVariableElement.mutableCopy(oldLoopVariable).setTypeQualifiers("__strong");
+    node.getParameter().setVariableElement(newLoopVariable);
+
+    // Replace variable references so that the VariableRenamer sees the link between the
+    // variable declarations and its references.
+    node.accept(
+        new UnitTreeVisitor(unit) {
+          @Override
+          public void endVisit(SimpleName node) {
+            if (oldLoopVariable == node.getElement()) {
+              node.setElement(newLoopVariable);
+            }
+          }
+        });
+  }
 }

translator/src/main/java/com/google/devtools/j2objc/translate/InstanceOfPatternRewriter.java

@@ -46,8 +46,6 @@
 public class InstanceOfPatternRewriter extends UnitTreeVisitor {
 
   private Deque<Block> enclosingScopes = new ArrayDeque<>();
-  private int tempCount;
-  private int patternCount;
 
   public InstanceOfPatternRewriter(CompilationUnit unit) {
     super(unit);
@@ -74,11 +72,6 @@ public void endVisit(InstanceofExpression node) {
 
     VariableElement patternVariable =
         ((Pattern.BindingPattern) node.getPattern()).getVariable().getVariableElement();
-    // TODO(b/454053746): Have a general pass that renames variables to avoid collisions.
-    // Create a unique name to avoid collisions.
-    nameTable.setVariableName(
-        patternVariable,
-        nameTable.getVariableShortName(patternVariable) + "$pattern$" + patternCount++);
     enclosingScopes.peek().addStatement(0, new VariableDeclarationStatement(patternVariable, null));
 
     Expression expression = node.getLeftOperand();
@@ -88,9 +81,7 @@ public void endVisit(InstanceofExpression node) {
       // that the expression doesn't have side effects and can be evaluated multiple times.
       VariableElement tempVariable =
           GeneratedVariableElement.newLocalVar(
-              "tmp$instanceof$" + tempCount++,
-              node.getLeftOperand().getTypeMirror(),
-              patternVariable.getEnclosingElement());
+              "tmp", node.getLeftOperand().getTypeMirror(), patternVariable.getEnclosingElement());
       enclosingScopes.peek().addStatement(0, new VariableDeclarationStatement(tempVariable, null));
       // tmp = expr
       replacement.addExpression(
@@ -99,7 +90,7 @@ public void endVisit(InstanceofExpression node) {
     }
 
     replacement.addExpressions(
-        // patternVariable = expr instanceof T ? (T) expr : null
+        // patternVariable = expression instanceof T ? (T) expression : null
         new Assignment(
             new SimpleName(patternVariable),
             new ConditionalExpression()

translator/src/main/java/com/google/devtools/j2objc/translate/VariableRenamer.java

@@ -14,21 +14,31 @@
 
 package com.google.devtools.j2objc.translate;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.j2objc.ast.AnnotationTypeDeclaration;
+import com.google.devtools.j2objc.ast.Block;
+import com.google.devtools.j2objc.ast.CatchClause;
 import com.google.devtools.j2objc.ast.CompilationUnit;
+import com.google.devtools.j2objc.ast.EnhancedForStatement;
 import com.google.devtools.j2objc.ast.EnumDeclaration;
+import com.google.devtools.j2objc.ast.ForStatement;
 import com.google.devtools.j2objc.ast.LambdaExpression;
+import com.google.devtools.j2objc.ast.MethodDeclaration;
 import com.google.devtools.j2objc.ast.RecordDeclaration;
 import com.google.devtools.j2objc.ast.SimpleName;
+import com.google.devtools.j2objc.ast.SingleVariableDeclaration;
 import com.google.devtools.j2objc.ast.TreeUtil;
 import com.google.devtools.j2objc.ast.TypeDeclaration;
 import com.google.devtools.j2objc.ast.UnitTreeVisitor;
+import com.google.devtools.j2objc.ast.VariableDeclarationFragment;
 import com.google.devtools.j2objc.util.ElementUtil;
 import com.google.devtools.j2objc.util.ErrorUtil;
-import java.util.ArrayList;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Set;
+import javax.lang.model.element.Element;
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.VariableElement;
@@ -41,8 +51,37 @@
 @SuppressWarnings("UngroupedOverloads")
 public class VariableRenamer extends UnitTreeVisitor {
 
-  private List<Set<String>> fieldNameStack = new ArrayList<>();
-  private Set<TypeElement> renamedTypes = new HashSet<>();
+  private final Deque<Set<String>> fieldNameStack = new ArrayDeque<>();
+  private final Set<TypeElement> renamedTypes = new HashSet<>();
+  // Keep track of the variables and names in a scope so we can rename variables in a
+  // scope-aware manner; start with a sentinel empty scope.
+  private final Deque<Scope> scopes = new ArrayDeque<>(ImmutableList.of(new Scope()));
+
+  // Keep track of variables that are in scope and the names that have already been used.
+  private static class Scope {
+    private final Set<VariableElement> variables = new HashSet<>();
+    private final Set<String> usedNames = new HashSet<>();
+
+    private Scope(Scope enclosingScope) {
+      this.variables.addAll(enclosingScope.variables);
+      this.usedNames.addAll(enclosingScope.usedNames);
+    }
+
+    private Scope() {}
+
+    /**
+     * Called when a variable reference has been seen.
+     *
+     * @return true if its the variable needs renaming, false if it is seen for the first time.
+     */
+    private boolean needsRenaming(VariableElement variable) {
+      if (!variables.add(variable)) {
+        // Variable has already been seen in the scope so its name was already resolved.
+        return false;
+      }
+      return !usedNames.add(ElementUtil.getName(variable));
+    }
+  }
 
   public VariableRenamer(CompilationUnit unit) {
     super(unit);
@@ -102,11 +141,28 @@ private void pushType(TypeElement type) {
     for (VariableElement field : fields) {
       fullFieldNames.add(nameTable.getVariableShortName(field));
     }
-    fieldNameStack.add(fullFieldNames);
+    fieldNameStack.push(fullFieldNames);
+    // A nested type defines a new nested scope. The used names from the enclosing scope are kept
+    // since the nested type can reference variables from the enclosing scope.
+    Scope newScope = enterVariableScope();
+    // Mark all the field names as unavailable for renaming.
+    newScope.usedNames.addAll(fullFieldNames);
   }
 
   private void popType() {
-    fieldNameStack.remove(fieldNameStack.size() - 1);
+    fieldNameStack.pop();
+    scopes.pop();
+  }
+
+  @CanIgnoreReturnValue
+  private Scope enterVariableScope() {
+    Scope newScope = new Scope(scopes.peek());
+    scopes.push(newScope);
+    return newScope;
+  }
+
+  private void exitVariableScope() {
+    scopes.pop();
   }
 
   @Override
@@ -119,13 +175,8 @@ public void endVisit(SimpleName node) {
       // Make sure fields for the declaring type are renamed.
       collectAndRenameFields(ElementUtil.getDeclaringClass(var), new HashSet<VariableElement>());
     } else {
-      // Local variable or parameter. Rename if it shares a name with a field.
-      String varName = ElementUtil.getName(var);
-      assert fieldNameStack.size() > 0;
-      Set<String> fieldNames = fieldNameStack.get(fieldNameStack.size() - 1);
-      if (fieldNames.contains(varName)) {
-        nameTable.setVariableName(var, varName + "Arg");
-      }
+      // Local variable or parameter.
+      handleVariable(var);
     }
   }
 
@@ -183,4 +234,98 @@ public boolean visit(RecordDeclaration node) {
   public void endVisit(RecordDeclaration node) {
     popType();
   }
+
+  @Override
+  public boolean visit(MethodDeclaration node) {
+    enterVariableScope();
+    return true;
+  }
+
+  @Override
+  public void endVisit(MethodDeclaration node) {
+    exitVariableScope();
+  }
+
+  @Override
+  public boolean visit(Block node) {
+    enterVariableScope();
+    return true;
+  }
+
+  @Override
+  public void endVisit(Block node) {
+    exitVariableScope();
+  }
+
+  @Override
+  public boolean visit(ForStatement node) {
+    enterVariableScope();
+    return true;
+  }
+
+  @Override
+  public void endVisit(ForStatement node) {
+    exitVariableScope();
+  }
+
+  @Override
+  public boolean visit(EnhancedForStatement node) {
+    enterVariableScope();
+    return true;
+  }
+
+  @Override
+  public void endVisit(EnhancedForStatement node) {
+    exitVariableScope();
+  }
+
+  @Override
+  public boolean visit(CatchClause node) {
+    enterVariableScope();
+    return true;
+  }
+
+  @Override
+  public void endVisit(CatchClause node) {
+    exitVariableScope();
+  }
+
+  @Override
+  public boolean visit(SingleVariableDeclaration variableDeclaration) {
+    handleVariable(variableDeclaration.getVariableElement());
+    return true;
+  }
+
+  @Override
+  public boolean visit(VariableDeclarationFragment variableDeclaration) {
+    handleVariable(variableDeclaration.getVariableElement());
+    return true;
+  }
+
+  private void handleVariable(VariableElement variable) {
+    if (variable.getKind().isField()) {
+      return;
+    }
+    Scope scope = scopes.peek();
+    if (scope.needsRenaming(variable)) {
+      // Variable needs renaming. Preserve the logic that renames parameters with "Arg" suffix.
+      String baseName = ElementUtil.getName(variable) + (isParameter(variable) ? "Arg" : "");
+      String suffix = "";
+      int count = 1;
+      while (scope.usedNames.contains(baseName + suffix)) {
+        suffix = "_" + count++;
+      }
+      String newName = baseName + suffix;
+      nameTable.setVariableName(variable, newName);
+      scope.usedNames.add(newName);
+    }
+  }
+
+  private boolean isParameter(VariableElement variable) {
+    Element element = variable.getEnclosingElement();
+    if (element instanceof ExecutableElement executableElement) {
+      return executableElement.getParameters().contains(variable);
+    }
+    return false;
+  }
 }

translator/src/test/java/com/google/devtools/j2objc/gen/StatementGeneratorTest.java

@@ -1996,10 +1996,10 @@ public void testInstanceOfPatternVariableTranslation() throws IOException {
             "Test.m");
     assertTranslatedLines(
         translation,
-        "NSString *s$pattern$0;",
-        "if ((s$pattern$0 = [o isKindOfClass:[NSString class]] ? (NSString *) o"
-            + " : nil, !JreStringEqualsEquals(s$pattern$0, nil))) {",
-        "return [s$pattern$0 java_length];",
+        "NSString *s;",
+        "if ((s = [o isKindOfClass:[NSString class]] ? (NSString *) o"
+            + " : nil, !JreStringEqualsEquals(s, nil))) {",
+        "return [s java_length];",
         "}",
         "return 0;");
   }
@@ -2027,10 +2027,10 @@ public void testInstanceOfPatternVariableTranslationWithGuards() throws IOExcept
             "Test.m");
     assertTranslatedLines(
         translation,
-        "Point *p$pattern$0;",
-        "if ((p$pattern$0 = [o isKindOfClass:[Point class]] ? (Point *) o : nil,"
-            + " !JreObjectEqualsEquals(p$pattern$0, nil)) && x_ == p$pattern$0->x_ && y_ =="
-            + " p$pattern$0->y_) {",
+        "Point *p;",
+        "if ((p = [o isKindOfClass:[Point class]] ? (Point *) o : nil,"
+            + " !JreObjectEqualsEquals(p, nil)) && x_ == p->x_ && y_ =="
+            + " p->y_) {",
         "  return true;",
         "}",
         "else {",

translator/src/test/java/com/google/devtools/j2objc/translate/InnerClassExtractorTest.java

@@ -792,8 +792,9 @@ public void testInnerClassWithVarargsAndCaptureVariables() throws IOException {
     assertTranslation(translation,
         "create_Test_1Inner_initWithInt_withNSObjectArray_(i, "
         + "[IOSObjectArray arrayWithObjects:(id[]){ o } count:1 type:NSObject_class_()])");
-    assertTranslation(translation,
+    assertTranslation(
+        translation,
         "void Test_1Inner_initWithInt_withNSObjectArray_("
-        + "Test_1Inner *self, int32_t capture$0, IOSObjectArray *o)");
+            + "Test_1Inner *self, int32_t capture$0, IOSObjectArray *oArg)");
   }
 }