Fix `%==` parsing in hack pipes #13536

nicolo-ribaudo · 2021-07-07T16:21:38Z

Q	A
Fixed Issues?	Fixes failing CI in #13413
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

b9c1884 removed exprAllowed usage from the main parser. However, it was used by the %followed by a token starting with=. This PR modify the parser to re-interpret %=as%` when parsing it in an expression position if hack pipes are enabled.

cc @js-choi (please review!)

codesandbox-ci · 2021-07-07T16:26:21Z

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 d668a78:

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

babel-bot · 2021-07-07T17:11:09Z

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

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

Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

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

packages/babel-parser/src/tokenizer/types.js

Co-authored-by: J. S. Choi <jschoi@jschoi.org>

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

+
+          this.state.value = "%";
+          this.state.type = tt.modulo;
+          this.state.pos--;


JLHwung · 2021-07-13T20:48:51Z

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

+          // reparse it as %.
+          // The next readToken() call will start parsing from =.
+
+          this.state.value = "%";


We could skip this step since this.state.value is not used.

lightmare · 2021-07-13T21:11:17Z

Wouldn't this be better handled wholly in the tokenizer? I mean, when the tokenizer sees %= it could check the next character and unambiguously decide whether to return % or %=:

%== can only be [%, ==], it cannot be [%=, =] because that doesn't make sense
%=<any> cannot be assignment to %, so it has to be %=

nicolo-ribaudo · 2021-07-13T22:00:39Z

Uh that's a really good point.

JLHwung · 2021-07-13T22:06:32Z

@lightmare %=<any> can be a topic reference and equal sign, so we can not decide at the tokenizer level until we are parsing a primary expression, see #13536 (comment).

lightmare · 2021-07-13T22:37:12Z

Is assignment to topic reference allowed? And if not, can it be an early error?

Assuming x |> %=y should fail with "invalid assignment to %", rather than "expression expected, got %=", I think that'd still be possible even if the tokenizer returned [x, |>, %=, y] — only instead of "fixing" the state in the tt.moduloAssign case to split the %=, you'd forge an "invalid assignment" error. Admittedly that'd be essentially the same amount of code, only without rewinding the state.

js-choi · 2021-07-14T12:23:30Z

@lightmare: Assignment to % is indeed syntactically not allowed and is an early error.

If we pursue handling everything in tokenizer.js, then should that be changed in this pull request or in another pull request after this?

lightmare · 2021-07-14T13:52:23Z

If we pursue handling everything in tokenizer.js, then should that be changed in this pull request or in another pull request after this?

As @JLHwung pointed out, handling everything in tokenizer is not feasible, because the tokenizer doesn't have enough information to decide whether %= should be one token [%=] or two [%, =]. I was trying to convey that if the tokenizer did what I suggested previously — i.e. interpret %== as [%, ==] instead of [%=, =]; and interpret %=<anything else> as [%=, ...] — then the parser could subsequently catch the %= token where an expression is expected and treat is as invalid assignment to %, without rewinding the state to reparse the = and fail later.

JLHwung · 2021-07-14T14:51:15Z

Assignment to % is indeed syntactically not allowed and is an early error.

Yes. We should implement early errors for cases like x |> (# = 0), x |> (# += 0), x |> x[#] = 0, x |> ((#) => {}) etc. I think we should move tt.pipeline handling from parseExprOps to

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

Line 289 in bfcf783

if (this.state.type.isAssign) {

and descend to parseMaybeConditional call for RHS. This can be addressed in another PR.

js-choi · 2021-07-14T21:57:20Z

@JLHwung: The current implementation in babel:feat-7.15.0/hack-pipes already throws parsing errors for those cases and has tests for them (though I might have left out a test for x |> (#) => 1.

However, the errors usually are just generic “unexpected token” errors. Do you mean that we should make special error cases with more descriptive messages? And would this count as a breaking change?

JLHwung · 2021-07-15T02:00:54Z

@js-choi I checked the babel:feat-7.15.0/hack-pipes branch.

x |> (# = 0), x |> (# += 0), x |> (async (#) => {}) and x |> ((#) => {}) throw because checkLVal does not support TopicReference, and it should not.

However, it seems to me the current spec does not forbid such composition, because we lost the PipeExpression production limit once descended to CoverParenthesizedExpressionAndArrowParameterList. I think we may want similar limitation as how we handle delete (((p))) in the spec:

It is a Syntax Error if the derived PipeBody is
PrimaryExpression : CoverParenthesizedExpressionAndArrowParameterList
and CoverParenthesizedExpressionAndArrowParameterList ultimately derives either LeftHandSideExpression = AssignmentExpression or LeftHandSideExpression AssignmentOperator AssignmentExpression. This rule is recursively applied.

So we can allow parenthesized arrow expressions / yield expressions

x |> (async (x = %) => {})
x |> ((x = %) => {})
(function* () { x |> (yield %) })

but this rule still forbid x |> (x[#] = 0).

x |> (x[#] = 0) is not throwing because checkLVal does not check if a MemberExpression contains topic tokens. I personally lean to allow it, if so we can forbid only ? = AssignmentExpression and ? AssignmentOperator AssignmentExpression.

However, the errors usually are just generic “unexpected token” errors.

Yes if we parse PipeExpression in restricted grammar as defined in the spec, we surely can not recover from many cases. As an early proposal I tend to prioritize on the correctness and then we can always improve the error handling later when the proposal gets advanced.

lightmare · 2021-07-15T09:20:41Z

x |> (x[#] = 0) is not throwing because checkLVal does not check if a MemberExpression contains topic tokens. I personally lean to allow it, if so we can forbid only ? = AssignmentExpression and ? AssignmentOperator AssignmentExpression.

x |> (x[#] = 0) is perfectly valid, # is not the l-value there, just a key. Similarly with x |> (#[x] = 0) or x |> (x[0] = #).

I think the right place to forbid topic assignment is in AssignmentTargetType.

nicolo-ribaudo · 2021-07-15T16:34:12Z

@lightmare

then the parser could subsequently catch the %= token where an expression is expected and treat is as invalid assignment to %, without rewinding the state to reparse the = and fail later.

We would still have to rewind the state and transform it to two tokens, because our parser needs to be able to recover from early errors and continue parsing.

JLHwung · 2021-07-20T14:40:19Z

Merging with the proposal author (@js-choi) 's approval and mine.

* Fix `%==` parsing in hack pipes * Use custom token type * Update packages/babel-parser/src/parser/expression.js Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com> * Apply suggestions from code review Co-authored-by: J. S. Choi <jschoi@jschoi.org> Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com> Co-authored-by: J. S. Choi <jschoi@jschoi.org>

Fix %== parsing in hack pipes

4a9fcf5

nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Pipeline Operator labels Jul 7, 2021

nicolo-ribaudo changed the title ~~Add support for the "Hack" pipeline proposal (#13191)~~ Fix %== parsing in hack pipes Jul 7, 2021

nicolo-ribaudo mentioned this pull request Jul 7, 2021

Add support for the "Hack" pipeline proposal (#13191) #13413

Merged

4 tasks

JLHwung reviewed Jul 7, 2021

View reviewed changes

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

JLHwung reviewed Jul 7, 2021

View reviewed changes

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

nicolo-ribaudo and others added 2 commits July 8, 2021 17:59

Use custom token type

e8feae8

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

1da535f

Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>

lightmare reviewed Jul 9, 2021

View reviewed changes

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

js-choi reviewed Jul 12, 2021

View reviewed changes

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

js-choi reviewed Jul 12, 2021

View reviewed changes

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

Apply suggestions from code review

d668a78

Co-authored-by: J. S. Choi <jschoi@jschoi.org>

js-choi approved these changes Jul 13, 2021

View reviewed changes

JLHwung reviewed Jul 13, 2021

View reviewed changes

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

this.state.value = "%";

this.state.type = tt.modulo;

this.state.pos--;

This comment was marked as resolved.

Sign in to view

JLHwung reviewed Jul 13, 2021

View reviewed changes

JLHwung approved these changes Jul 13, 2021

View reviewed changes

JLHwung mentioned this pull request Jul 14, 2021

feat: support hack pipeline in @babel/standalone #13555

Merged

js-choi mentioned this pull request Jul 16, 2021

Grammar allows topic in some assignment targets tc39/proposal-hack-pipes#9

Closed

JLHwung merged commit 85d7e2f into babel:feat-7.15.0/hack-pipes Jul 20, 2021

nicolo-ribaudo deleted the fix-mod-eq-parsing-hack-pipes branch July 20, 2021 19:02

This was referenced Aug 5, 2021

chore(deps-dev): bump @babel/eslint-parser from 7.13.14 to 7.15.0 filecoin-project/slate#863

Merged

chore(deps-dev): bump @babel/core from 7.13.14 to 7.15.0 filecoin-project/slate#862

Closed

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

github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `%==` parsing in hack pipes #13536

Fix `%==` parsing in hack pipes #13536

nicolo-ribaudo commented Jul 7, 2021 •

edited

codesandbox-ci bot commented Jul 7, 2021 •

edited

babel-bot commented Jul 7, 2021 •

edited

This comment was marked as resolved.

JLHwung Jul 13, 2021

lightmare commented Jul 13, 2021

nicolo-ribaudo commented Jul 13, 2021

JLHwung commented Jul 13, 2021

lightmare commented Jul 13, 2021

js-choi commented Jul 14, 2021 •

edited

lightmare commented Jul 14, 2021

JLHwung commented Jul 14, 2021

js-choi commented Jul 14, 2021 •

edited

JLHwung commented Jul 15, 2021 •

edited

lightmare commented Jul 15, 2021

nicolo-ribaudo commented Jul 15, 2021

JLHwung commented Jul 20, 2021

Fix %== parsing in hack pipes #13536

Fix %== parsing in hack pipes #13536

Conversation

nicolo-ribaudo commented Jul 7, 2021 • edited

codesandbox-ci bot commented Jul 7, 2021 • edited

babel-bot commented Jul 7, 2021 • edited

This comment was marked as resolved.

JLHwung Jul 13, 2021

Choose a reason for hiding this comment

lightmare commented Jul 13, 2021

nicolo-ribaudo commented Jul 13, 2021

JLHwung commented Jul 13, 2021

lightmare commented Jul 13, 2021

js-choi commented Jul 14, 2021 • edited

lightmare commented Jul 14, 2021

JLHwung commented Jul 14, 2021

js-choi commented Jul 14, 2021 • edited

JLHwung commented Jul 15, 2021 • edited

lightmare commented Jul 15, 2021

nicolo-ribaudo commented Jul 15, 2021

JLHwung commented Jul 20, 2021

Fix `%==` parsing in hack pipes #13536

Fix `%==` parsing in hack pipes #13536

nicolo-ribaudo commented Jul 7, 2021 •

edited

codesandbox-ci bot commented Jul 7, 2021 •

edited

babel-bot commented Jul 7, 2021 •

edited

js-choi commented Jul 14, 2021 •

edited

js-choi commented Jul 14, 2021 •

edited

JLHwung commented Jul 15, 2021 •

edited