Use the Symbol.iterator method for array destructuring, rather than a…

…ssuming the object being destructured is an array or array-like object. This makes destructuring work correctly for all iterables (including strings and generator objects) rather than just array-likes. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=105254746
google · Oct 13, 2015 · 4f305b4 · 4f305b4
1 parent 38bd350
commit 4f305b4
Show file tree

Hide file tree

Showing 6 changed files with 135 additions and 87 deletions.
diff --git a/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java b/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java
@@ -15,6 +15,8 @@
  */
 package com.google.javascript.jscomp;
 
+import static com.google.javascript.jscomp.Es6ToEs3Converter.makeIterator;
+
 import com.google.common.base.Preconditions;
 import com.google.javascript.rhino.IR;
 import com.google.javascript.rhino.JSDocInfo;
@@ -279,42 +281,65 @@ private void visitArrayPattern(NodeTraversal t, Node arrayPattern, Node parent)
     }
 
     // Convert 'var [x, y] = rhs' to:
-    // var temp = rhs;
-    // var x = temp[0];
-    // var y = temp[1];
+    // var temp = $jscomp.makeIterator(rhs);
+    // var x = temp.next().value;
+    // var y = temp.next().value;
     String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
-    Node tempDecl = IR.var(IR.name(tempVarName), rhs.detachFromParent())
-        .useSourceInfoIfMissingFromForTree(arrayPattern);
+    Node tempDecl = IR.var(
+        IR.name(tempVarName),
+        makeIterator(compiler, rhs.detachFromParent()));
+    tempDecl.useSourceInfoIfMissingFromForTree(arrayPattern);
     nodeToDetach.getParent().addChildBefore(tempDecl, nodeToDetach);
 
-    int i = 0;
-    for (Node child = arrayPattern.getFirstChild(), next; child != null; child = next, i++) {
+    for (Node child = arrayPattern.getFirstChild(), next; child != null; child = next) {
       next = child.getNext();
       if (child.isEmpty()) {
+        // Just call the next() method to advance the iterator, but throw away the value.
+        Node nextCall = IR.exprResult(IR.call(IR.getprop(IR.name(tempVarName), IR.string("next"))));
+        nextCall.useSourceInfoIfMissingFromForTree(child);
+        nodeToDetach.getParent().addChildBefore(nextCall, nodeToDetach);
         continue;
       }
 
       Node newLHS, newRHS;
       if (child.isDefaultValue()) {
-        Node getElem = IR.getelem(IR.name(tempVarName), IR.number(i));
         //   [x = defaultValue] = rhs;
         // becomes
-        //   var temp = rhs;
-        //   x = (temp[0] === undefined) ? defaultValue : temp[0];
+        //   var temp0 = $jscomp.makeIterator(rhs);
+        //   var temp1 = temp.next().value
+        //   x = (temp1 === undefined) ? defaultValue : temp1;
+        String nextVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++);
+        Node var = IR.var(
+            IR.name(nextVarName),
+            IR.getprop(
+                IR.call(IR.getprop(IR.name(tempVarName), IR.string("next"))),
+                IR.string("value")));
+        var.useSourceInfoIfMissingFromForTree(child);
+        nodeToDetach.getParent().addChildBefore(var, nodeToDetach);
+
         newLHS = child.getFirstChild().detachFromParent();
-        newRHS = defaultValueHook(getElem, child.getLastChild().detachFromParent());
+        newRHS = defaultValueHook(IR.name(nextVarName), child.getLastChild().detachFromParent());
       } else if (child.isRest()) {
+        //   [...x] = rhs;
+        // becomes
+        //   var temp = $jscomp.makeIterator(rhs);
+        //   x = $jscomp.arrayFromIterator(temp);
         newLHS = child.detachFromParent();
         newLHS.setType(Token.NAME);
-        // [].slice.call(temp, i)
         newRHS =
             IR.call(
-                IR.getprop(IR.getprop(IR.arraylit(), IR.string("slice")), IR.string("call")),
-                IR.name(tempVarName),
-                IR.number(i));
+                NodeUtil.newQName(compiler, "$jscomp.arrayFromIterator"),
+                IR.name(tempVarName));
       } else {
+        // LHS is just a name (or a nested pattern).
+        //   var [x] = rhs;
+        // becomes
+        //   var temp = $jscomp.makeIterator(rhs);
+        //   var x = temp.next().value;
         newLHS = child.detachFromParent();
-        newRHS = IR.getelem(IR.name(tempVarName), IR.number(i));
+        newRHS = IR.getprop(
+            IR.call(IR.getprop(IR.name(tempVarName), IR.string("next"))),
+            IR.string("value"));
       }
       Node newNode;
       if (parent.isAssign()) {

diff --git a/src/com/google/javascript/jscomp/Es6RewriteGenerators.java b/src/com/google/javascript/jscomp/Es6RewriteGenerators.java
@@ -15,6 +15,8 @@
  */
 package com.google.javascript.jscomp;
 
+import static com.google.javascript.jscomp.Es6ToEs3Converter.makeIterator;
+
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Supplier;
@@ -185,9 +187,7 @@ private void visitYieldFor(Node n, Node parent) {
 
     Node generator = IR.var(
         IR.name(GENERATOR_YIELD_ALL_NAME),
-        IR.call(
-            NodeUtil.newQName(compiler, Es6ToEs3Converter.MAKE_ITER),
-            n.removeFirstChild()));
+        makeIterator(compiler, n.removeFirstChild()));
     Node entryDecl = IR.var(IR.name(GENERATOR_YIELD_ALL_ENTRY));
 
     Node assignIterResult = IR.assign(
@@ -1184,5 +1184,4 @@ public ExceptionContext(int catchStartCase, Node catchBlock) {
       this.catchBlock = catchBlock;
     }
   }
-
 }
diff --git a/src/com/google/javascript/jscomp/Es6ToEs3Converter.java b/src/com/google/javascript/jscomp/Es6ToEs3Converter.java
@@ -88,7 +88,6 @@ public final class Es6ToEs3Converter implements NodeTraversal.Callback, HotSwapC
 
   // These functions are defined in js/es6_runtime.js
   static final String INHERITS = "$jscomp.inherits";
-  static final String MAKE_ITER = "$jscomp.makeIterator";
 
   public Es6ToEs3Converter(AbstractCompiler compiler) {
     this.compiler = compiler;
@@ -258,13 +257,7 @@ private void visitForOf(Node node, Node parent) {
     }
     Node iterResult = IR.name(ITER_RESULT + variableName);
 
-    Node makeIter = IR.call(
-        NodeUtil.newQName(
-            compiler, MAKE_ITER),
-        iterable);
-    compiler.needsEs6Runtime = true;
-
-    Node init = IR.var(iterName.cloneTree(), makeIter);
+    Node init = IR.var(iterName.cloneTree(), makeIterator(compiler, iterable));
     Node initIterResult = iterResult.cloneTree();
     initIterResult.addChildToFront(getNext.cloneTree());
     init.addChildToBack(initIterResult);
@@ -899,6 +892,16 @@ private void cannotConvertYet(Node n, String feature) {
     compiler.report(JSError.make(n, CANNOT_CONVERT_YET, feature));
   }
 
+  /**
+   * Returns a call to $jscomp.makeIterator with {@code iterable} as its argument.
+   */
+  static Node makeIterator(AbstractCompiler compiler, Node iterable) {
+    compiler.needsEs6Runtime = true;
+    return IR.call(
+        NodeUtil.newQName(compiler, "$jscomp.makeIterator"),
+        iterable);
+  }
+
   /**
    * Represents static metadata on a class declaration expression - i.e. the qualified name that a
    * class declares (directly or by assignment), whether it's anonymous, and where transpiled code

diff --git a/src/com/google/javascript/jscomp/js/es6_runtime.js b/src/com/google/javascript/jscomp/js/es6_runtime.js
@@ -92,8 +92,9 @@ $jscomp.makeIterator = function(iterable) {
   if (iterable[Symbol.iterator]) {
     return iterable[Symbol.iterator]();
   }
-  if (!(iterable instanceof Array) && typeof iterable != 'string') {
-    throw new Error(iterable + ' is not iterable');
+  if (!(iterable instanceof Array) && typeof iterable != 'string' &&
+      !(iterable instanceof String)) {
+    throw new TypeError(iterable + ' is not iterable');
   }
   var index = 0;
   return /** @type {!Iterator} */ ({
@@ -111,18 +112,31 @@ $jscomp.makeIterator = function(iterable) {
 };
 
 /**
+ * Copies the values from an Iterable into an Array.
  * @param {string|!Array<T>|!Iterable<T>} iterable
  * @return {!Array<T>}
  * @template T
  */
 $jscomp.arrayFromIterable = function(iterable) {
   if (iterable instanceof Array) {
     return iterable;
+  } else {
+    return $jscomp.arrayFromIterator($jscomp.makeIterator(iterable));
   }
+};
+
 
-  var arr = [];
-  var iterator = $jscomp.makeIterator(iterable);
-  var i;
+/**
+ * Copies the values from an Iterator into an Array. The important difference
+ * between this and $jscomp.arrayFromIterable is that if the iterator's
+ * next() method has already been called one or more times, this method returns
+ * only the values that haven't been yielded yet.
+ * @param {!Iterator<T>} iterator
+ * @return {!Array<T>}
+ * @template T
+ */
+$jscomp.arrayFromIterator = function(iterator) {
+  var i, arr = [];
   while (!(i = iterator.next()).done) {
     arr.push(i.value);
   }

diff --git a/src/com/google/javascript/rhino/IR.java b/src/com/google/javascript/rhino/IR.java
@@ -390,8 +390,8 @@ public static Node getelem(Node target, Node elem) {
   }
 
   public static Node assign(Node target, Node expr) {
-    Preconditions.checkState(target.isValidAssignmentTarget());
-    Preconditions.checkState(mayBeExpression(expr));
+    Preconditions.checkState(target.isValidAssignmentTarget(), target);
+    Preconditions.checkState(mayBeExpression(expr), expr);
     return new Node(Token.ASSIGN, target, expr);
   }