[parser] validation for parentheses in the left-hand side of assignment expressions #10576

boweihan · 2019-10-18T23:58:42Z

Q	A
Fixed Issues?	Fixes #8796, fixes #9899
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link	No
Any Dependency Changes?	No
License	MIT

@nicolo-ribaudo has a good description of the problem in the linked issue. Basically, the parser is missing some validation for parentheses in the left-hand side of assignment expressions.

This means that expressions such as [a = 1] = t and [ { x: y = 3 } ] = t are valid (and correctly so!).

The issue with the current behaviour is that expressions such as 1:(a = 1) = t and 2:[(a = 1)] = t are treated as valid. The left-hand side of assignment expressions should be invalid if there are any parenthesized child nodes which are not of type Identifier or MemberExpression.

The fix in this pr involves:

adding the parens check to toAssignable
updating / adding test cases to reflect new behavior

buildsize · 2019-10-19T00:12:59Z

File name	Previous Size	New Size	Change
babel-preset-env.js	2.77 MB	2.77 MB	-78 bytes (0%)
babel-preset-env.min.js	1.67 MB	1.67 MB	-13 bytes (0%)
babel.js	2.95 MB	2.95 MB	-78 bytes (0%)
babel.min.js	1.63 MB	1.63 MB	-13 bytes (0%)
test262.tap	4.84 MB	[deleted]

nicolo-ribaudo · 2019-10-19T11:07:45Z

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

+              prop,
+              isBinding,
+              isLast,
+              allowAssign,


Why is it passed down? Inside an object property, assignments are always allowed (for default props)

The original approach needed to pass it down to "remember" that there had been a parenthesis somewhere up the stack - to invalidate expressions such as [({ a: [b = 2]})] = t. It's become more clear to me now that the presence of certain parenthesis in the left-hand side of an assignment expression is the problem so I'll clean this up.

nicolo-ribaudo

While playing in the repl, I found another case which is deeply related to this one:

[([x])] = y;

I know that I wrote the hint you followed to implement this PR, but I now think that there could be a better way of doing it 😅
Since parenthesized assignment target are the exception, and they should only be allowed in a top-level target (i.e. not inside a destructuring), I think that we can pass a isParenthesized flag instead of allowAssign and:

If it isn't set, you should set it to node.extra?.parenthesized using a default param
If isParenthesized is true and the current node type is not MemberExpression or Identifier, throw an error (e.g. Invalid parenthesized assignment pattern).

Note that the isParenthesized parameter should be set to true only when converting the child a ParenthesizedExpression

boweihan · 2019-10-19T19:59:08Z

packages/babel-parser/test/fixtures/es2015/destructuring/parenthesized-lhs-array/options.json

