Hack-pipe proposal with % topic token

[js-choi]

Q	A
Fixed Issues?	None
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link	babel/website#2541
Any Dependency Changes?	No
License	MIT

This is a continuation of #13191, which added support for { proposal: "hack", topicToken: "#" } to the proposal-pipeline-operator plugin. This pull request adds support for configuring proposal-pipeline-operator with topicToken: "%" instead of "#". See also js-choi/proposal-hack-pipes, tc39/proposal-pipeline-operator#91, and tc39/proposal-hack-pipes#2.

[codesandbox-ci]

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 974d8fa:

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

[JLHwung]

The beforeExpr is required by v8intrinsic plugin.

[js-choi]

Is there a v8intrinsic test case that is broken by this change? The babel-parser/test/fixtures/v8intrinsic suite is still succeeding.

It does seem that currently, when babel-parser is configured with both pipelineOperator: { proposal: "hack", topicToken: "%" } and v8intrinsic on, x |> % breaks due toparseExprAtom always calling parseV8Intrinsic first, which then calls unexpected. This seems to be a separate problem, though.

[nicolo-ribaudo]

I think beforeExpr is not needed for the v8intrinsic plugin, but for the normal % operator.

Could you check if this code is still parsed correctly?

5 % /3/g

We might need to manually manage exprAllowed for % instead of relying on beforeExpr.

Also, we should check if these two things are parsed correctly:

a |> %
function f() {}

a %
function f() {}

[js-choi]

@nicolo-ribaudo: I added tests for those cases in 5d24958. They seem to be still parsing correctly.

[nicolo-ribaudo]

Oh awesome. I'll still try to find an example that breaks it because I'm not 100% convinced yet that beforeExpr is completely unnecessary, but if I cannot find it then it's good 😅

[js-choi]

I just pushed efdac2d, which makes v8intrinsics and topicToken: "%" compatible. I don’t like the current implementation, and I think it can be refactored to remove code duplication. Edit: The duplication has been improved in b1dd014.

The problem was that activating babel-parser’s v8intrinsic plugin causes parseExprAtom to preferentially call parseV8Intrinsic. If the current token is %, then parseV8Intrinsic will consume the % and then attempt to match an identifier after the %. If it does not find such an identifier after the consumed %, then it will throw a fatal unexpected error.

This makes the default parseExprAtom method, when v8intrinsic is on, unable to handle any other % atom (that is not followed by an identifier). Because parseV8Intrinsic consumes the % token, it short-circuits the default parseExprAtom method.

Ideally, parseV8Intrinsic would use a lookahead method like match that would look ahead at the next two tokens. If parseV8Intrinsic looked ahead two tokens (for a % token followed by an identifier), instead of consuming the %, then it could defer matching any other % token to the default parseExprAtom method.

But, barring that, efdac2d adds a new branch to parseV8Intrinsic that checks whether topicToken: "%" is active, and prevents it from calling unexpected. I’m sure its duplicate code can be reduced further.

CC: @ljharb

[js-choi]

We also need to also add tests for Hack pipes with syntactic placeholders.

We might have to face some interesting questions. For example, should x |> %%%%EXPRESSION%% be valid and equivalent to x |> %%EXPRESSION%% % %? (Is x%%%EXPRESSION%% even a valid modulo operation?)

Or perhaps we should simply forbid topicToken: "%" with placeholders, like how v8intrinsics with placeholders are already forbidden.

Edit: It looks like, when with placeholders are turned on:

1. x%%%VALUE%% already throws Unexpected token, rather than parsing successfully as (x) % (%%VALUE%%), in the status quo without Hack pipes.
2. With Hack pipes and percent topic tokens, x |> %% %%VALUE%%, x |> %%%%VALUE%%, x |> %%VALUE%% %%, and x |> %%VALUE%%%% also throw Unexpected token, rather than parsing successfully as (x |> (%) % (%%VALUE%%)) or (x |> (%%VALUE%%) % (%)), since the tokenizer eagerly consumes any %% as a placeholder sigil.
3. Everything else, like x |> % % %%VALUE%% and x |> %%VALUE%% % %, already does successfully and correctly parse.

Perhaps the second examples should remain invalid, in order to match the first example’s precedent, and to avoid modifying babel-parser/src/plugins/placeholders.js. I added tests assuming as such in 0a65467.

[nicolo-ribaudo]

We can start by disallowing the v8intrinsics and placeholders plugins, and think about it again if TC39 will end up deciding to use %.

[js-choi]

fa257f5 disables combining v8intrinsic/placeholders and Hack pipes in babel-parser. It also reverts changes to babel-parser/src/plugins/v8intrinsic.js.

[JLHwung]

I don't think it conflicts with v8intrinsic: we can check if an identifier start follows % immediately when parsing PrimayExpression in parseExprAtom. If it is, it goes with v8intrinsic, otherwise it is a pipe topic.

[js-choi]

@JLHwung: Yes, it is true that v8intrinsic and topicToken: "%" do not actually syntactically conflict. I had already made a now-reverted change that enabled their coexistence, in efdac2d. However, in order to do this, I had to change babel-parser/src/plugins/v8intrinsic.js.

When @nicolo-ribaudo suggested disallowing v8intrinsic unless TC39 commits to Hack pipes with %, I assumed that it was to prevent changing v8intrinsic.js unnecessarily, in case TC39 ends up not choosing Hack pipes with %. So I reverted the changes to v8intrinsic.js in fa257f5.

[JLHwung]

I have rebased on upstream/feat-7.15.0/hack-pipes and pushed here: https://github.com/JLHwung/babel/commits/proposal-hack-pipes-percent-rebased

I think it can be a good start for reviewing the real diff. The are some parser testing errors from outdated test fixtures, likely due to #13426. I haven't updated the test fixtures.

[js-choi]

@JLHwung: I have made a merge from upstream/feat-7.15.0/hack-pipes to this branch, in 91d28f9.

I can rebase this branch on upstream/feat-7.15.0/hack-pipes instead, but there a particular reason that we need to rebase instead of merge?

[JLHwung]

@js-choi We squash merge PRs so if your PR depends on the unsquashed branch, GitHub will show all the different commits (87) instead of real different commits (20). The rebased history is more focused because it does not include a merge commit.

[js-choi]

I have squashed, rebased onto #13413, and force pushed.

[nicolo-ribaudo]

I'm merging this with my and @fedeci's reviews: even if @fedeci's check is not green, he has quite a bit of experience in @babel/parser now.

If there needs to be other changes, this PR is not yet on main but in feat-7.15.0/hack-pipes. I'm merging this because I need to rebase that branch and it's easier to rebase both the commits at the same time.