Don't use args rest/spread to hoist super method calls (#9939)

* Don't use args rest/spread to hoist super method calls

* Hoist this in hoisted super method calls

* Make the code more readable

Review by edoardocavazza

packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/deeply-nested-asyncs/output.js

@@ -4,8 +4,8 @@ function s(_x) {
 
 function _s() {
   _s = babelHelpers.asyncToGenerator(function* (x) {
-    var _this = this,
+    var _arguments = arguments,
-        _arguments = arguments;
+        _this = this;
 
     for (var _len = arguments.length, args = new Array(_len > 1 ? _len - 1 : 0), _key = 1; _key < _len; _key++) {
       args[_key - 1] = arguments[_key];

packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/output.js

@@ -1,11 +1,12 @@
 class Foo extends class {} {
   method() {
-    var _superprop_callMethod = (..._args) => super.method(..._args);
+    var _superprop_getMethod = () => super.method,
+        _this = this;
 
     return babelHelpers.asyncToGenerator(function* () {
-      _superprop_callMethod();
+      _superprop_getMethod().call(_this);
 
-      var arrow = () => _superprop_callMethod();
+      var arrow = () => _superprop_getMethod().call(_this);
     })();
   }
 

packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/input.js

@@ -0,0 +1,7 @@
+class MyClass extends BaseClass {
+  async loadEntity() {
+    this.website = await this.loadWebsite();
+    this.report.setCompany(this.website.company);
+    super.loadEntity();
+  }
+}

packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/options.json

@@ -0,0 +1,16 @@
+{
+  "presets": [
+    [
+      "env",
+      {
+        "targets": [
+          "Chrome >= 60",
+          "Safari >= 11",
+          "iOS >= 10.3",
+          "Firefox >= 55",
+          "Edge >= 16"
+        ]
+      }
+    ]
+  ]
+}

packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/output.js

@@ -0,0 +1,19 @@
+function asyncGeneratorStep(gen, resolve, reject, _next, _throw, key, arg) { try { var info = gen[key](arg); var value = info.value; } catch (error) { reject(error); return; } if (info.done) { resolve(value); } else { Promise.resolve(value).then(_next, _throw); } }
+
+function _asyncToGenerator(fn) { return function () { var self = this, args = arguments; return new Promise(function (resolve, reject) { var gen = fn.apply(self, args); function _next(value) { asyncGeneratorStep(gen, resolve, reject, _next, _throw, "next", value); } function _throw(err) { asyncGeneratorStep(gen, resolve, reject, _next, _throw, "throw", err); } _next(undefined); }); }; }
+
+class MyClass extends BaseClass {
+  loadEntity() {
+    var _superprop_getLoadEntity = () => super.loadEntity,
+        _this = this;
+
+    return _asyncToGenerator(function* () {
+      _this.website = yield _this.loadWebsite();
+
+      _this.report.setCompany(_this.website.company);
+
+      _superprop_getLoadEntity().call(_this);
+    })();
+  }
+
+}

packages/babel-traverse/src/path/conversion.js

@@ -216,30 +216,6 @@ function hoistFunctionEnvironment(
     });
   }
 
-  // Convert all "this" references in the arrow to point at the alias.
-  let thisBinding;
-  if (thisPaths.length > 0 || specCompliant) {
-    thisBinding = getThisBinding(thisEnvFn, inConstructor);
-
-    if (
-      !specCompliant ||
-      // In subclass constructors, still need to rewrite because "this" can't be bound in spec mode
-      // because it might not have been initialized yet.
-      (inConstructor && hasSuperClass(thisEnvFn))
-    ) {
-      thisPaths.forEach(thisChild => {
-        const thisRef = thisChild.isJSX()
-          ? t.jsxIdentifier(thisBinding)
-          : t.identifier(thisBinding);
-
-        thisRef.loc = thisChild.node.loc;
-        thisChild.replaceWith(thisRef);
-      });
-
-      if (specCompliant) thisBinding = null;
-    }
-  }
-
   // Convert all "arguments" references in the arrow to point at the alias.
   if (argumentsPaths.length > 0) {
     const argumentsBinding = getBinding(thisEnvFn, "arguments", () =>
@@ -286,20 +262,12 @@ function hoistFunctionEnvironment(
         ? ""
         : superProp.get("property").node.name;
 
-      if (superProp.parentPath.isCallExpression({ callee: superProp.node })) {
-        const superBinding = getSuperPropCallBinding(thisEnvFn, key);
-
-        if (superProp.node.computed) {
-          const prop = superProp.get("property").node;
-          superProp.replaceWith(t.identifier(superBinding));
-          superProp.parentPath.node.arguments.unshift(prop);
-        } else {
-          superProp.replaceWith(t.identifier(superBinding));
-        }
-      } else {
       const isAssignment = superProp.parentPath.isAssignmentExpression({
         left: superProp.node,
       });
+      const isCall = superProp.parentPath.isCallExpression({
+        callee: superProp.node,
+      });
       const superBinding = getSuperPropBinding(thisEnvFn, isAssignment, key);
 
       const args = [];
@@ -310,18 +278,48 @@ function hoistFunctionEnvironment(
       if (isAssignment) {
         const value = superProp.parentPath.node.right;
         args.push(value);
-          superProp.parentPath.replaceWith(
-            t.callExpression(t.identifier(superBinding), args),
-          );
-        } else {
-          superProp.replaceWith(
-            t.callExpression(t.identifier(superBinding), args),
-          );
       }
+
+      const call = t.callExpression(t.identifier(superBinding), args);
+
+      if (isCall) {
+        superProp.parentPath.unshiftContainer("arguments", t.thisExpression());
+        superProp.replaceWith(t.memberExpression(call, t.identifier("call")));
+
+        thisPaths.push(superProp.parentPath.get("arguments.0"));
+      } else if (isAssignment) {
+        // Replace not only the super.prop, but the whole assignment
+        superProp.parentPath.replaceWith(call);
+      } else {
+        superProp.replaceWith(call);
       }
     });
   }
 
+  // Convert all "this" references in the arrow to point at the alias.
+  let thisBinding;
+  if (thisPaths.length > 0 || specCompliant) {
+    thisBinding = getThisBinding(thisEnvFn, inConstructor);
+
+    if (
+      !specCompliant ||
+      // In subclass constructors, still need to rewrite because "this" can't be bound in spec mode
+      // because it might not have been initialized yet.
+      (inConstructor && hasSuperClass(thisEnvFn))
+    ) {
+      thisPaths.forEach(thisChild => {
+        const thisRef = thisChild.isJSX()
+          ? t.jsxIdentifier(thisBinding)
+          : t.identifier(thisBinding);
+
+        thisRef.loc = thisChild.node.loc;
+        thisChild.replaceWith(thisRef);
+      });
+
+      if (specCompliant) thisBinding = null;
+    }
+  }
+
   return thisBinding;
 }
 
@@ -485,37 +483,6 @@ function getSuperBinding(thisEnvFn) {
   });
 }
 
-// Create a binding for a function that will call "super.foo()" or "super[foo]()".
-function getSuperPropCallBinding(thisEnvFn, propName) {
-  return getBinding(thisEnvFn, `superprop_call:${propName || ""}`, () => {
-    const argsBinding = thisEnvFn.scope.generateUidIdentifier("args");
-    const argsList = [t.restElement(argsBinding)];
-
-    let fnBody;
-    if (propName) {
-      // (...args) => super.foo(...args)
-      fnBody = t.callExpression(
-        t.memberExpression(t.super(), t.identifier(propName)),
-        [t.spreadElement(t.identifier(argsBinding.name))],
-      );
-    } else {
-      const method = thisEnvFn.scope.generateUidIdentifier("prop");
-      // (method, ...args) => super[method](...args)
-      argsList.unshift(method);
-      fnBody = t.callExpression(
-        t.memberExpression(
-          t.super(),
-          t.identifier(method.name),
-          true /* computed */,
-        ),
-        [t.spreadElement(t.identifier(argsBinding.name))],
-      );
-    }
-
-    return t.arrowFunctionExpression(argsList, fnBody);
-  });
-}
-
 // Create a binding for a function that will call "super.foo" or "super[foo]".
 function getSuperPropBinding(thisEnvFn, isAssignment, propName) {
   const op = isAssignment ? "set" : "get";

packages/babel-traverse/test/arrow-transform.js

@@ -539,7 +539,7 @@ describe("arrow function conversion", () => {
     );
   });
 
-  it("should convert super.prop() calls", () => {
+  it("should convert super.prop() calls without params", () => {
     assertConversion(
       `
       () => {
@@ -549,10 +549,11 @@ describe("arrow function conversion", () => {
       () => super.foo();
     `,
       `
-      var _superprop_callFoo = (..._args) => super.foo(..._args);
+      var _superprop_getFoo = () => super.foo,
+          _this = this;
 
       (function () {
-        _superprop_callFoo();
+        _superprop_getFoo().call(_this);
       });
       super.foo();
       () => super.foo();
@@ -560,6 +561,28 @@ describe("arrow function conversion", () => {
     );
   });
 
+  it("should convert super.prop() calls with params", () => {
+    assertConversion(
+      `
+      () => {
+        super.foo(a, b, ...c);
+      };
+      super.foo(a, b, ...c);
+      () => super.foo(a, b, ...c);
+    `,
+      `
+      var _superprop_getFoo = () => super.foo,
+          _this = this;
+
+      (function () {
+        _superprop_getFoo().call(_this, a, b, ...c);
+      });
+      super.foo(a, b, ...c);
+      () => super.foo(a, b, ...c);
+    `,
+    );
+  });
+
   it("should convert super[prop]() calls", () => {
     assertConversion(
       `
@@ -570,10 +593,11 @@ describe("arrow function conversion", () => {
       () => super[foo]();
     `,
       `
-      var _superprop_call = (_prop, ..._args) => super[_prop](..._args);
+      var _superprop_get = _prop => super[_prop],
+          _this = this;
 
       (function () {
-        _superprop_call(foo);
+        _superprop_get(foo).call(_this);
       });
       super[foo]();
       () => super[foo]();