Fix ES6RewriteBlockScopedDeclaration to generate normalized FOR_IN loop initializer

rishipal · copybara-github · commit 501ae9683b3d · 2023-09-25T16:41:50.000-07:00
The pass currently generates var in the initializer part of FOR_IN loops. This makes the AST unnormalized. Fixing this to preserve normalization brings us closer to moving the pass post normalize.

PiperOrigin-RevId: 568357040
diff --git a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java
@@ -31,6 +31,7 @@
 import com.google.javascript.rhino.IR;
 import com.google.javascript.rhino.JSDocInfo;
 import com.google.javascript.rhino.Node;
+import com.google.javascript.rhino.Token;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.Map;
@@ -174,10 +175,42 @@ private void handleDeclarationList(Node declarationList, Node parent) {
       // (i.e. it would've become a separate var with an {@code /** const */} annotation).
       return;
     }
+
     if (parent.isVanillaFor()) {
-      handleDeclarationListInForInitializer(declarationList, parent);
+      /*
+       * This is needed to handle the case where we get:
+       *
+       * <ol>
+       *   <li>`for (let x = ...)` or
+       *   <li>`for (const x = ...`. <
+       * </ol>
+       *
+       * <p>Normalize only moves "var" outside the for initializer, it allows let/const within the
+       * initializer. So both #1 and #2 can exist regardless if this pass runs before and after
+       * normalize.
+       */
+      handleDeclarationListInVanillaForInitializer(declarationList, parent);
+      return;
+    }
+
+    if (parent.isForIn()) {
+      /*
+       * This is needed to handle the case where we get:
+       *
+       * <ol>
+       *   <li>`for (let x in ...)` or
+       *   <li>`for (const x in ...`. <
+       * </ol>
+       *
+       *
+       * <p>Normalize only moves "var" outside the for initializer, it allows let/const within the
+       * initializer. So both #1 and #2 can exist regardless if this pass runs before and after
+       * normalize.
+       */
+      handleDeclarationListInForInInitializer(declarationList, parent);
       return;
     }
