Skip to content

Conversation

liuxingbaoyu
Copy link
Member

@liuxingbaoyu liuxingbaoyu commented Feb 24, 2025

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

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 24, 2025

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

@JLHwung JLHwung added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser PR: Needs Docs labels Feb 24, 2025
@@ -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 ||
Copy link
Contributor

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.

Copy link
Contributor

@JLHwung JLHwung Feb 25, 2025

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.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.27.0 milestone Feb 24, 2025
@liuxingbaoyu
Copy link
Member Author

I'm a bit hesitant to add a template option, because of #9975.
Otherwise we'll just have to add a ton of options.

@liuxingbaoyu
Copy link
Member Author

Would you prefer to add allowYieldOutsideFunction and allowPrivateNameOutSideClass or add a template?

@JLHwung
Copy link
Contributor

JLHwung commented Mar 5, 2025

I prefer the specific option flag. The template option seems like the loose option of other Babel plugins that we have tried to get rid of in the favor of assumptions.

Unlike allowReturnOutsideFunctino, both allowAwaitOutsideFunction and allowYieldOutsideFunction potentially parse the same snippet into different AST: e.g. await [foo] can be either an AwaitExpression or a MemberExpression. Though we may speculate that the former is more desired, the template usage example for the latter might be a bit confusing:

// 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, allowAwaitOutsideFunction may still have to interact with the template option: One may want top level yield but not top level await. Then we will have to define the precedence of template over specific options, which sounds like a deep rabbit hole for me.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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 {
Copy link
Member

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?

@magic-akari
Copy link
Contributor

The allowYieldOutsideFunction option seems potentially misleading. Based on the original issue, the core requirement appears to be enabling parser state management to simulate being within generator/async function contexts - rather than literally allowing yield/await outside functions.

Key observations:

  1. yield expr parsing requires assuming generator function context.
  2. await expr parsing needs async function context simulation (including TLA as pseudo-async).
  3. Current examples like:
    function foo() { yield 1; }
    function bar() { await 2; }
    don't represent the intended use cases.

I suggest considering context simulation through parser state flags rather than literal "allow outside function" semantics. Would appreciate other perspectives on this interpretation.

@liuxingbaoyu
Copy link
Member Author

liuxingbaoyu commented Mar 7, 2025

@magic-akari
As I understand it the use case here is to allow the use of yield at the top level to support require('babel-template')('yield A').
allowYieldOutsideFunction only allows the use of yield at the top level.
function foo() { yield 1; } is still rejected.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 7, 2025

@magic-akari I agree that the name is a bit unfortunate, ideally I would like some option like template/context that is context: { generator: boolean; async: boolean; function boolean; }. However, we already have allowReturnOutsideFunction/allowAwaitOutsideFunction/allowNewTargetOutsideFunction, and this option is just continuing that pattern.

Note that in your example the yield inside function foo() { yield 1; } is not outside of a function, thus not affected by the option :)

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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.

@nicolo-ribaudo nicolo-ribaudo requested a review from JLHwung March 20, 2025 10:02
@nicolo-ribaudo
Copy link
Member

@liuxingbaoyu Could you prepare the docs PR?

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Mar 20, 2025
@nicolo-ribaudo nicolo-ribaudo merged commit 36275df into babel:main Mar 21, 2025
55 checks passed
laine-hallot pushed a commit to laine-hallot/uwu-parser that referenced this pull request Mar 31, 2025
* allowYieldOutsideFunction

* dts

* review

* template

* Revert "template"

This reverts commit 324e089.

* improve error

* review
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 21, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Babel Template Yield statement fails
5 participants