Move es6RewriteRestAndSpread pass post normalization

PiperOrigin-RevId: 619204806

Author: rishipal

src/com/google/javascript/jscomp/AstFactory.java

@@ -570,6 +570,9 @@ Node createArgumentsAliasDeclaration(String aliasName) {
    * (e.g. because you are creating a new variable), you can use this method. However, if you've
    * just created the variable declaration, you could also just clone the {@code NAME} node from it
    * to create the new reference.
+   *
+   * <p>This method assumes that if the AST is normalized and the name starts with "$jscomp", then
+   * it must be a constant name.
    */
   Node createName(String name, Type type) {
     Node result = IR.name(name);

src/com/google/javascript/jscomp/Es6RewriteRestAndSpread.java

@@ -93,10 +93,12 @@ private void visitRestParam(NodeTraversal t, Node restParam, Node paramList) {
       return;
     }
 
+    // Now that the restParam is deleted, create a let declaration by making a new NAME node of the
+    // same name `paramName`
     Node let =
         astFactory
             .createSingleLetNameDeclaration(
-                paramName,
+                paramName, // creates a new NAME node with name `paramName`
                 astFactory.createCall(
                     astFactory.createGetPropWithUnknownType(
                         astFactory.createQName(this.namespace, "$jscomp.getRestArguments"),
@@ -105,24 +107,37 @@ private void visitRestParam(NodeTraversal t, Node restParam, Node paramList) {
                     astFactory.createNumber(restIndex),
                     astFactory.createArgumentsReference()))
             .srcrefTreeIfMissing(functionBody);
-    if (paramName.startsWith("$jscomp$destructuring$var")) {
-      // Currently this pass is before normalization. Hence, the createSingleLetNameDeclaration
-      // creates a non-const names. But the earlier Es6RewriteDesctructuring pass creates these
-      // names as const names.
-      // TODO(b/197349249): When this pass moves post normalization, stop explictly marking these
-      // names as const in this if-block.
-      checkState(
-          nameNode.getBooleanProp(Node.IS_CONSTANT_NAME),
-          "The $jscomp$destructuring$var[num] name must've been marked as const by the"
-              + " Es6RewriteDesctructuring pass");
-      let.getFirstChild().putBooleanProp(Node.IS_CONSTANT_NAME, true);
+    Node insertBeforePoint = getInsertBeforePoint(functionBody);
+    if (insertBeforePoint != null) {
+      let.insertBefore(insertBeforePoint);
+    } else {
+      // functionBody only contains hoisted function declarations
+      functionBody.addChildToBack(let);
     }
-    functionBody.addChildToFront(let);
     NodeUtil.addFeatureToScript(t.getCurrentScript(), Feature.LET_DECLARATIONS, compiler);
     compiler.ensureLibraryInjected("es6/util/restarguments", /* force= */ false);
     t.reportCodeChange();
   }
 
+  /**
+   * Gets an insertion point in the function body before which we want to insert the new let
+   * declaration. If there's not good insertion point (e.g. the function is empty or only contains
+   * inner function declarations), return null.
+   */
+  private Node getInsertBeforePoint(Node functionBody) {
+    checkState(functionBody.getParent().isFunction());
+    Node current = functionBody.getFirstChild();
+
+    // Do not insert the let declaration before any hoisted function declarations in this function
+    // body as those function declarations are hoisted by normalization. We must maintain
+    // normalization.
+    while (current != null && NodeUtil.isFunctionDeclaration(current)) {
+      current = current.getNext();
+    }
+
+    return current;
+  }
+
   /**
    * Processes array literals or calls to eliminate spreads.
    *

src/com/google/javascript/jscomp/TranspilationPasses.java

@@ -209,10 +209,6 @@ public static void addEarlyOptimizationTranspilationPasses(
     if (options.needsTranspilationOf(Feature.CLASSES)) {
       passes.maybeAdd(es6RewriteClass);
     }
-    if (options.needsTranspilationFrom(
-        FeatureSet.BARE_MINIMUM.with(Feature.REST_PARAMETERS, Feature.SPREAD_EXPRESSIONS))) {
-      passes.maybeAdd(es6RewriteRestAndSpread);
-    }
   }
 
   /**
@@ -226,6 +222,11 @@ public static void addPostNormalizationTranspilationPasses(
     // TODO(b/197349249): Move passes from `addEarlyOptimizationTranspilationPasses()` to here
     // until that method can be deleted as a no-op.
 
+    if (options.needsTranspilationFrom(
+        FeatureSet.BARE_MINIMUM.with(Feature.REST_PARAMETERS, Feature.SPREAD_EXPRESSIONS))) {
+      passes.maybeAdd(es6RewriteRestAndSpread);
+    }
+
     if (options.needsTranspilationFrom(
         FeatureSet.BARE_MINIMUM.with(
             Feature.COMPUTED_PROPERTIES, Feature.MEMBER_DECLARATIONS, Feature.TEMPLATE_LITERALS))) {

test/com/google/javascript/jscomp/Es6RewriteRestAndSpreadTest.java

@@ -569,6 +569,44 @@ public void testUsedRestParameterAtPositionTwo() {
     assertThat(getLastCompiler().getInjected()).containsExactly("es6/util/restarguments");
   }
 
+  @Test
+  public void testUsedRestParameterAtPositionTwo_maintainsNormalization() {
+    test(
+        "function f(zero, one, ...two) { function inner() {} return two; }",
+        lines(
+            "function f(zero, one) {",
+            "  function inner() {}", // stays hoisted
+            "  let two = $jscomp.getRestArguments.apply(2, arguments);",
+            "  return two;",
+            "}"));
+    assertThat(getLastCompiler().getInjected()).containsExactly("es6/util/restarguments");
+  }
+
+  @Test
+  public void testUsedRestParameterAtPositionTwo_maintainsNormalization_withoutReturn() {
+    test(
+        "function f(zero, one, ...two) { function inner() {} two; }",
+        lines(
+            "function f(zero, one) {",
+            "  function inner() {}", // stays hoisted
+            "  let two = $jscomp.getRestArguments.apply(2, arguments);",
+            "  two;",
+            "}"));
+    assertThat(getLastCompiler().getInjected()).containsExactly("es6/util/restarguments");
+  }
+
+  @Test
+  public void testUnusedRestParameterAtPositionTwo_noGoodInsertionPoint() {
+    test(
+        "function f(zero, one, ...two) { function inner() {} }",
+        lines(
+            "function f(zero, one) {",
+            "  function inner() {}", // stays hoisted
+            "  let two = $jscomp.getRestArguments.apply(2, arguments);", // declaration inserted
+            "}"));
+    assertThat(getLastCompiler().getInjected()).containsExactly("es6/util/restarguments");
+  }
+
   @Test
   public void testUnusedRestParameterAtPositionZeroWithTypingOnFunction() {
     test("/** @param {...number} zero */ function f(...zero) {}", "function f() {}");