New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: async arrow functions should not be allowed after binary operator. #11284
Conversation
This commit makes Babel throw an error when parsing code like "3 + async() => 2".
): N.Expression { | ||
const state = { | ||
optionalChainMember: false, | ||
maybeAsyncArrow: this.atPossibleAsync(base), | ||
maybeAsyncArrow: this.atPossibleAsync(base) && !asyncArrowFuncForbidden, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify a bit to
maybeAsyncArrow: this.atPossibleAsync(base) && !asyncArrowFuncForbidden, | |
maybeAsyncArrow: this.atPossibleAsync(base) && startPos === this.state.potentialArrowAt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find your suggestion also easier to understand 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 Also, why is
from earlier in Lastly, what is a scenario where |
Great questions!
This is a very good one! I think we can remove var \u{61}sync
// base.name === "async" Oh I think
One of such scenarios is that the expression atom can be parenthesized (async);
// base.end points to 'c'
// this.state.lastTokEnd points to ')' |
@@ -748,7 +748,7 @@ export default class ExpressionParser extends LValParser { | |||
return this.finishNode(node, "TaggedTemplateExpression"); | |||
} | |||
|
|||
atPossibleAsync(base: N.Expression): boolean { | |||
atPossibleAsyncArrow(base: N.Expression): boolean { | |||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
)
Add an extra test to atPossibleAsync and refactor it to atPossibleAsyncArrow. This also fixes a bug in the Typescript plugin, so a new test has been added.
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 Should I add a test for async arrow functions after unary operators? |
If it was broken before, then yes please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Previously, code such as
1 + async () => 2
was parsed succesfully.The root cause of the issue is inside
parseExprAtom
, which is called byparseExprSubscripts
. Theswitch
statement matches tocase tt.name:
and returns an Identifier node with name "async" (skipping all the if stmts inside the case). Then,parseExprSubscripts
will reachreturn this.parseSubscripts(expr, startPos, startLoc);
, which parses the rest of the expression and returns anArrowFunctionExpression
.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 parsingasync() => 2
as an async function declaration. Instead, it will throw once it reaches the=
sign, because it will expect a semicolon since it is parsingasync()
as a function call (instead of a async arrow function declaration).