fix: async arrow functions should not be allowed after binary operator.

[vedantroy]

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

Previously, code such as 1 + async () => 2 was parsed succesfully.

The root cause of the issue is inside parseExprAtom, which is called by parseExprSubscripts. The switch statement matches to case tt.name: and returns an Identifier node with name "async" (skipping all the if stmts inside the case). Then, parseExprSubscripts will reach return this.parseSubscripts(expr, startPos, startLoc);, which parses the rest of the expression and returns an ArrowFunctionExpression.

The issue is fixed by adding an extra parameter to this.parseSubscripts that indicates an async arrow function declaration is forbidden at the location. This prevents Babel from parsing async() => 2 as an async function declaration. Instead, it will throw once it reaches the = sign, because it will expect a semicolon since it is parsing async() as a function call (instead of a async arrow function declaration).

[JLHwung]

Can we simplify a bit to

Suggested change

	maybeAsyncArrow: this.atPossibleAsync(base) && !asyncArrowFuncForbidden,
	maybeAsyncArrow: this.atPossibleAsync(base) && startPos === this.state.potentialArrowAt,

[nicolo-ribaudo]

I find your suggestion also easier to understand 👍

[JLHwung]

I realize that atPossibleAsync is only used to detect whether there is an async arrow ahead. I suggest we rename atPossibleAsync to atPossibleAsyncArrow. And the condition base.start === this.state.potentialArrowAt can be added to current atPossibleAsync.

By doing so we can also fix another related case for free

4 + async<number>() => 2 
// This typescript snippet should throw but now it is parsed successfully

[vedantroy]

Fixed!

Also, out of curiosity/so I can better understand the parser, I have a few questions. Not really sure if this is the best place to ask them, but I'll give it a shot.

What does the noCalls parameter do in parseSubscripts?

Also, why is this.input.slice(base.start, base.end) === "async" needed if we know

base.type === "Identifier" &&
base.name === "async"

from earlier in atPossibleAsyncArrow

Lastly, what is a scenario where this.state.lastTokEnd would not equal base.end in atPossibleAsyncArrow?

[JLHwung]

Great questions!

What does the noCalls parameter do in parseSubscripts?

noCalls is used to indicate whether a left parenthesis after an identifier should be considered as a CallExpression. i.e. When parsing a NewExpression new C(), we set noCalls to be true so the ( after C is not a CallExpression.

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

Line 1435 in 2e6f958

node.callee = this.parseNoCallExpr();

Why is this.input.slice(base.start, base.end) === "async" needed if we know (the other two conditions)

This is a very good one! I think we can remove base.name === "async" check because this.input.slice(base.start, base.end) === "async" && base.type === "Identifier" implies base.name === "async". However the opposite direction can't follow because identifier may contain escaped sequence:

var \u{61}sync
// base.name === "async"

Oh I think base.name should be faster than this.input.slice. I think we can check base.end - base.start === 5 instead, see my comments at #11284 (comment).

What is a scenario where this.state.lastTokEnd would not equal base.end in

One of such scenarios is that the expression atom can be parenthesized

(async);
// base.end points to 'c'
// this.state.lastTokEnd points to ')'

[JLHwung]

The conditions here corresponds to atPossibleAsync. I was meant to add base.start === this.state.potentialArrowAt here so it can be rebranded as atPossibleAsyncArrow.

atPossibleAsyncArrow(base: N.Expression): boolean {
  return (base.type === "Identifier" &&
  base.name === "async" &&
  this.state.lastTokEnd === base.end &&
  !this.canInsertSemicolon() &&
  base.end - base.start === 5 &&
  base.start === this.state.potentialArrowAt)
}

The first 5 conditions are about atPossibleAsync and the last one is hasPossibleArrow so it combines into atPossibleAsyncArrow.

[vedantroy]

I think I understand. I'll add the extra condition to this atPossibleAsyncArrow method, and delete the atPossibleAsync method. This means I will also change

if (!noCalls && this.atPossibleAsync(base)) {

in the Typescript plugin to

if (!noCalls && this.atPossibleAsyncArrow(base)) {

[nicolo-ribaudo]

If you are going with the base.end - base.start === 5 route, could you add a comment in the code that it's needed to check that there are no escape sequences?

[JLHwung]

Half joking: If this.state.lastTokEnd >= base.end && base.start >= this.state.potentialArrowAt is always true (I don't know). We can even further simplify it to

return (base.type === "Identifier" &&
  base.name === "async" &&
  !this.canInsertSemicolon() &&
  this.state.lastTokEnd - this.state.potentialArrowAt === 5)
)

[vedantroy]

Updated. I also added a test for Typescript, since this PR will fix a bug there too.

Edit: I just realized this PR fixes async arrow functions after unary operators. So things like void async () => 1 will no longer be allowed.

Should I add a test for async arrow functions after unary operators?

[nicolo-ribaudo]

Should I add a test for async arrow functions after unary operators?

If it was broken before, then yes please!

[nicolo-ribaudo]

Thanks!