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.

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

fix: disallow all parenthesized pattern except parsing LHS #12327

Merged
merged 9 commits into from Nov 10, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 7, 2020

Q                       A
Fixed Issues? Babel does not throw on invalid ([(a)]) => {} and async ([(a)]) => {} (REPL)
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

Found this bug when skimming on parser source and reading on

we need to make sure that if this is an async arrow functions, that we don't allow inner parens inside the params

My gut tells me the current implementation is not complete and it turns out it can not handle edge cases like I mentioned above. I come up with current solution when jogging in the warm early November.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser labels Nov 7, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 7, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/32292/

let parenthesized = undefined;
if (node.type === "ParenthesizedExpression" || node.extra?.parenthesized) {
parenthesized = unwrapParenthesizedExpression(node);
if (
parenthesized.type !== "Identifier" &&
parenthesized.type !== "MemberExpression"
!isLHS ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first attempt is to use !this.state.maybeInArrowParameters here but I realize that then we have to reset this state for class blocks. It is easy to mess up when things get nested.

(a = class { static { [(a)] = 1 }) => {}

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 7, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 46de321:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@@ -891,11 +884,6 @@ export default class ExpressionParser extends LValParser {
);
}

// we found an async arrow function so let's not allow any inner parens
if (possibleAsyncArrow && innerParenStart && this.shouldParseAsyncArrow()) {
this.unexpected();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of throwing unexpected errors on

var foo = async ((foo)) => {};

we can now recover and raise this error in toAssignable, which I think is more helpful

"SyntaxError: Invalid parenthesized assignment pattern (1:18)"

@JLHwung JLHwung marked this pull request as draft November 7, 2020 15:16
@JLHwung JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from 76b8566 to 4eecb1b Compare November 7, 2020 16:21
@@ -1421,12 +1409,6 @@ export default class ExpressionParser extends LValParser {
) {
this.expressionScope.validateAsPattern();
this.expressionScope.exit();
for (const param of exprList) {
if (param.extra && param.extra.parenthesized) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can now recover from var foo = ((foo)) => {};

@@ -1 +0,0 @@
((a)) => 42
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is removed because it is almost identical to

@@ -1 +0,0 @@
((a)) => 42
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is removed because it is almost identical to

@@ -1 +0,0 @@
(a, (b)) => 42
Copy link
Contributor Author

@JLHwung JLHwung Nov 7, 2020

Choose a reason for hiding this comment

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

This test is removed because it is identical to

@JLHwung JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from 4eecb1b to 0c1c38e Compare November 7, 2020 16:34
@JLHwung JLHwung marked this pull request as ready for review November 7, 2020 16:44

* @returns {Node} The converted assignable pattern
* @memberof LValParser
*/
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the doc comments you are introducing!

packages/babel-parser/src/parser/lval.js Outdated Show resolved Hide resolved
packages/babel-parser/src/parser/lval.js Outdated Show resolved Hide resolved
@JLHwung JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from 9429966 to 00a776c Compare November 9, 2020 16:53
@@ -1 +0,0 @@
([ [(a)] = [] ] = []) => {}
Copy link
Contributor Author

@JLHwung JLHwung Nov 9, 2020

Choose a reason for hiding this comment

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

@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Nov 9, 2020
@JLHwung JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from cc6569c to 64b9236 Compare November 9, 2020 19:50
Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Nice work!

Co-authored-by: Brian Ng <bng412@gmail.com>
@JLHwung JLHwung merged commit 5bbad89 into babel:main Nov 10, 2020
@JLHwung JLHwung deleted the fix-invalid-parenthesized-param-in-arrow branch November 10, 2020 19:42
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants