Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Grammar allows topic in some assignment targets #9

Closed
js-choi opened this issue Jul 16, 2021 · 2 comments
Closed

Grammar allows topic in some assignment targets #9

js-choi opened this issue Jul 16, 2021 · 2 comments
Assignees

Comments

@js-choi
Copy link
Collaborator

js-choi commented Jul 16, 2021

@JLHwung raised a concern in babel/babel#13536 (comment). To prevent cluttering up babel/babel#13536, I’m opening an issue here. The topic token is assumed to be % in this issue.

The current grammar does not correctly raise early errors for x |> ((%) => {}) and x |> (async (%) => {}). This is because, inside of a PipeBody, % is hidden from the Contains operation—including in any CoverParenthesizedExpressionAndArrowParameterList that is within the PipeBody.

So, just like how the current grammar (correctly) allows x |> ((%)), so too does it (incorrectly) allow x |> ((%) => {}). In both cases, PipeBody contains a topic inside of two nested CoverParenthesizedExpressionAndArrowParameterLists.

@lightmare pointed to AssignmentTargetType as a good place to prohibit the topic for these cases. This should also let expressions like x |> %[y] = z, x |> y[%] = z, and x |> ((y = %) => {}) remain correctly valid.

(@JLHwung also said that the current grammar also does not correctly raise early errors for x |> (% = 0) and x |> (% += 0). However, I’m not sure if this is true. LeftHandSideExpression allows only NewExpression, CallExpression, and OptionalExpression, and none of these expressions can result in an assignment to a topic reference.)

@js-choi js-choi self-assigned this Jul 16, 2021
@lightmare
Copy link

so too does it (incorrectly) allow x |> ((%) => {})

I don't think it does. ArrowParameters only allow BindingIdentifier, which % is not.

LeftHandSideExpression allows only NewExpression

NewExpression > MemberExpression > PrimaryExpression > %

@js-choi
Copy link
Collaborator Author

js-choi commented Jul 16, 2021

@lightmare: Thanks for the correction; I was mistaken. The grammar currently correctly prohibits x |> ((%) => {}) ((%) matches neither BindingIdentifier nor ( UniqueFormalParameters )) but incorrectly allows x |> (% = 0). Either way, we should add a restriction to AssignmentTargetType as you suggested.

@js-choi js-choi changed the title Grammar allows topic in some arrow parameters Grammar allows topic in some assignment targets Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants