fix: disallow all parenthesized pattern except parsing LHS #12327

JLHwung · 2020-11-07T01:26:00Z

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.

babel-bot · 2020-11-07T01:28:42Z

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

JLHwung · 2020-11-07T01:29:16Z

packages/babel-parser/src/parser/lval.js

      if (
-        parenthesized.type !== "Identifier" &&
-        parenthesized.type !== "MemberExpression"
+        !isLHS ||


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 · 2020-11-07T01:31:03Z

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

JLHwung · 2020-11-07T01:32:10Z

packages/babel-parser/src/parser/expression.js


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


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 · 2020-11-07T16:23:46Z

packages/babel-parser/src/parser/expression.js

      this.expressionScope.validateAsPattern();
      this.expressionScope.exit();
-      for (const param of exprList) {
-        if (param.extra && param.extra.parenthesized) {


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

JLHwung · 2020-11-07T16:28:46Z

packages/babel-parser/test/fixtures/es2015/uncategorised/240/input.js

@@ -1 +0,0 @@
-((a)) => 42


This test is removed because it is almost identical to

babel/packages/babel-parser/test/fixtures/es2015/arrow-functions/inner-parens/input.js

Line 1 in 3227914

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

JLHwung · 2020-11-07T16:29:52Z

packages/babel-parser/test/fixtures/esprima/invalid-syntax/migrated_0085/input.js

@@ -1 +0,0 @@
-((a)) => 42


This test is removed because it is almost identical to

babel/packages/babel-parser/test/fixtures/es2015/arrow-functions/inner-parens/input.js

Line 1 in 3227914

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

JLHwung · 2020-11-07T16:31:39Z

packages/babel-parser/test/fixtures/esprima/invalid-syntax/migrated_0086/input.js

@@ -1 +0,0 @@
-(a, (b)) => 42


This test is removed because it is identical to

babel/packages/babel-parser/test/fixtures/es2015/arrow-functions/inner-parens-2/input.js

Line 1 in 4eecb1b

(a, (b)) => 42

nicolo-ribaudo · 2020-11-07T16:47:07Z

packages/babel-parser/src/parser/lval.js

+
+   * @returns {Node} The converted assignable pattern
+   * @memberof LValParser
+   */


Thanks for all the doc comments you are introducing!

packages/babel-parser/src/parser/lval.js

...ges/babel-parser/test/fixtures/es2015/arrow-functions/.inner-parens-array-pattern-2/input.js

...ages/babel-parser/test/fixtures/es2015/arrow-functions/.inner-parens-object-pattern/input.js

JLHwung · 2020-11-09T17:06:15Z

...ges/babel-parser/test/fixtures/es2015/arrow-functions/.inner-parens-array-pattern-2/input.js

@@ -1 +0,0 @@
-([ [(a)] = [] ] = []) => {}


I have enabled these tests on 00a776c but forgot to remove the skipped ones: https://github.com/babel/babel/pull/12327/files#diff-cb581cc9c63ffb0abb517f0508ad77cfd53fa50f53f16ee6e61eda11a0be0e3d

existentialism

Nice work!

packages/babel-parser/src/util/expression-scope.js

Co-authored-by: Brian Ng <[email protected]>

JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser labels Nov 7, 2020

JLHwung commented Nov 7, 2020

View reviewed changes

JLHwung marked this pull request as draft November 7, 2020 15:16

JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from 76b8566 to 4eecb1b Compare November 7, 2020 16:21

JLHwung commented Nov 7, 2020

View reviewed changes

JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from 4eecb1b to 0c1c38e Compare November 7, 2020 16:34

JLHwung marked this pull request as ready for review November 7, 2020 16:44

nicolo-ribaudo reviewed Nov 7, 2020

View reviewed changes

JLHwung commented Nov 7, 2020

View reviewed changes

...ges/babel-parser/test/fixtures/es2015/arrow-functions/.inner-parens-array-pattern-2/input.js Show resolved Hide resolved

JLHwung commented Nov 7, 2020

View reviewed changes

...ages/babel-parser/test/fixtures/es2015/arrow-functions/.inner-parens-object-pattern/input.js Show resolved Hide resolved

JLHwung added 6 commits November 9, 2020 11:53

fix: disallow all parenthesized pattern except parsing LHS

b14317c

fix: forStatement requires LHS

fb8883f

simplify toAssignable

ff6740b

add more test cases

8e809d9

fix: pass through isLHS on object property and assignment expression

8a8e3f5

fix: record parenthesized identifier error for LHS

00a776c

JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from 9429966 to 00a776c Compare November 9, 2020 16:53

remove duplicated skipped tests

e9c2070

JLHwung commented Nov 9, 2020

View reviewed changes

nicolo-ribaudo approved these changes Nov 9, 2020

View reviewed changes

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

fix: do not record errors on ancestry arrow head

64b9236

JLHwung force-pushed the fix-invalid-parenthesized-param-in-arrow branch from cc6569c to 64b9236 Compare November 9, 2020 19:50

nicolo-ribaudo approved these changes Nov 9, 2020

View reviewed changes

JLHwung mentioned this pull request Nov 10, 2020

Parenthesized LHS is not reinterpreted as parethensized destructuring target in some edge cases KFlash/seafox#13

Open

existentialism approved these changes Nov 10, 2020

View reviewed changes

existentialism reviewed Nov 10, 2020

View reviewed changes

packages/babel-parser/src/util/expression-scope.js Outdated Show resolved Hide resolved

Update packages/babel-parser/src/util/expression-scope.js

46de321

Co-authored-by: Brian Ng <[email protected]>

JLHwung merged commit 5bbad89 into babel:main Nov 10, 2020

JLHwung deleted the fix-invalid-parenthesized-param-in-arrow branch November 10, 2020 19:42

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 bot locked as resolved and limited conversation to collaborators Feb 10, 2021

		@@ -1 +0,0 @@
		((a)) => 42 No newline at end of file

		@@ -1 +0,0 @@
		((a)) => 42

Uh oh!

fix: disallow all parenthesized pattern except parsing LHS #12327

fix: disallow all parenthesized pattern except parsing LHS #12327

Uh oh!

Conversation

JLHwung commented Nov 7, 2020 • edited by gitpod-io bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLHwung Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

codesandbox-ci bot commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLHwung Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

JLHwung Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

JLHwung Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

JLHwung Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

JLHwung Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JLHwung Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

existentialism left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JLHwung commented Nov 7, 2020 •

edited by gitpod-io bot

Loading

babel-bot commented Nov 7, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 7, 2020 •

edited

Loading

JLHwung Nov 7, 2020 •

edited

Loading

JLHwung Nov 9, 2020 •

edited

Loading