clean up destructuring transformer and optimise array destructuring - closes #749

lib/babel/transformation/transformers/es6/destructuring.js

@@ -41,90 +41,114 @@ DestructuringTransformer.prototype.buildVariableDeclaration = function (id, init
   return declar;
 };
 
-DestructuringTransformer.prototype.push = function (elem, parentId) {
-  if (t.isObjectPattern(elem)) {
-    this.pushObjectPattern(elem, parentId);
-  } else if (t.isArrayPattern(elem)) {
-    this.pushArrayPattern(elem, parentId);
-  } else if (t.isAssignmentPattern(elem)) {
-    this.pushAssignmentPattern(elem, parentId);
+DestructuringTransformer.prototype.push = function (id, init) {
+  if (t.isObjectPattern(id)) {
+    this.pushObjectPattern(id, init);
+  } else if (t.isArrayPattern(id)) {
+    this.pushArrayPattern(id, init);
+  } else if (t.isAssignmentPattern(id)) {
+    this.pushAssignmentPattern(id, init);
   } else {
-    this.nodes.push(this.buildVariableAssignment(elem, parentId));
+    this.nodes.push(this.buildVariableAssignment(id, init));
   }
 };
 
-DestructuringTransformer.prototype.pushAssignmentPattern = function (pattern, parentId) {
-  var tempParentId = this.scope.generateUidBasedOnNode(parentId);
+DestructuringTransformer.prototype.get = function () {
+
+};
+
+DestructuringTransformer.prototype.pushAssignmentPattern = function (pattern, valueRef) {
+  // we need to assign the current value of the assignment to avoid evaluating
+  // it more than once
+
+  var tempValueRef = this.scope.generateUidBasedOnNode(valueRef);
 
   var declar = t.variableDeclaration("var", [
-    t.variableDeclarator(tempParentId, parentId)
+    t.variableDeclarator(tempValueRef, valueRef)
   ]);
   declar._blockHoist = this.blockHoist;
   this.nodes.push(declar);
 
+  //
+
   this.nodes.push(this.buildVariableAssignment(
     pattern.left,
     t.conditionalExpression(
-      t.binaryExpression("===", tempParentId, t.identifier("undefined")),
+      t.binaryExpression("===", tempValueRef, t.identifier("undefined")),
       pattern.right,
-      tempParentId
+      tempValueRef
     )
   ));
 };
 
-DestructuringTransformer.prototype.pushObjectSpread = function (pattern, parentId, prop, i) {
+DestructuringTransformer.prototype.pushObjectSpread = function (pattern, objRef, spreadProp, spreadPropIndex) {
   // get all the keys that appear in this object before the current spread
+
   var keys = [];
-  for (var i2 = 0; i2 < pattern.properties.length; i2++) {
-    var prop2 = pattern.properties[i2];
 
-    if (i2 >= i) break;
-    if (t.isSpreadProperty(prop2)) continue;
+  for (var i = 0; i < pattern.properties.length; i++) {
+    var prop = pattern.properties[i];
 
-    var key = prop2.key;
-    if (t.isIdentifier(key)) {
-      key = t.literal(prop2.key.name);
-    }
+    // we've exceeded the index of the spread property to all properties to the
+    // right need to be ignored
+    if (i >= spreadPropIndex) break;
+
+    // ignore other spread properties
+    if (t.isSpreadProperty(prop)) continue;
+
+    var key = prop.key;
+    if (t.isIdentifier(key)) key = t.literal(prop.key.name);
     keys.push(key);
   }
+
   keys = t.arrayExpression(keys);
 
-  var value = t.callExpression(this.file.addHelper("object-without-properties"), [parentId, keys]);
-  this.nodes.push(this.buildVariableAssignment(prop.argument, value));
+  //
+
+  var value = t.callExpression(this.file.addHelper("object-without-properties"), [objRef, keys]);
+  this.nodes.push(this.buildVariableAssignment(spreadProp.argument, value));
 };
 
-DestructuringTransformer.prototype.pushObjectProperty = function (prop, parentId) {
+DestructuringTransformer.prototype.pushObjectProperty = function (prop, propRef) {
   if (t.isLiteral(prop.key)) prop.computed = true;
 
-  var pattern2   = prop.value;
-  var patternId2 = t.memberExpression(parentId, prop.key, prop.computed);
+  var pattern = prop.value;
+  var objRef  = t.memberExpression(propRef, prop.key, prop.computed);
 
-  if (t.isPattern(pattern2)) {
-    this.push(pattern2, patternId2);
+  if (t.isPattern(pattern)) {
+    this.push(pattern, objRef);
   } else {
-    this.nodes.push(this.buildVariableAssignment(pattern2, patternId2));
+    this.nodes.push(this.buildVariableAssignment(pattern, objRef));
   }
 };
 
-DestructuringTransformer.prototype.pushObjectPattern = function (pattern, parentId) {
+DestructuringTransformer.prototype.pushObjectPattern = function (pattern, objRef) {
+  // https://github.com/babel/babel/issues/681
+
   if (!pattern.properties.length) {
     this.nodes.push(t.expressionStatement(
-      t.callExpression(this.file.addHelper("object-destructuring-empty"), [parentId])
+      t.callExpression(this.file.addHelper("object-destructuring-empty"), [objRef])
     ));
   }
 
-  if (pattern.properties.length > 1 && t.isMemberExpression(parentId)) {
-    var temp = this.scope.generateUidBasedOnNode(parentId, this.file);
-    this.nodes.push(this.buildVariableDeclaration(temp, parentId));
-    parentId = temp;
+  // if we have more than one properties in this pattern and the objectRef is a
+  // member expression then we need to assign it to a temporary variable so it's
+  // only evaluated once
+
+  if (pattern.properties.length > 1 && t.isMemberExpression(objRef)) {
+    var temp = this.scope.generateUidBasedOnNode(objRef, this.file);
+    this.nodes.push(this.buildVariableDeclaration(temp, objRef));
+    objRef = temp;
   }
 
+  //
+
   for (var i = 0; i < pattern.properties.length; i++) {
     var prop = pattern.properties[i];
     if (t.isSpreadProperty(prop)) {
-      this.pushObjectSpread(pattern, parentId, prop, i);
+      this.pushObjectSpread(pattern, objRef, prop, i);
     } else {
-      this.pushObjectProperty(prop, parentId);
+      this.pushObjectProperty(prop, objRef);
     }
   }
 };
@@ -138,19 +162,61 @@ var hasRest = function (pattern) {
   return false;
 };
 
-DestructuringTransformer.prototype.pushArrayPattern = function (pattern, parentId) {
+DestructuringTransformer.prototype.canUnpackArrayPattern = function (pattern, arr) {
+  if (!t.isArrayExpression(arr)) return false;
+  if (pattern.elements.length < arr.elements.length) return false;
+
+  for (var i = 0; i < pattern.elements.length; i++) {
+    var elem = pattern.elements[i];
+    if (!elem) return false; // hole
+    if (t.isRestElement(elem)) return false;
+  }
+
+  return true;
+};
+
+DestructuringTransformer.prototype.pushUnpackedArrayPattern = function (pattern, arr) {
+  for (var i = 0; i < pattern.elements.length; i++) {
+    this.push(pattern.elements[i], arr.elements[i]);
+  }
+};
+
+DestructuringTransformer.prototype.pushArrayPattern = function (pattern, arrayRef) {
   if (!pattern.elements) return;
 
-  // if we have a rest then we need all the elements
+  // optimise basic array destructuring of an array expression
+  //
+  // we can't do this to a pattern of unequal size to it's right hand
+  // array expression as then there will be values that wont be evaluated
+  //
+  // eg: var [a, b] = [1, 2];
+
+  if (this.canUnpackArrayPattern(pattern, arrayRef)) {
+    return this.pushUnpackedArrayPattern(pattern, arrayRef);
+  }
+
+  // if we have a rest then we need all the elements so don't tell
+  // `scope.toArray` to only get a certain amount
+
   var count = !hasRest(pattern) && pattern.elements.length;
 
-  var toArray = this.scope.toArray(parentId, count);
+  // so we need to ensure that the `arrayRef` is an array, `scope.toArray` will
+  // return a locally bound identifier if it's been inferred to be an array,
+  // otherwise it'll be a call to a helper that will ensure it's one
 
-  var _parentId = this.scope.generateUidBasedOnNode(parentId);
-  this.nodes.push(this.buildVariableDeclaration(_parentId, toArray));
-  parentId = _parentId;
+  var toArray = this.scope.toArray(arrayRef, count);
 
-  this.scope.assignTypeGeneric(parentId.name, "Array");
+  if (t.isIdentifier(toArray)) {
+    // we've been given an identifier so it must have been inferred to be an
+    // array
+    arrayRef = toArray;
+  } else {
+    arrayRef = this.scope.generateUidBasedOnNode(arrayRef);
+    this.nodes.push(this.buildVariableDeclaration(arrayRef, toArray));
+    this.scope.assignTypeGeneric(arrayRef.name, "Array");
+  }
+
+  //
 
   for (var i = 0; i < pattern.elements.length; i++) {
     var elem = pattern.elements[i];
@@ -158,32 +224,39 @@ DestructuringTransformer.prototype.pushArrayPattern = function (pattern, parentI
     // hole
     if (!elem) continue;
 
-    var newPatternId;
+    var elemRef;
 
     if (t.isRestElement(elem)) {
-      newPatternId = this.scope.toArray(parentId);
+      elemRef = this.scope.toArray(arrayRef);
 
       if (i > 0) {
-        newPatternId = t.callExpression(t.memberExpression(newPatternId, t.identifier("slice")), [t.literal(i)]);
+        elemRef = t.callExpression(t.memberExpression(elemRef, t.identifier("slice")), [t.literal(i)]);
       }
 
+      // set the element to the rest element argument since we've dealt with it
+      // being a rest already
       elem = elem.argument;
     } else {
-      newPatternId = t.memberExpression(parentId, t.literal(i), true);
+      elemRef = t.memberExpression(arrayRef, t.literal(i), true);
     }
 
-    this.push(elem, newPatternId);
+    this.push(elem, elemRef);
   }
 };
 
-DestructuringTransformer.prototype.init = function (pattern, parentId) {
-  if (!t.isArrayExpression(parentId) && !t.isMemberExpression(parentId) && !t.isIdentifier(parentId)) {
-    var key = this.scope.generateUidBasedOnNode(parentId);
-    this.nodes.push(this.buildVariableDeclaration(key, parentId));
-    parentId = key;
+DestructuringTransformer.prototype.init = function (pattern, ref) {
+  // trying to destructure a value that we can't evaluate more than once so we
+  // need to save it to a variable
+
+  if (!t.isArrayExpression(ref) && !t.isMemberExpression(ref) && !t.isIdentifier(ref)) {
+    var key = this.scope.generateUidBasedOnNode(ref);
+    this.nodes.push(this.buildVariableDeclaration(key, ref));
+    ref = key;
   }
 
-  this.push(pattern, parentId);
+  //
+
+  this.push(pattern, ref);
 };
 
 exports.ForInStatement =
@@ -244,7 +317,7 @@ exports.Function = function (node, parent, scope, file) {
     if (!t.isPattern(pattern)) return pattern;
 
     hasDestructuringTransformer = true;
-    var parentId = scope.generateUidIdentifier("ref");
+    var ref = scope.generateUidIdentifier("ref");
 
     var destructuring = new DestructuringTransformer({
       blockHoist: node.params.length - i,
@@ -253,9 +326,9 @@ exports.Function = function (node, parent, scope, file) {
       file:       file,
       kind:       "var",
     });
-    destructuring.init(pattern, parentId);
+    destructuring.init(pattern, ref);
 
-    return parentId;
+    return ref;
   });
 
   if (!hasDestructuringTransformer) return;

test/fixtures/transformation/es6-destructuring/array-unpack-optimisation/actual.js

@@ -0,0 +1,9 @@
+// opt
+var [a, b] = [1, 2];
+var [[a, b]] = [[1, 2]];
+
+// deopt
+var [a, b] = [1, 2, 3];
+var [[a, b]] = [[1, 2, 3]];
+var [a, b, ...items] = [1, 2, 3];
+var [[a, b, ...items]] = [[1, 2, 3]];

test/fixtures/transformation/es6-destructuring/array-unpack-optimisation/expected.js

@@ -0,0 +1,23 @@
+"use strict";
+
+// opt
+var a = 1;
+var b = 2;
+var a = 1;
+var b = 2;
+
+// deopt
+var _ref = [1, 2, 3];
+var a = _ref[0];
+var b = _ref[1];
+var _ref2 = [1, 2, 3];
+var a = _ref2[0];
+var b = _ref2[1];
+var _ref3 = [1, 2, 3];
+var a = _ref3[0];
+var b = _ref3[1];
+var items = _ref3.slice(2);
+var _ref4 = [1, 2, 3];
+var a = _ref4[0];
+var b = _ref4[1];
+var items = _ref4.slice(2);