+
     // convert all names to their own, separate (normalized) declarations
     while (declarationList.hasChildren()) {
       Node name = declarationList.getLastChild();
@@ -215,7 +248,74 @@ private void handleDeclarationList(Node declarationList, Node parent) {
     compiler.reportChangeToEnclosingScope(parent);
   }
 
-  private void handleDeclarationListInForInitializer(Node declarationList, Node parent) {
+  /**
+   * TODO: b/197349249 - We won't have any declaration lists in the FOR_IN initializer here when
+   * this pass runs post normalize. After that, all lets/consts can be directly converted to vars.
+   */
+  private void handleDeclarationListInForInInitializer(Node declarationList, Node parent) {
+    checkState(parent.isForIn());
+    Node first = declarationList;
+    Node lhs = first.getFirstChild();
+    Node loopNode = parent;
+    if (lhs.isDestructuringLhs()) {
+      // This pass relies on destructuring syntax being already removed. Hence we must not enter
+      // this case.
+      // TODO: b/279640656 Enable this code path once this pass runs unconditionally in stage3.
+      checkState(
+          false, "Destructuring syntax is unsupported in ES6RewriteBlockScopedDeclarations pass");
+      // Transform:
+      //    `for (let [a, b = 3] in c) {}` or `for (const [a, b = 3] in c) {}`
+      // to:
+      // <pre>
+      //    `var a; var b; for ([a, b = 3] in c) {}`
+      // </pre>
+      // respectively
+      NodeUtil.visitLhsNodesInNode(
+          lhs,
+          (name) -> {
+            // Add a declaration outside the for loop for the given name.
+            checkState(
+                name.isName(),
+                "lhs in destructuring declaration should be a simple name. (%s)",
+                name);
+            Node newName = IR.name(name.getString()).srcref(name);
+            Node newVar = IR.var(newName).srcref(name);
+            extractInlineJSDoc(declarationList, name, newVar);
+            // if the initializer name was a const, the newName must no longer be a const.
+            addNodeBeforeLoop(newVar, loopNode);
+          });
+
+      // Transform `for (var [a, b]... )` to `for ([a, b]...`
+      Node destructuringPattern = lhs.removeFirstChild();
+      first.replaceWith(destructuringPattern);
+    } else {
+      // Transform:
+      //    for (let a in b) {}
+      // to:
+      //    var a; for (a in b) {};
+      // and:
+      //    for (const a in b) {}
+      // to:
+      //    var a; for (a in b) {};
+      Node newStatement = first.cloneTree();
+      Node name = newStatement.getFirstChild().cloneNode();
+      // cloning also copies over any properties
+      if (name.getBooleanProp(Node.IS_CONSTANT_NAME)) {
+        // if the initializer name was a const, it must no longer be a const var.
+        name.putProp(Node.IS_CONSTANT_NAME, false);
+      }
+      newStatement.setToken(Token.VAR);
+      extractInlineJSDoc(first, first.getFirstChild(), newStatement);
+      first.replaceWith(name);
+      addNodeBeforeLoop(newStatement, loopNode);
+    }
+  }
+
+  /**
+   * TODO: b/197349249 - We won't have any declaration lists in the FOR initializer here when this
+   * pass runs post normalize. After that, all lets/consts can be directly converted to vars.
+   */
+  private void handleDeclarationListInVanillaForInitializer(Node declarationList, Node parent) {
     checkState(parent.isVanillaFor());
     // if the declarationList is in a FOR initializer, move it outside
     Node insertSpot = getInsertSpotBeforeLoop(parent);
@@ -224,13 +324,16 @@ private void handleDeclarationListInForInitializer(Node declarationList, Node pa
       Node name = declarationList.getLastChild();
       Node newDeclaration = IR.var(name.detach()).srcref(declarationList);
       extractInlineJSDoc(declarationList, name, newDeclaration);
-      maybeAddConstJSDoc(declarationList, name, newDeclaration);
+      if (name.getBooleanProp(Node.IS_CONSTANT_NAME)) {
+        // if the initializer name was a const, it must no longer be a const var after rewriting.
+        name.putProp(Node.IS_CONSTANT_NAME, false);
+      }
       // generate normalized var initializer (i.e. outside FOR)
       newDeclaration.insertBefore(insertSpot);
       insertSpot = newDeclaration;
       compiler.reportChangeToEnclosingScope(parent);
     }
-    // make FOR initializer empty
+    // make FOR initializer empty `for (; cond; incr)`
     Node empty = astFactory.createEmpty().srcref(declarationList);
     declarationList.replaceWith(empty);
     compiler.reportChangeToEnclosingScope(empty);
diff --git a/src/com/google/javascript/jscomp/Normalize.java b/src/com/google/javascript/jscomp/Normalize.java
@@ -552,11 +552,10 @@ private void extractForInitializer(Node n, @Nullable Node before, @Nullable Node
                 first.replaceWith(destructuringPattern);
               } else {
                 // Transform:
-                //    for (var a = 1 in b) {}
+                //    for (var a in b) {}
                 // to:
-                //    var a = 1; for (a in b) {};
+                //    var a; for (a in b) {};
                 Node newStatement = first;
-                // Clone just the node, to remove any initialization.
                 Node name = newStatement.getFirstChild().cloneNode();
                 first.replaceWith(name);
                 newStatement.insertBefore(insertBefore);
diff --git a/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java b/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationTest.java
@@ -394,7 +394,7 @@ public void testRenameConflict() {
   }
 
   @Test
-  public void testForLetInitializer_getsNormalized() {
+  public void testVanillaFor_letInitializer_getsNormalized() {
     Sources srcs =
         srcs(
             lines(
@@ -419,6 +419,34 @@ public void testForLetInitializer_getsNormalized() {
     test(srcs, expected);
   }
 
+  @Test
+  public void testVanillaFor_constInitializer_getsNormalized() {
+    Sources srcs =
+        srcs(
+            lines(
+                "function f() {",
+                "  return function foo() {",
+                // const initializer (allowed syntactically) can not be updated in the incr.
+                "     for (const i = 0; i < 10; /* no incr */) {",
+                "     }",
+                "  };",
+                "}"));
+
+    Expected expected =
+        expected(
+            lines(
+                "function f() {",
+                "  return function foo() {",
+                "     var i = 0;", // normalized (outside the for) and no @const annotation after
+                // rewriting
+                "     for (; i < 10; /* no incr */) {",
+                "     }",
+                "  };",
+                "}"));
+
+    test(srcs, expected);
+  }
+
   @Test
   public void testForLetInitializer_declarationList_getsNormalized() {
     Sources srcs =
@@ -518,7 +546,8 @@ public void testForLoop() {
     expected =
         expected(
             lines(
-                "for (var i in [0, 1]) {",
+                "var i;",
+                "for (i in [0, 1]) {",
                 "  var f = function() {",
                 "    var i$jscomp$1 = 0;",
                 "    if (true) {",
@@ -538,6 +567,10 @@ public void testForLoop() {
 
     test(
         lines(
+            // TODO: b/197349249  This unnormalized code would not reach this pass once this
+            // pass runs post normalize. The test must then be updated to the new "normalized"
+            // code form:
+            //  var x; for (x in y) { ...}
             "for (var x in y) {", //
             "  /** @type {number} */",
             "  let i;",
@@ -561,7 +594,8 @@ public void testForLoop() {
     expected =
         expected(
             lines(
-                "for (/** @const */ var i in [0, 1]) {",
+                "var i;", //
+                "for (i in [0, 1]) {",
                 "  var f = function() {",
                 "    var i$jscomp$1 = 0;",
                 "    if (true) {",
@@ -982,7 +1016,8 @@ public void testLoopClosureWithContinue() {
                 "/** @const */ var obj = {a: 1, b: 2, c: 3, skipMe: 4};",
                 "/** @const */ var arr = [];",
                 "var LOOP$0 = {};",
-                "for (/** @const */ var p in obj) {",
+                "var p",
+                "for (p in obj) {",
                 "  LOOP$0 = {",
                 "    p: LOOP$0.p",
                 "  };",
@@ -1577,7 +1612,8 @@ public void testForIn() {
             lines(
                 "/** @const */ var arr = [];",
                 "var LOOP$0 = {};",
-                "for (var i in [0, 1]) {",
+                "var i;",
+                "for (i in [0, 1]) {",
                 "  LOOP$0 = {",
                 "    i: LOOP$0.i",
                 "  };",
@@ -1594,6 +1630,10 @@ public void testForIn() {
                 "for (;;) {",
                 "  let a = getArray();",
                 "  f = function() {",
+                // TODO: b/197349249  This unnormalized code would not reach this pass once this
+                // pass runs post normalize. The test must then be updated to the new "normalized"
+                // code form:
+                //  var x; for (x in use(a)) { ...}
                 "    for (var x in use(a)) {",
                 "      f(a);",
                 "      a.push(x);",
@@ -1656,7 +1696,8 @@ public void testDoWhileForInCapturedLet() {
                 "      5: 5",
                 "    };",
                 "  var LOOP$0 = {};",
-                "  for (var i in obj) {",
+                "  var i;",
+                "  for (i in obj) {",
                 "    LOOP$0 = {",
                 "      i: LOOP$0.i",
                 "    };",
@@ -2102,7 +2143,8 @@ public void testUniqueIdAcrossMultipleFiles() {
     Expected expected =
         expected(
             lines(
-                "for (/** @const */ var i in [0, 1]) {",
+                "var i;",
+                "for (i in [0, 1]) {",
                 "  var f = function() {",
                 "    var i$jscomp$1 = 0;",
                 "    if (true) {",
@@ -2111,8 +2153,8 @@ public void testUniqueIdAcrossMultipleFiles() {
                 "  }",
                 "}"),
             lines(
-                "for (/** @const */",
-                "var b in[0, 1]) {",
+                "var b",
+                "for (b in[0, 1]) {",
                 "  var f$jscomp$1 = function() {",
                 "    var b$jscomp$1 = 0;",
                 "    if (true) {",
