Generate new vars only after function declarations to preserve normalization during rewriteArrowFunctions pass

PiperOrigin-RevId: 623378338

Author: rishipal

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

@@ -16,6 +16,7 @@
 package com.google.javascript.jscomp;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.javascript.jscomp.parsing.parser.FeatureSet;
@@ -126,7 +127,7 @@ private void addVarDeclarations(NodeTraversal t, ThisAndArgumentsContext context
       makeTreeNonIndexable(thisVar);
 
       if (context.lastSuperStatement == null) {
-        scopeBody.addChildToFront(thisVar);
+        insertVarDeclaration(thisVar, scopeBody);
       } else {
         // Not safe to reference `this` until after super() has been called.
         // TODO(bradfordcsmith): Some complex cases still aren't covered, like
@@ -140,13 +141,47 @@ private void addVarDeclarations(NodeTraversal t, ThisAndArgumentsContext context
       Node argumentsVar =
           astFactory.createArgumentsAliasDeclaration(ARGUMENTS_VAR + "$" + context.uniqueId);
       NodeUtil.addFeatureToScript(t.getCurrentScript(), Feature.CONST_DECLARATIONS, compiler);
-      scopeBody.addChildToFront(argumentsVar);
+
+      insertVarDeclaration(argumentsVar, scopeBody);
 
       argumentsVar.srcrefTreeIfMissing(scopeBody);
       compiler.reportChangeToEnclosingScope(argumentsVar);
     }
   }
 
+  private void insertVarDeclaration(Node varDeclaration, Node scopeBody) {
+    if (scopeBody.getParent().isFunction()) {
+      // for functions, we must find the correct insertion point to preserve normalization
+      Node insertBeforePoint = getInsertBeforePoint(scopeBody);
+      if (insertBeforePoint != null) {
+        varDeclaration.insertBefore(insertBeforePoint);
+      } else {
+        // functionBody only contains hoisted function declarations
+        scopeBody.addChildToBack(varDeclaration);
+      }
+    } else {
+      scopeBody.addChildToFront(varDeclaration);
+    }
+  }
+
+  /**
+   * 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;
+  }
+
   private void makeTreeNonIndexable(Node n) {
     n.makeNonIndexable();
     for (Node child = n.getFirstChild(); child != null; child = child.getNext()) {

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

@@ -381,6 +381,32 @@ public void testCapturingThisInMultipleArrowsInSameFunctionScope() {
             "})"));
   }
 
+  @Test
+  public void testGeneratedVariableDeclarations_placedAfterFunctionDeclarations() {
+    testArrowRewriting(
+        lines(
+            "({",
+            "  x: 0,",
+            "  y: 'a',",
+            "  f: function() {",
+            "    function foo() { this.x;}",
+            "    var a2 = () => this.y;",
+            "  },",
+            "})"),
+        lines(
+            "({",
+            "  x: 0,",
+            "  y: 'a',",
+            "  f: function() {",
+            // stays hoisted
+            "    function foo() { this.x; }",
+            // variable declarations placed after function declarations
+            "    const $jscomp$this$UID$1 = this;",
+            "    var a2 = function() { return $jscomp$this$UID$1.y; };",
+            "  },",
+            "})"));
+  }
+
   @Test
   public void testPassingMultipleArrowsInSameFreeScopeAsMethodParams() {
     testArrowRewriting(