Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Throw error when compiling super() in arrow functions with default / rest parameters #15163

Merged
merged 4 commits into from Nov 22, 2022
Merged

fix: Throw error when compiling super() in arrow functions with default / rest parameters #15163

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
073b3b0
Fix: Throw when super in arrow with default / rest
SuperSodaSea Nov 8, 2022
c66111b
Add allowInsertArrowWithRest and improve throw
SuperSodaSea Nov 21, 2022
db066ab
Merge branch 'main' into fix/15148
SuperSodaSea Nov 21, 2022
fd549b3
Update packages/babel-traverse/src/path/conversion.ts
SuperSodaSea Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .../babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/super-call/input.js
View file
Open in desktop
@@ -0,0 +1,6 @@
class Bar extends Foo {
constructor() {
let f = () => super();
f();
}
}
4 changes: 4 additions & 0 deletions ...el-plugin-transform-arrow-functions/test/fixtures/arrow-functions/super-call/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
"plugins": ["transform-arrow-functions"],
"throws": "When using '@babel/plugin-transform-arrow-functions', it's not possible to compile `super()` in an arrow function without compiling classes.\nPlease add '@babel/plugin-transform-classes' to your Babel configuration."
}
6 changes: 6 additions & 0 deletions .../babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/super-prop/input.js
View file
Open in desktop
@@ -0,0 +1,6 @@
class Bar extends Foo {
constructor() {
let f = () => super.x;
f();
}
}
4 changes: 4 additions & 0 deletions ...el-plugin-transform-arrow-functions/test/fixtures/arrow-functions/super-prop/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
"plugins": ["transform-arrow-functions"],
"throws": "When using '@babel/plugin-transform-arrow-functions', it's not possible to compile `super.prop` in an arrow function without compiling classes.\nPlease add '@babel/plugin-transform-classes' to your Babel configuration."
}
5 changes: 4 additions & 1 deletion packages/babel-plugin-transform-parameters/src/index.ts
View file
Open in desktop
Expand Up @@ -27,7 +27,10 @@ export default declare((api, options: Options) => {
.some(param => param.isRestElement() || param.isAssignmentPattern())
) {
// default/rest visitors require access to `arguments`, so it cannot be an arrow
path.arrowFunctionToExpression({ noNewArrows });
path.arrowFunctionToExpression({
allowInsertArrowWithRest: false,
noNewArrows,
});

// In some cases arrowFunctionToExpression replaces the function with a wrapper.
// Return early; the wrapped function will be visited later in the AST traversal.
Expand Down
6 changes: 6 additions & 0 deletions ...nsform-parameters/test/fixtures/parameters/arrow-default-super-no-classes-plugin/input.js
View file
Open in desktop
@@ -0,0 +1,6 @@
class Bar extends Foo {
constructor() {
let f = (x = super()) => x;
f();
}
}
4 changes: 4 additions & 0 deletions ...rm-parameters/test/fixtures/parameters/arrow-default-super-no-classes-plugin/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
"plugins": ["transform-parameters"],
"throws": "When using '@babel/plugin-transform-parameters', it's not possible to compile `super()` in an arrow function with default or rest parameters without compiling classes.\nPlease add '@babel/plugin-transform-classes' to your Babel configuration."
}
6 changes: 6 additions & 0 deletions ...transform-parameters/test/fixtures/parameters/arrow-rest-super-no-classes-plugin/input.js
View file
Open in desktop
@@ -0,0 +1,6 @@
class Bar extends Foo {
constructor() {
let f = (...args) => super(...args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the input here is very similar to how we would transform super():

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

Lines 53 to 71 in 9862d42

assertConversion(
`
() => {
super(345);
};
super();
() => super();
`,
`
var _supercall = (..._args) => super(..._args);
(function () {
_supercall(345);
});
_supercall();
() => _supercall();
`,
{ methodName: "constructor", extend: true },
);

My gut is that it should have been fixed in how we generate the super call:

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

Line 652 in 9862d42

function getSuperBinding(thisEnvFn: NodePath<t.Function>) {

We already have an arrow function check when constructing supercall,

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

Lines 287 to 291 in 9862d42

if (!allowInsertArrow) {
throw superCalls[0].buildCodeFrameError(
"Unable to handle nested super() usage in arrow",
);
}

somehow it does not throw, probably because allowInsertArrow is not properly passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can add allowInsertArrow: false here?

babel/packages/babel-plugin-transform-parameters/src/index.ts

Lines 48 to 49 in 073b3b0

// default/rest visitors require access to `arguments`, so it cannot be an arrow
path.arrowFunctionToExpression({ noNewArrows });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to add allowInsertArrow: false and it fails on #13939 related tests...:thinking:
Maybe we need something like allowInsertArrowWithRest?

    × 13939
    × 13939 complex
    × 13939 private
    × 13939 private complex

SyntaxError: babel/packages/babel-plugin-transform-parameters/test/fixtures/regression/13939/input.js: Unable to transform arrow inside class property
      1 | class A {
    > 2 |   handle = (x = 0) => {
        |            ^
      3 |     console.log(x);
      4 |   };
      5 | }

...

f();
}
}
4 changes: 4 additions & 0 deletions ...sform-parameters/test/fixtures/parameters/arrow-rest-super-no-classes-plugin/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
"plugins": ["transform-parameters"],
"throws": "When using '@babel/plugin-transform-parameters', it's not possible to compile `super()` in an arrow function with default or rest parameters without compiling classes.\nPlease add '@babel/plugin-transform-classes' to your Babel configuration."
}
20 changes: 18 additions & 2 deletions packages/babel-traverse/src/path/conversion.ts
View file
Open in desktop
Expand Up @@ -149,12 +149,14 @@ export function arrowFunctionToExpression(
this: NodePath<t.ArrowFunctionExpression>,
{
allowInsertArrow = true,
allowInsertArrowWithRest = allowInsertArrow,
/** @deprecated Use `noNewArrows` instead */
specCompliant = false,
// TODO(Babel 8): Consider defaulting to `false` for spec compliancy
noNewArrows = !specCompliant,
}: {
allowInsertArrow?: boolean | void;
allowInsertArrowWithRest?: boolean | void;
specCompliant?: boolean | void;
noNewArrows?: boolean;
} = {},
Expand All @@ -169,6 +171,7 @@ export function arrowFunctionToExpression(
this,
noNewArrows,
allowInsertArrow,
allowInsertArrowWithRest,
);

// @ts-expect-error TS requires explicit fn type annotation
Expand Down Expand Up @@ -240,6 +243,7 @@ function hoistFunctionEnvironment(
// TODO(Babel 8): Consider defaulting to `false` for spec compliancy
noNewArrows: boolean | void = true,
allowInsertArrow: boolean | void = true,
allowInsertArrowWithRest: boolean | void = true,
): { thisBinding: string; fnPath: NodePath<t.Function> } {
let arrowParent;
let thisEnvFn: NodePath<t.Function> = fnPath.findParent(p => {
Expand Down Expand Up @@ -286,7 +290,17 @@ function hoistFunctionEnvironment(
if (inConstructor && superCalls.length > 0) {
if (!allowInsertArrow) {
throw superCalls[0].buildCodeFrameError(
"Unable to handle nested super() usage in arrow",
"When using '@babel/plugin-transform-arrow-functions', " +
"it's not possible to compile `super()` in an arrow function without compiling classes.\n" +
"Please add '@babel/plugin-transform-classes' to your Babel configuration.",
);
}
if (!allowInsertArrowWithRest) {
// NOTE: This may happen in `@babel/preset-env` with target `since 2017`, or in the default setting of `create-react-app`.
SuperSodaSea marked this conversation as resolved.
Show resolved Hide resolved
throw superCalls[0].buildCodeFrameError(
"When using '@babel/plugin-transform-parameters', " +
"it's not possible to compile `super()` in an arrow function with default or rest parameters without compiling classes.\n" +
"Please add '@babel/plugin-transform-classes' to your Babel configuration.",
);
}
const allSuperCalls: NodePath<t.CallExpression>[] = [];
Expand Down Expand Up @@ -345,7 +359,9 @@ function hoistFunctionEnvironment(
if (superProps.length > 0) {
if (!allowInsertArrow) {
throw superProps[0].buildCodeFrameError(
"Unable to handle nested super.prop usage",
"When using '@babel/plugin-transform-arrow-functions', " +
"it's not possible to compile `super.prop` in an arrow function without compiling classes.\n" +
"Please add '@babel/plugin-transform-classes' to your Babel configuration.",
);
}

Expand Down