From 99035ca96e4b0b22b68464b5c2800e90e3b65dff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Fri, 11 Oct 2019 10:18:54 +0200 Subject: [PATCH] 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 --- .../deeply-nested-asyncs/output.js | 4 +- .../object-method-with-super/output.js | 7 +- .../plugins-integration/issue-9935/input.js | 7 + .../issue-9935/options.json | 16 ++ .../plugins-integration/issue-9935/output.js | 19 +++ .../babel-traverse/src/path/conversion.js | 137 +++++++----------- .../babel-traverse/test/arrow-transform.js | 34 ++++- 7 files changed, 129 insertions(+), 95 deletions(-) create mode 100644 packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/input.js create mode 100644 packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/options.json create mode 100644 packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/output.js diff --git a/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/deeply-nested-asyncs/output.js b/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/deeply-nested-asyncs/output.js index a5914b25f7..c6437ce0bb 100644 --- a/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/deeply-nested-asyncs/output.js +++ b/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, - _arguments = arguments; + var _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]; diff --git a/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/output.js b/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/output.js index c8bccac252..6e26d643e0 100644 --- a/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/output.js +++ b/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); })(); } diff --git a/packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/input.js b/packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/input.js new file mode 100644 index 0000000000..af57902922 --- /dev/null +++ b/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(); + } +} diff --git a/packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/options.json b/packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/options.json new file mode 100644 index 0000000000..70f242c2ca --- /dev/null +++ b/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" + ] + } + ] + ] +} diff --git a/packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/output.js b/packages/babel-preset-env/test/fixtures/plugins-integration/issue-9935/output.js new file mode 100644 index 0000000000..c955d4e85a --- /dev/null +++ b/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); + })(); + } + +} diff --git a/packages/babel-traverse/src/path/conversion.js b/packages/babel-traverse/src/path/conversion.js index 99a2b00a73..fd14953544 100644 --- a/packages/babel-traverse/src/path/conversion.js +++ b/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,42 +262,64 @@ function hoistFunctionEnvironment( ? "" : superProp.get("property").node.name; - if (superProp.parentPath.isCallExpression({ callee: superProp.node })) { - const superBinding = getSuperPropCallBinding(thisEnvFn, key); + const isAssignment = superProp.parentPath.isAssignmentExpression({ + left: superProp.node, + }); + const isCall = superProp.parentPath.isCallExpression({ + callee: superProp.node, + }); + const superBinding = getSuperPropBinding(thisEnvFn, isAssignment, 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)); - } + const args = []; + if (superProp.node.computed) { + args.push(superProp.get("property").node); + } + + if (isAssignment) { + const value = superProp.parentPath.node.right; + args.push(value); + } + + 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 { - const isAssignment = superProp.parentPath.isAssignmentExpression({ - left: superProp.node, - }); - const superBinding = getSuperPropBinding(thisEnvFn, isAssignment, key); - - const args = []; - if (superProp.node.computed) { - args.push(superProp.get("property").node); - } - - 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), - ); - } + 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"; diff --git a/packages/babel-traverse/test/arrow-transform.js b/packages/babel-traverse/test/arrow-transform.js index f21ba52ac3..fdeeb4bd66 100644 --- a/packages/babel-traverse/test/arrow-transform.js +++ b/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]();