optimise rest parameters in spread element position and allocate rest array at the earliest common ancestor of all references - fixes #1768

.eslintrc

@@ -17,7 +17,8 @@
     "no-fallthrough": 0,
     "new-cap": 0,
     "no-loop-func": 0,
-    "no-unreachable": 0
+    "no-unreachable": 0,
+    "no-labels": 0
   },
   "env": {
     "node": true

src/babel/transformation/transformers/es6/parameters.rest.js

@@ -9,27 +9,26 @@ var memberExpressionOptimisationVisitor = {
     }
   },
 
-  enter(node, parent, scope, state) {
+  Function(node, parent, scope, state) {
-    var stop = () => {
-      state.canOptimise = false;
-      this.stop();
-    };
-
-    if (this.isArrowFunctionExpression()) return stop();
-
     // skip over functions as whatever `arguments` we reference inside will refer
     // to the wrong function
-    if (this.isFunctionDeclaration() || this.isFunctionExpression()) {
     state.noOptimise = true;
     this.traverse(memberExpressionOptimisationVisitor, state);
     state.noOptimise = false;
-      return this.skip();
+    this.skip();
+  },
+
+  ReferencedIdentifier(node, parent, scope, state) {
+    // we can't guarantee the purity of arguments
+    if (node.name === "arguments") {
+      state.deopted = true;
     }
 
     // is this a referenced identifier and is it referencing the rest parameter?
-    if (!this.isReferencedIdentifier({ name: state.name })) return;
+    if (node.name !== state.name) return;
 
-    if (!state.noOptimise && t.isMemberExpression(parent) && parent.computed) {
+    if (!state.noOptimise) {
+      if (this.parentPath.isMemberExpression({ computed: true, object: node })) {
         // if we know that this member expression is referencing a number then we can safely
         // optimise it
         var prop = this.parentPath.get("property");
@@ -39,11 +38,24 @@ var memberExpressionOptimisationVisitor = {
         }
       }
 
-    stop();
+      // optimise single spread args in calls
+      if (this.parentPath.isSpreadElement() && state.offset === 0) {
+        var call = this.parentPath.parentPath;
+        if (call.isCallExpression() && call.node.arguments.length === 1) {
+          return state.argumentsNode;
+        }
+      }
+    }
+
+    if (state.noOptimise) {
+      state.deopted = true;
+    } else {
+      state.references.push(this);
+    }
   }
 };
 
-function optimizeMemberExpression(parent, offset) {
+function optimiseMemberExpression(parent, offset) {
   if (offset === 0) return;
 
   var newExpr;
@@ -86,25 +98,38 @@ export var visitor = {
       node.body.body.unshift(declar);
     }
 
-    // check if rest is used in member expressions and optimise for those cases
+    // check and optimise for extremely common cases
 
     var state = {
+      references: [],
+      offset:     node.params.length,
+
+      argumentsNode: argsId,
       outerBinding:  scope.getBindingIdentifier(rest.name),
-      canOptimise:  true,
+
+      // candidate member expressions we could optimise if there are no other references
       candidates: [],
-      method:       node,
+
-      name:         rest.name
+      // local rest binding name
+      name: rest.name,
+
+      // whether any references to the rest parameter were made in a function
+      deopted: false
     };
 
     this.traverse(memberExpressionOptimisationVisitor, state);
 
-    // we only have shorthands and there's no other references
+    if (!state.deopted && !state.references.length) {
-    if (state.canOptimise && state.candidates.length) {
+      // we only have shorthands and there are no other references
+      if (state.candidates.length) {
         for (var candidate of (state.candidates: Array)) {
           candidate.replaceWith(argsId);
-        optimizeMemberExpression(candidate.parent, node.params.length);
+          optimiseMemberExpression(candidate.parent, state.offset);
+        }
       }
       return;
+    } else {
+      state.references = state.references.concat(state.candidates);
     }
 
     //
@@ -144,6 +169,13 @@ export var visitor = {
       KEY:        key,
       LEN:        len
     });
+
+    if (!state.deopted) {
+      // perform allocation at the lowest common denominator of all references
+      this.getEarliestCommonAncestorFrom(state.references).getStatementParent().insertBefore(loop);
+      return;
+    }
+
     loop._blockHoist = node.params.length + 1;
     node.body.body.unshift(loop);
   }

src/babel/transformation/transformers/other/regenerator.js

@@ -38,9 +38,9 @@ function convertNodePath(path) {
       keysAlongPath.push(path.key);
 
       if (parentNode !== path.container) {
-        var found = Object.keys(parentNode).some(containerKey => {
+        var found = Object.keys(parentNode).some(listKey => {
-          if (parentNode[containerKey] === path.container) {
+          if (parentNode[listKey] === path.container) {
-            keysAlongPath.push(containerKey);
+            keysAlongPath.push(listKey);
             return true;
           }
         });

src/babel/traversal/context.js

@@ -26,19 +26,19 @@ export default class TraversalContext {
     return false;
   }
 
-  create(node, obj, key, containerKey) {
+  create(node, obj, key, listKey) {
     var path = NodePath.get({
       parentPath: this.parentPath,
       parent: node,
       container: obj,
       key: key,
-      containerKey: containerKey
+      listKey
     });
     path.unshiftContext(this);
     return path;
   }
 
-  visitMultiple(container, parent, containerKey) {
+  visitMultiple(container, parent, listKey) {
     // nothing to traverse!
     if (container.length === 0) return false;
 
@@ -51,7 +51,7 @@ export default class TraversalContext {
     for (let key = 0; key < container.length; key++) {
       var self = container[key];
       if (self && this.shouldVisit(self)) {
-        queue.push(this.create(parent, container, key, containerKey));
+        queue.push(this.create(parent, container, key, listKey));
       }
     }
 

src/babel/traversal/path/ancestry.js

@@ -1,3 +1,6 @@
+import * as t from "../../types";
+import NodePath from "./index";
+
 /**
  * Description
  */
@@ -28,16 +31,118 @@ export function getStatementParent() {
  * Description
  */
 
-export function getAncestry() {
+export function getEarliestCommonAncestorFrom(paths: Array<NodePath>): NodePath {
+  return this.getDeepestCommonAncestorFrom(paths, function (deepest, i, ancestries) {
+    var earliest;
+    var keys = t.VISITOR_KEYS[deepest.type];
+
+    for (var ancestry of (ancestries: Array)) {
+      var path = ancestry[i - 0];
+
+      if (!earliest) {
+        earliest = path;
+        continue;
+      }
+
+      // handle containers
+      if (path.listKey && earliest.listKey === path.listKey) {
+        // we're in the same container so check
+        if (path.key < earliest.key) {
+          earliest = path;
+          continue;
+        }
+      }
+
+      // handle keys
+      var earliestKeyIndex = keys.indexOf(earliest.parentKey);
+      var currentKeyIndex = keys.indexOf(path.parentKey);
+      if (earliestKeyIndex > currentKeyIndex) {
+        // key appears after
+        earliest = path;
+      }
+    }
+
+    return earliest;
+  });
+}
+
+/**
+ * Get the earliest path in the tree where the provided `paths` intersect.
+ *
+ * TODO: Possible optimisation target.
+ */
+
+export function getDeepestCommonAncestorFrom(paths: Array<NodePath>, filter?: Function): NodePath {
+  if (!paths.length) {
+    return this;
+  }
+
+  if (paths.length === 1) {
+    return paths[0];
+  }
+
+  // minimum depth of the tree so we know the highest node
+  var minDepth = Infinity;
+
+  // last common ancestor
+  var lastCommonIndex, lastCommon;
+
+  // get the ancestors of the path, breaking when the parent exceeds ourselves
+  var ancestries = paths.map((path) => {
     var ancestry = [];
 
-  var path = this.parentPath;
+    do {
-  while (path) {
+      ancestry.unshift(path);
-    ancestry.push(path.node);
+    } while((path = path.parentPath) && path !== this);
-    path = path.parentPath;
+
+    // save min depth to avoid going too far in
+    if (ancestry.length < minDepth) {
+      minDepth = ancestry.length;
     }
 
     return ancestry;
+  });
+
+  // get the first ancestry so we have a seed to assess all other ancestries with
+  var first = ancestries[0];
+
+  // check ancestor equality
+  depthLoop: for (var i = 0; i < minDepth; i++) {
+    var shouldMatch = first[i];
+
+    for (var ancestry of (ancestries: Array)) {
+      if (ancestry[i] !== shouldMatch) {
+        // we've hit a snag
+        break depthLoop;
+      }
+    }
+
+    lastCommonIndex = i;
+    lastCommon = shouldMatch;
+  }
+
+  if (lastCommon) {
+    if (filter) {
+      return filter(lastCommon, lastCommonIndex, ancestries);
+    } else {
+      return lastCommon;
+    }
+  } else {
+    throw new Error("Couldn't find intersection");
+  }
+}
+
+/**
+ * Description
+ */
+
+export function getAncestry() {
+  var path = this;
+  var paths = [];
+  do {
+    paths.push(path);
+  } while(path = path.parentPath);
+  return paths;
 }
 
 /**

src/babel/traversal/path/context.js

@@ -148,7 +148,7 @@ export function resync() {
   if (this.removed) return;
 
   this._resyncParent();
-  this._resyncContainer();
+  this._resyncList();
   this._resyncKey();
   //this._resyncRemoved();
 }
@@ -184,12 +184,12 @@ export function _resyncKey() {
   this.key = null;
 }
 
-export function _resyncContainer() {
+export function _resyncList() {
-  var containerKey = this.containerKey;
+  var listKey    = this.listKey;
   var parentPath = this.parentPath;
-  if (!containerKey || !parentPath) return;
+  if (!listKey || !parentPath) return;
 
-  var newContainer = parentPath.node[containerKey];
+  var newContainer = parentPath.node[listKey];
   if (this.container === newContainer) return;
 
   // container is out of sync. this is likely the result of it being reassigned
@@ -229,8 +229,9 @@ export function unshiftContext(context) {
  * Description
  */
 
-export function setup(parentPath, container, containerKey, key) {
+export function setup(parentPath, container, listKey, key) {
-  this.containerKey = containerKey;
+  this.listKey   = listKey;
+  this.parentKey = listKey || key;
   this.container = container;
 
   this.parentPath = parentPath || this.parentPath;

src/babel/traversal/path/family.js

@@ -71,7 +71,7 @@ export function getSibling(key) {
     parentPath: this.parentPath,
     parent: this.parent,
     container: this.container,
-    containerKey: this.containerKey,
+    listKey: this.listKey,
     key: key
   });
 }
@@ -101,7 +101,7 @@ export function _getKey(key) {
     // requested a container so give them all the paths
     return container.map((_, i) => {
       return NodePath.get({
-        containerKey: key,
+        listKey: key,
         parentPath: this,
         parent: node,
         container: container,

src/babel/traversal/path/index.js

@@ -20,11 +20,11 @@ export default class NodePath {
     this.parentPath         = null;
     this.context            = null;
     this.container          = null;
-    this.containerKey       = null;
+    this.listKey            = null;
+    this.parentKey          = null;
     this.key                = null;
     this.node               = null;
     this.scope              = null;
-    this.breakOnScopePaths  = null;
     this.type               = null;
     this.typeAnnotation     = null;
   }
@@ -33,7 +33,7 @@ export default class NodePath {
    * Description
    */
 
-  static get({ hub, parentPath, parent, container, containerKey, key }) {
+  static get({ hub, parentPath, parent, container, listKey, key }) {
     if (!hub && parentPath) {
       hub = parentPath.hub;
     }
@@ -55,7 +55,7 @@ export default class NodePath {
       paths.push(path);
     }
 
-    path.setup(parentPath, container, containerKey, key);
+    path.setup(parentPath, container, listKey, key);
 
     return path;
   }

src/babel/traversal/path/introspection.js

@@ -260,10 +260,10 @@ export function _guessExecutionStatusRelativeTo(target) {
     return "function";
   }
 
-  var targetPaths = getAncestry(target);
+  var targetPaths = target.getAncestry();
   //if (targetPaths.indexOf(this) >= 0) return "after";
 
-  var selfPaths   = getAncestry(this);
+  var selfPaths   = this.getAncestry();
 
   // get ancestor where the branches intersect
   var commonPath;
@@ -289,7 +289,7 @@ export function _guessExecutionStatusRelativeTo(target) {
   }
 
   // container list so let's see which one is after the other
-  if (targetRelationship.containerKey && targetRelationship.container === selfRelationship.container) {
+  if (targetRelationship.listKey && targetRelationship.container === selfRelationship.container) {
     return targetRelationship.key > selfRelationship.key ? "before" : "after";
   }
 
@@ -299,14 +299,6 @@ export function _guessExecutionStatusRelativeTo(target) {
   return targetKeyPosition > selfKeyPosition ? "before" : "after";
 }
 
-function getAncestry(path) {
-  var paths = [];
-  do {
-    paths.push(path);
-  } while(path = path.parentPath);
-  return paths;
-}
-
 /**
  * Resolve a "pointer" `NodePath` to it's absolute path.
  */

src/babel/traversal/path/lib/removal-hooks.js

@@ -53,7 +53,7 @@ export var post = [
 
     // var NODE;
     // remove an entire declaration if there are no declarators left
-    removeParent = removeParent || (self.containerKey === "declarations" && parent.isVariableDeclaration() && parent.node.declarations.length === 0);
+    removeParent = removeParent || (self.listKey === "declarations" && parent.isVariableDeclaration() && parent.node.declarations.length === 0);
 
     // NODE;
     // remove the entire expression statement if there's no expression

src/babel/traversal/path/modification.js

@@ -42,7 +42,7 @@ export function _containerInsert(from, nodes) {
     this.container.splice(to, 0, node);
 
     if (this.context) {
-      var path = this.context.create(this.parent, this.container, to, this.containerKey);
+      var path = this.context.create(this.parent, this.container, to, this.listKey);
       paths.push(path);
       this.queueNode(path);
     } else {
@@ -50,7 +50,7 @@ export function _containerInsert(from, nodes) {
         parentPath: this,
         parent: node,
         container: this.container,
-        containerKey: this.containerKey,
+        listKey: this.listKey,
         key: to
       }));
     }
@@ -154,7 +154,7 @@ export function _verifyNodeList(nodes) {
  * Description
  */
 
-export function unshiftContainer(containerKey, nodes) {
+export function unshiftContainer(listKey, nodes) {
   this._assertUnremoved();
 
   nodes = this._verifyNodeList(nodes);
@@ -162,12 +162,12 @@ export function unshiftContainer(containerKey, nodes) {
   // get the first path and insert our nodes before it, if it doesn't exist then it
   // doesn't matter, our nodes will be inserted anyway
 
-  var container = this.node[containerKey];
+  var container = this.node[listKey];
   var path      = NodePath.get({
     parentPath: this,
     parent: this.node,
     container: container,
-    containerKey,
+    listKey,
     key: 0
   });
 
@@ -178,7 +178,7 @@ export function unshiftContainer(containerKey, nodes) {
  * Description
  */
 
-export function pushContainer(containerKey, nodes) {
+export function pushContainer(listKey, nodes) {
   this._assertUnremoved();
 
   nodes = this._verifyNodeList(nodes);
@@ -186,13 +186,13 @@ export function pushContainer(containerKey, nodes) {
   // get an invisible path that represents the last node + 1 and replace it with our
   // nodes, effectively inlining it
 
-  var container = this.node[containerKey];
+  var container = this.node[listKey];
   var i         = container.length;
   var path      = NodePath.get({
     parentPath: this,
     parent: this.node,
     container: container,
-    containerKey,
+    listKey,
     key: i
   });
 

src/babel/traversal/scope/index.js

@@ -390,7 +390,8 @@ export default class Scope {
         var binding = scope.bindings[name];
         console.log(" -", name, {
           constant: binding.constant,
-          references: binding.references
+          references: binding.references,
+          kind: binding.kind
         });
       }
     } while(scope = scope.parent);

test/core/fixtures/transformation/es6.parameters.rest/arrow-functions/actual.js

@@ -12,7 +12,6 @@ var somefun = function () {
     let somefg = (c, d, e, f, ...args3) => {
       var _a = args3[0];
     };
-    var _c = arguments[1];
     var _d = args1[1];
   };
   let get1stArg = (...args) => args[0];

test/core/fixtures/transformation/es6.parameters.rest/arrow-functions/expected.js

@@ -6,22 +6,15 @@ var concat = function concat() {
 };
 
 var somefun = function somefun() {
-  var _arguments = arguments;
-
   var get2ndArg = function get2ndArg(a, b) {
-    for (var _len = arguments.length, args1 = Array(_len > 2 ? _len - 2 : 0), _key = 2; _key < _len; _key++) {
+    var _b = arguments[2];
-      args1[_key - 2] = arguments[_key];
-    }
-
-    var _b = args1[0];
     var somef = function somef(x, y, z) {
       var _a = arguments[3];
     };
     var somefg = function somefg(c, d, e, f) {
       var _a = arguments[4];
     };
-    var _c = _arguments[1];
+    var _d = arguments[3];
-    var _d = args1[1];
   };
   var get1stArg = function get1stArg() {
     return arguments[0];
@@ -29,8 +22,8 @@ var somefun = function somefun() {
 };
 
 function demo1() {
-  for (var _len2 = arguments.length, args = Array(_len2), _key2 = 0; _key2 < _len2; _key2++) {
+  for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
-    args[_key2] = arguments[_key2];
+    args[_key] = arguments[_key];
   }
 
   return function (i) {

test/core/fixtures/transformation/es6.parameters.rest/deepest-common-ancestor-earliest-child/actual.js

@@ -0,0 +1,32 @@
+// single referenes
+function r(...rest){
+  if (noNeedToWork) return 0;
+  return rest;
+}
+
+// multiple references
+function r(...rest){
+  if (noNeedToWork) return 0;
+
+  rest;
+  rest;
+}
+
+// multiple nested references
+function r(...rest){
+  if (noNeedToWork) return 0;
+
+  if (true) {
+    return rest;
+  } else {
+    return rest;
+  }
+}
+
+// nested reference with root reference
+function r(...rest){
+  if (noNeedToWork) return 0;
+
+  if (lol) rest;
+  rest;
+}

test/core/fixtures/transformation/es6.parameters.rest/deepest-common-ancestor-earliest-child/expected.js

@@ -0,0 +1,51 @@
+// single referenes
+"use strict";
+
+function r() {
+  if (noNeedToWork) return 0;
+
+  for (var _len = arguments.length, rest = Array(_len), _key = 0; _key < _len; _key++) {
+    rest[_key] = arguments[_key];
+  }
+
+  return rest;
+}
+
+// multiple references
+function r() {
+  if (noNeedToWork) return 0;
+
+  for (var _len2 = arguments.length, rest = Array(_len2), _key2 = 0; _key2 < _len2; _key2++) {
+    rest[_key2] = arguments[_key2];
+  }
+
+  rest;
+  rest;
+}
+
+// multiple nested references
+function r() {
+  if (noNeedToWork) return 0;
+
+  for (var _len3 = arguments.length, rest = Array(_len3), _key3 = 0; _key3 < _len3; _key3++) {
+    rest[_key3] = arguments[_key3];
+  }
+
+  if (true) {
+    return rest;
+  } else {
+    return rest;
+  }
+}
+
+// nested reference with root reference
+function r() {
+  if (noNeedToWork) return 0;
+
+  for (var _len4 = arguments.length, rest = Array(_len4), _key4 = 0; _key4 < _len4; _key4++) {
+    rest[_key4] = arguments[_key4];
+  }
+
+  if (lol) rest;
+  rest;
+}

test/core/fixtures/transformation/es6.parameters.rest/member-expression-deoptimisation/expected.js

@@ -47,10 +47,11 @@ var a = function a(foo) {
 };
 
 var b = function b(foo) {
+  var join = "join";
+
   for (var _len6 = arguments.length, bar = Array(_len6 > 1 ? _len6 - 1 : 0), _key6 = 1; _key6 < _len6; _key6++) {
     bar[_key6 - 1] = arguments[_key6];
   }
 
-  var join = "join";
   return bar[join];
 };

test/core/fixtures/transformation/es6.parameters.rest/spread-optimisation/actual.js

@@ -0,0 +1,15 @@
+// optimisation
+
+function foo(...bar) {
+  foo(...bar);
+}
+
+// deoptimisation
+
+function foo(a, ...b) {
+  foo(...b);
+}
+
+function foo(...b) {
+  foo(1, ...b);
+}

test/core/fixtures/transformation/es6.parameters.rest/spread-optimisation/expected.js

@@ -0,0 +1,25 @@
+// optimisation
+
+"use strict";
+
+function foo() {
+  foo.apply(undefined, arguments);
+}
+
+// deoptimisation
+
+function foo(a) {
+  for (var _len = arguments.length, b = Array(_len > 1 ? _len - 1 : 0), _key = 1; _key < _len; _key++) {
+    b[_key - 1] = arguments[_key];
+  }
+
+  foo.apply(undefined, b);
+}
+
+function foo() {
+  for (var _len2 = arguments.length, b = Array(_len2), _key2 = 0; _key2 < _len2; _key2++) {
+    b[_key2] = arguments[_key2];
+  }
+
+  foo.apply(undefined, [1].concat(b));
+}