@@ -1,3 +1,3 @@
 {
-  "throws": "You're trying to assign to a parenthesized expression, eg. instead of `([a]) = 0` use `([a] = 0)` (1:1)"


We lose this specific error message by applying a more general fix but I think it's probably worth it in terms of code cleanliness - messages for specific cases can be added as an extension.

boweihan · 2019-10-19T20:10:27Z

@nicolo-ribaudo makes sense - I've updated the code. I think we can get away with not having an extra parameter in toAssignable.

The new logic covers most of the parens validation needed except for when the createParenthesizedExpressions parser option is set to true - github.com//pull/8025. There is separate validation for that code path which I have kept as-is (unwrapParenthesizedExpression in parser/expression.js).

nicolo-ribaudo

The new logic covers most of the parens validation needed except for when the createParenthesizedExpressions parser option is set to true

So it still doesn't throw the error in that case?

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

boweihan · 2019-11-03T03:59:05Z

@nicolo-ribaudo @JLHwung originally I thought there might need to be a separate flow for handling the createParenthesizedExpressions parser option but I've figured out a more straightforward approach that works for both. Have added tests to cover each case.

Edit: looks like tests may be failing due to a memory issue - ENOMEM: not enough memory, read

boweihan · 2019-11-03T04:01:37Z

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

@@ -19,6 +19,12 @@ import { isStrictBindReservedWord } from "../util/identifier";
 import { NodeUtils } from "./node";
 import { type BindingTypes, BIND_NONE } from "../util/scopeflags";

+const unwrapParenthesizedExpression = (node: Node) => {


this is needed to handle nested parenthesis when createParenthesizedExpressions is enabled. i.e. [(((x)))]

For nested parenthesis, toAssignable will be called with parenthesizee recursively. Therefore we may check if node.expression is one of Identifier, MemberExpression and ParenthesizedExpression.

JLHwung · 2019-11-03T22:07:29Z

@boweihan Circle CI is flaky. I have rerun the build.

JLHwung · 2019-12-10T01:29:26Z

@boweihan Well done! Thanks.

boweihan changed the title ~~[parser] validation for assignment patterns in the left-hand side of expressions~~ [parser] validation for left-hand side assignments in assignments Oct 19, 2019

boweihan force-pushed the issue/8796 branch from d09457a to d12d61d Compare October 19, 2019 00:05

boweihan changed the title ~~[parser] validation for left-hand side assignments in assignments~~ [parser] validation for left-hand side assignments in assignment expressions Oct 19, 2019

nicolo-ribaudo added pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories labels Oct 19, 2019

nicolo-ribaudo reviewed Oct 19, 2019

View reviewed changes

boweihan force-pushed the issue/8796 branch 4 times, most recently from 6807302 to 7e158a1 Compare October 19, 2019 19:54

boweihan commented Oct 19, 2019

View reviewed changes

boweihan changed the title ~~[parser] validation for left-hand side assignments in assignment expressions~~ [parser] validation for parentheses in the left-hand side of assignment expressions Oct 19, 2019

JLHwung self-requested a review November 1, 2019 15:45

nicolo-ribaudo reviewed Nov 2, 2019

View reviewed changes

packages/babel-parser/src/parser/lval.js Outdated Show resolved Hide resolved

boweihan force-pushed the issue/8796 branch from 2ac810d to 484a6d6 Compare November 3, 2019 03:51

boweihan commented Nov 3, 2019

View reviewed changes

boweihan added 2 commits December 10, 2019 01:10

invalid left-hand assignments based on parenthesis

9724926

validate against nested ParenthesizedExpressions

b1ca931

nicolo-ribaudo force-pushed the issue/8796 branch from 484a6d6 to b1ca931 Compare December 10, 2019 00:10

nicolo-ribaudo approved these changes Dec 10, 2019

View reviewed changes

JLHwung approved these changes Dec 10, 2019

View reviewed changes

JLHwung merged commit 20e43ad into babel:master Dec 10, 2019

snyk-bot mentioned this pull request Jan 12, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.7.7 mboocannon/msgraph-sdk-javascript#1

Open

snyk-bot mentioned this pull request Jan 20, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.8.0 mboocannon/msgraph-sdk-javascript#3

Open

snyk-bot mentioned this pull request Feb 5, 2020

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.8.3 mboocannon/msgraph-sdk-javascript#4

Open

This was referenced Mar 7, 2020

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.8.7 ryanhefner/contentful-sitemap#49

Merged

[Snyk] Upgrade @babel/runtime from 7.5.5 to 7.8.7 ryanhefner/creature-cache#13

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 10, 2020

github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[parser] validation for parentheses in the left-hand side of assignment expressions #10576

[parser] validation for parentheses in the left-hand side of assignment expressions #10576

boweihan commented Oct 18, 2019 •

edited by JLHwung

buildsize bot commented Oct 19, 2019 •

edited

nicolo-ribaudo Oct 19, 2019

boweihan Oct 19, 2019 •

edited

nicolo-ribaudo left a comment •

edited

boweihan Oct 19, 2019 •

edited

boweihan commented Oct 19, 2019

nicolo-ribaudo left a comment

boweihan commented Nov 3, 2019 •

edited

boweihan Nov 3, 2019

JLHwung Nov 3, 2019 •

edited

JLHwung commented Nov 3, 2019

JLHwung commented Dec 10, 2019

		@@ -1,3 +1,3 @@
		{
		"throws": "You're trying to assign to a parenthesized expression, eg. instead of `([a]) = 0` use `([a] = 0)` (1:1)"

[parser] validation for parentheses in the left-hand side of assignment expressions #10576

[parser] validation for parentheses in the left-hand side of assignment expressions #10576

Conversation

boweihan commented Oct 18, 2019 • edited by JLHwung

buildsize bot commented Oct 19, 2019 • edited

nicolo-ribaudo Oct 19, 2019

Choose a reason for hiding this comment

boweihan Oct 19, 2019 • edited

Choose a reason for hiding this comment

nicolo-ribaudo left a comment • edited

Choose a reason for hiding this comment

boweihan Oct 19, 2019 • edited

Choose a reason for hiding this comment

boweihan commented Oct 19, 2019

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

boweihan commented Nov 3, 2019 • edited

boweihan Nov 3, 2019

Choose a reason for hiding this comment

JLHwung Nov 3, 2019 • edited

Choose a reason for hiding this comment

JLHwung commented Nov 3, 2019

JLHwung commented Dec 10, 2019

boweihan commented Oct 18, 2019 •

edited by JLHwung

buildsize bot commented Oct 19, 2019 •

edited

boweihan Oct 19, 2019 •

edited

nicolo-ribaudo left a comment •

edited

boweihan Oct 19, 2019 •

edited

boweihan commented Nov 3, 2019 •

edited

JLHwung Nov 3, 2019 •

edited