Reduce `exprAllowed` usage #13431

JLHwung · 2021-06-07T21:39:36Z

Q	A
Tests Added + Pass?	Yes
License	MIT

This PR reduces the tokenizer state exprAllowed usage. It is now used only in the JSX plugin and we could consider move exprAllowed updates to the plugin.

The exprAllowed was used for 1) checking regex start, 2) allowing *=> in the Flow plugin and 3) checking JSX expression start. It turns out we can come up with an alternative approach for the 1) and 2) use case, which greatly simplifies the tokenizer context update logic which involves many checks.

In this PR, the tokenizer context now only takes care of whether } matches to ${ or {. The exprAllowed are now only maintained in the JSX plugin, and used for disambiguate 1) relational expression < and JSX Tag start <jsx>; 2) object literal ({name}) and JSX text.

~~I will mark this PR ready for review when refactoring exprAllowed for the third usage is done.~~

babel-bot · 2021-06-07T21:42:11Z

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

codesandbox-ci · 2021-06-07T21:44:53Z

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

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

nicolo-ribaudo

I love how this simplifies the code.

nicolo-ribaudo · 2021-06-07T22:56:19Z

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

@@ -140,6 +140,7 @@ export const types: { [name: string]: TokenType } = {

  eq: new TokenType("=", { beforeExpr, isAssign }),
  assign: new TokenType("_=", { beforeExpr, isAssign }),
+  slashAssign: new TokenType("_=", { beforeExpr, isAssign }),


nit

Suggested change

slashAssign: new TokenType("_=", { beforeExpr, isAssign }),

slashAssign: new TokenType("/=", { beforeExpr, isAssign }),

The token type label is visible when tokens: true is enabled so it may breaks if people are depending on token.label since /= was parsed as tt.assign whose label is /=. We can modify the label in Babel 8.

why wrap it in a new class and use the 'new' keyword. Did you run a benchmark test on that?

The token is initiated once and reused elsewhere. I am aware that using a binary packed number is more optimal because it eliminates the memory loading instructions compiled from tt.slashAssign. Also note that the NewExpression here is a one time cost, we reuse the token object defined here in finishToken.

However this PR is refactoring, it only tries to not degrade the performance so reviewers can be focused on the code architecture changes. We will land performance improvement in a separate PR.

JLHwung · 2021-06-08T02:53:38Z

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

+        case tt.bracketR:
+        case tt.braceBarR:
+        case tt.colon:
+        case tt.comma:


The original approach checks tt.semi and then excludes tokens with type.startsExpr. I think the new approach easier to reason about since it is an allowlist. This approach is taken from V8:

https://source.chromium.org/chromium/chromium/src/+/main:v8/src/parsing/parser-base.h;l=2979;drc=d17745f350d4956a07cb1113ee19e9cbc4be699f;bpv=1;bpt=1

However, V8 has tt._in here, which I think it is redundant because it seems to me that in never follow an AssignmentExpression. The spec has

[+In] RelationalExpression[?In, ?Yield, ?Await] in ShiftExpression for ( LeftHandSideExpression in Expression ) Statement for ( var ForBinding in Expression ) Statement for ( ForDeclaration in Expression ) Statement

None of the productions before in can parsed down to an argument-less YieldExpression.

Maybe it's because of this?

function* fn() { a ? yield : 2 }

Oh I meant for tt._in. 🤦

JLHwung · 2021-06-08T14:42:53Z

packages/babel-parser/src/plugins/jsx/index.js

-  this.state.exprAllowed = false;
-};
-
-tt.jsxTagEnd.updateContext = function (prevType) {


It is merged with updateContext because it manipulates this.state.exprAllowed based on different contexts.

nicolo-ribaudo · 2021-06-08T14:46:05Z

packages/babel-parser/test/fixtures/es2015/yield/regexp/input.js

@@ -0,0 +1,2 @@
+function *f1() { yield / 1 /g }
+function *f2() { yield /=2 /i }


If we don't already have it, can you add this test? (with normal functions)

function f1() { yield / 1 /g } function f2() { yield /=2 /i }

JLHwung · 2021-06-08T14:50:31Z

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

-// regular expression).
+// The `beforeExpr` property is used to disambiguate between 1) binary
+// expression (<) and JSX Tag start (<name>); 2) object literal and JSX
+// texts. It is set on the `updateContext` function in the JSX plugin.


We can probably further simplify the exprAllowed logic in updateContext of JSX plugin, based on the usage here. However I would like to land it separately before this PR is too big for review.

packages/babel-parser/src/plugins/jsx/index.js

nicolo-ribaudo · 2021-06-08T21:32:38Z

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

-      node.argument = this.parseMaybeAssign();
+    let delegating = false;
+    let argument = null;
+    if (!this.hasPrecedingLineBreak()) {


I'm curious about why

function* fn() { yield * [] }

is disallowed, it doesn't seem to be ambiguous 🤔

Good question. I have no idea why it is disallowed, maybe @bakkot can shred some light here?

That decision was before my time, so I can only speculate. I agree it would still be unambiguous without the NLTH restriction. My best guess is that it's future-proofing, to reserve the possibility of introducing * as a prefix operator.

nicolo-ribaudo · 2021-06-08T21:44:27Z

packages/babel-parser/src/plugins/jsx/index.js

+      } else if (type === tt.jsxTagEnd) {
+        const out = context.pop();
+        if ((out === tc.j_oTag && prevType === tt.slash) || out === tc.j_cTag) {
+          context.pop();


What are we popping out here? j_expr?

If out is j_oTag (<name></name>), context.pop is j_expr

If out is j_cTag (<name />), context.pop is the context before j_cTag, which is also the one before j_expr since we reinterpret j_expr, j_oTag as j_cTag when we see slash following jsxTagStart.

The logic here is was tt.jsxTagEnd.updateContext, I move it here so we can simplify the updateContext interface.

KFlash · 2021-06-09T10:30:41Z

How much memory does it consume to parse this? If I try to parse this on an Intel 386 sx / dx 16 mhz cpu, I will have no problems?

existentialism

❤️

JLHwung added PR: Internal 🏠 A type of pull request used for our changelog categories pkg: parser labels Jun 7, 2021

nicolo-ribaudo reviewed Jun 7, 2021

View reviewed changes

JLHwung force-pushed the reduce-exprAllowed-usage branch from f5898d2 to b5ce3e1 Compare June 8, 2021 02:41

JLHwung commented Jun 8, 2021

View reviewed changes

JLHwung force-pushed the reduce-exprAllowed-usage branch from b5ce3e1 to a787a7d Compare June 8, 2021 02:55

JLHwung added 17 commits June 8, 2021 10:39

refactor: scan and regexp in parseExprAtom

a12f921

remove function token context

f357e4f

remove parentheses token context

05b9a6d

chore: unify braceExpression and braceStatement

80a7adc

remove updateContext of tt.name

ba9d7c9

remove exprAllowed reset on tt.braceR

a5d9102

refactor: avoid depending on exprAllowed on parsing *

db4fb1c

remove exprAllowed from LookaheadState

7374676

merge recordExpression with brace

d581a94

remove isExpr from TokContext

56c41be

clean up braceR updateContext

e841c4e

refactor: parse solo yield from predefined token set

0970138

remove exprAllowed usage

fc0fce6

refactor: simplify updateContext interface

f542cf0

refactor: move exprAllowed tracking to jsx

66da823

chore: use arrow function for updateContext

63deead

update docs

930a7aa

JLHwung force-pushed the reduce-exprAllowed-usage branch from 403623a to 930a7aa Compare June 8, 2021 14:40

JLHwung commented Jun 8, 2021

View reviewed changes

nicolo-ribaudo reviewed Jun 8, 2021

View reviewed changes

JLHwung marked this pull request as ready for review June 8, 2021 14:48

JLHwung commented Jun 8, 2021

View reviewed changes

packages/babel-parser/src/plugins/jsx/index.js Outdated Show resolved Hide resolved

add more test cases

f2434cb

nicolo-ribaudo approved these changes Jun 8, 2021

View reviewed changes

existentialism approved these changes Jun 9, 2021

View reviewed changes

nicolo-ribaudo merged commit b9c1884 into babel:main Jun 9, 2021

nicolo-ribaudo deleted the reduce-exprAllowed-usage branch June 9, 2021 14:36

sync-by-unito bot mentioned this pull request Jun 10, 2021

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

Closed

This was referenced Jun 10, 2021

[Bug]: non-null assertion operator in comparison #13445

Closed

Disallow JSX tag forming after TS non-null assertion #13449

Merged

Simplify token context #13450

Merged

This was referenced Jun 14, 2021

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

Closed

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

Closed

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

github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce `exprAllowed` usage #13431

Reduce `exprAllowed` usage #13431

JLHwung commented Jun 7, 2021 •

edited

babel-bot commented Jun 7, 2021 •

edited

codesandbox-ci bot commented Jun 7, 2021 •

edited

nicolo-ribaudo left a comment

nicolo-ribaudo Jun 7, 2021

JLHwung Jun 8, 2021

KFlash Jun 9, 2021

JLHwung Jun 9, 2021 •

edited

JLHwung Jun 8, 2021 •

edited

nicolo-ribaudo Jun 8, 2021

JLHwung Jun 9, 2021

JLHwung Jun 8, 2021 •

edited

nicolo-ribaudo Jun 8, 2021 •

edited

JLHwung Jun 8, 2021

nicolo-ribaudo Jun 8, 2021

JLHwung Jun 9, 2021

bakkot Jun 9, 2021 •

edited

nicolo-ribaudo Jun 8, 2021

JLHwung Jun 9, 2021

KFlash commented Jun 9, 2021

existentialism left a comment

	slashAssign: new TokenType("_=", { beforeExpr, isAssign }),
	slashAssign: new TokenType("/=", { beforeExpr, isAssign }),

		@@ -0,0 +1,2 @@
		function *f1() { yield / 1 /g }
		function *f2() { yield /=2 /i }

Reduce exprAllowed usage #13431

Reduce exprAllowed usage #13431

Conversation

JLHwung commented Jun 7, 2021 • edited

babel-bot commented Jun 7, 2021 • edited

codesandbox-ci bot commented Jun 7, 2021 • edited

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLHwung Jun 9, 2021 • edited

Choose a reason for hiding this comment

JLHwung Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLHwung Jun 8, 2021 • edited

Choose a reason for hiding this comment

nicolo-ribaudo Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakkot Jun 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KFlash commented Jun 9, 2021

existentialism left a comment

Choose a reason for hiding this comment

Reduce `exprAllowed` usage #13431

Reduce `exprAllowed` usage #13431

JLHwung commented Jun 7, 2021 •

edited

babel-bot commented Jun 7, 2021 •

edited

codesandbox-ci bot commented Jun 7, 2021 •

edited

JLHwung Jun 9, 2021 •

edited

JLHwung Jun 8, 2021 •

edited

JLHwung Jun 8, 2021 •

edited

nicolo-ribaudo Jun 8, 2021 •

edited

bakkot Jun 9, 2021 •

edited