-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add allowYieldOutsideFunction
to parser
#17149
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
Add allowYieldOutsideFunction
to parser
#17149
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58824 |
@@ -273,7 +273,10 @@ export default abstract class ExpressionParser extends LValParser { | |||
): N.Expression { | |||
const startLoc = this.state.startLoc; | |||
if (this.isContextual(tt._yield)) { | |||
if (this.prodParam.hasYield) { | |||
if ( | |||
this.optionFlags & OptionFlags.AllowYieldOutsideFunction || |
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.
It should be chained with the !this.scope.inFunction
check, after all we don't allow yield
within a plain function.
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 just realize that the scope.inFunction
check will omit the static block, which is essentially a plain function body and thus should be disallowed in my opinion. Since a var scope can only be one of Program, Function, Static Block and TSModule, I suggest we append the production parameter when parsing Program/TSModule according to AllowYieldOutsideFunction
and AllowAwaitOutsideFunction
, and then the check for every specific yield/await can be restored to only production parameters.
This approach should be potentially faster as a typical source has only one program node and a few TSModule blocks, but it may contain lots of yield/await expressions.
I'm a bit hesitant to add a |
Would you prefer to add |
I prefer the specific option flag. The Unlike // template option
require('babel-template')(`await [foo]`, { template: false })(foo: identifier("foo"))
// allowAwaitOutsideFunction option
require('babel-template')(`await [foo]`, { allowAwaitOutsideFunction: false })(foo: identifier("foo")) And because of that, |
This reverts commit 324e089.
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.
Note that await and yield have different parsing rules.
Can you test that we can properly parse this code without error?
async function fn() {
yield + 1; // this is a sum between "yield" and 1
await + 1; // this is awaiting "+1"
}
@@ -2883,7 +2896,7 @@ export default abstract class ExpressionParser extends LValParser { | |||
return this.finishNode(node, "AwaitExpression"); | |||
} | |||
|
|||
isAmbiguousAwait(): boolean { | |||
isAmbiguousAst(): boolean { |
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 rename this to isAmbiguousPrefixOrIdentifier
?
The Key observations:
I suggest considering context simulation through parser state flags rather than literal "allow outside function" semantics. Would appreciate other perspectives on this interpretation. |
@magic-akari |
@magic-akari I agree that the name is a bit unfortunate, ideally I would like some option like Note that in your example the |
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.
Good that this also improves error recovery.
@liuxingbaoyu Could you prepare the docs PR? |
* allowYieldOutsideFunction * dts * review * template * Revert "template" This reverts commit 324e089. * improve error * review
Uh oh!
There was an error while loading. Please reload this page.