fix: dynamic Function/Generator/AsyncFunction constructor coercion gaps#476
fix: dynamic Function/Generator/AsyncFunction constructor coercion gaps#476dowdiness wants to merge 3 commits into
Conversation
…rcion gaps §20.2.1.1.1 CreateDynamicFunction — three gaps addressed: 1. GeneratorFunction/AsyncFunction: coerce params left-to-right (step 5a) and use interp.to_js_string() (ES ToString) for non-string arguments instead of MoonBit .to_string(). 2. Function: add .prototype.constructor wiring to dynamically created functions so that `new (new Function(...))` correctly sets up the prototype chain per §20.2.1.1.1 step 27a. 3. AsyncFunction [[Prototype]]: set to the Function constructor per §27.7.2, so Object.getPrototypeOf(AsyncFunction) === Function. 4. GeneratorFunction params: add raw-source-text check for `yield` as a keyword in param strings (§20.2.1.1.1 step 17a). The parser does not enter generator context for params, so yield is parsed as an identifier; the keyword check is applied before parse. All 42 GeneratorFunction and 34 AsyncFunction test262 tests now pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per §15.5.1 of the ES spec, default parameter expressions in a generator function's formal parameter list share the generator's yield context. Push generator context before parsing params so that `yield` in default values is correctly parsed as YieldExpr rather than as an identifier. This enables the AST-based §20.2.1.1.1 step 17a check to work correctly via params_contain_yield(), replacing the previous raw-text keyword scan which had false positives for `yield` in comments, string literals, and object binding property keys. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 12 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR updates generator constructor parsing to reject ChangesConstructor semantics updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 245bf2b56c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // §15.5.1: params and body share generator context so that `yield` in | ||
| // default-parameter expressions is parsed as YieldExpression, enabling | ||
| // the §20.2.1.1.1 step 17a Contains check via the AST. | ||
| self.push_generator_context(true) |
There was a problem hiding this comment.
With the generator context pushed before parsing parameters, parse_params_ext() now also parses object binding-pattern property names under in_generator(). That path uses expect_ident(), so valid declarations such as function* g({yield: x}) {} (and dynamic GeneratorFunction("{yield: x}", "")) now throw even though yield is only a property name, not a binding identifier or a yield expression. The parser needs to keep rejecting yield bindings/expressions while still allowing keyword property names in binding patterns.
Useful? React with 👍 / 👎.
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/28274914476
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
parser/stmt.mbt (1)
188-211: 🩺 Stability & Availability | 🟡 MinorExtended error recovery path leaks context on secondary parsing failures.
The generator/async contexts are pushed at lines 9–10. If
parse_params()fails with a specific error (e.g., "REST_PARAM_FOUND"), the code attempts recovery by callingparse_params_ext()andparse_block_body()(lines 17–19). However, if either of these calls raises an exception, the explicitpopoperations (lines 20–21) are bypassed, and the exception propagates.The surrounding
catchblock only wraps the initialparse_params()call; it does not guard the recovery logic inside theFailurearm. Consequently, any failure in the recovery path leaves the contexts pushed at lines 9–10 on the stack.The standard success path and the initial
parse_paramsfailure paths correctly balance pushes and pops, but this asymmetry corrupts the parser's context stack depth during error recovery.@@ -16,16 +16,23 @@ fn Parser::parse_generator_decl(self : Parser) -> `@ast.Stmt` raise Error { if msg == "REST_PARAM_FOUND" || msg == "DEFAULT_PARAM_FOUND" || msg == "DESTRUCTURE_PARAM_FOUND" { self.pos = saved_pos - let (ext_params, rest_param) = self.parse_params_ext() - // Context already pushed; parse body within the same context. - let body = self.parse_block_body() - self.pop_async_context() - self.pop_generator_context() - return GeneratorDeclExt( - name, - ext_params, - rest_param, - body, - tok.loc, - self.consume_source_text(tok.loc.offset), - ) + // Guard recovery calls to ensure context cleanup on error. + try { + let (ext_params, rest_param) = self.parse_params_ext() + let body = self.parse_block_body() + return GeneratorDeclExt( + name, + ext_params, + rest_param, + body, + tok.loc, + self.consume_source_text(tok.loc.offset), + ) + } + // Ensure contexts are popped if recovery fails. + finally { + self.pop_async_context() + self.pop_generator_context() + } } else { self.pop_async_context() self.pop_generator_context() raise `@errors.SyntaxError`(message=msg) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@parser/stmt.mbt` around lines 188 - 211, The recovery branch in the generator/async parsing flow leaves contexts pushed if parse_params_ext() or parse_block_body() throws, so update the parse logic in stmt.mbt to ensure pop_async_context() and pop_generator_context() run for every recovery attempt. Wrap the recovery work inside the same GeneratorDeclExt path with a cleanup guard or nested catch/finally-style handling so the contexts are always unwound before re-raising, and verify both the success and failure paths in the parse_params()/parse_params_ext() flow stay balanced.
🧹 Nitpick comments (1)
interpreter/stdlib/builtins_function.mbt (1)
425-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated prototype/back-reference setup.
The
FuncDecl,FuncDeclExt, and fallback paths now duplicate the same prototype object,"prototype"descriptor, and"constructor"descriptor wiring. A small helper would keep these spec-sensitive attributes synchronized if one branch changes later.Also applies to: 490-503, 507-522, 569-582, 588-646
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interpreter/stdlib/builtins_function.mbt` around lines 425 - 440, The prototype/back-reference wiring is duplicated across the FuncDecl, FuncDeclExt, and fallback paths, so extract that repeated setup into a shared helper. Move the common creation of the prototype object plus the "prototype" and "constructor" descriptors into one place, and have the affected branches call it from builtins_function.mbt so the spec-sensitive attributes stay consistent if one branch changes. Use the existing function/class symbols around the function-creation flow to centralize this logic without altering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interpreter/runtime/generator.mbt`:
- Around line 364-371: The generator-parameter yield validation in
GeneratorDeclExt is only catching the current AST shape and misses destructuring
and computed-key parameter forms that can also contain yield expressions. Update
params_contain_yield to recursively inspect all parameter patterns used by
generator params, or add equivalent validation in the GeneratorDeclExt handling
so every nested default, destructuring, and computed-key case is rejected
consistently before codegen.
- Around line 176-185: params_contain_yield only checks Param.default_val, so it
misses yield expressions nested inside destructuring patterns. Update this
verifier in generator.mbt to also traverse p.pattern and recurse through
ArrayPat/ObjectPat trees, checking DefaultPat expressions, PropPat.default_val,
and PropPat.computed_key with expr_contains_yield. Keep the existing top-level
default_val check, but add pattern traversal so GeneratorFunction cases like
destructured defaults and computed keys are rejected as SyntaxError.
---
Outside diff comments:
In `@parser/stmt.mbt`:
- Around line 188-211: The recovery branch in the generator/async parsing flow
leaves contexts pushed if parse_params_ext() or parse_block_body() throws, so
update the parse logic in stmt.mbt to ensure pop_async_context() and
pop_generator_context() run for every recovery attempt. Wrap the recovery work
inside the same GeneratorDeclExt path with a cleanup guard or nested
catch/finally-style handling so the contexts are always unwound before
re-raising, and verify both the success and failure paths in the
parse_params()/parse_params_ext() flow stay balanced.
---
Nitpick comments:
In `@interpreter/stdlib/builtins_function.mbt`:
- Around line 425-440: The prototype/back-reference wiring is duplicated across
the FuncDecl, FuncDeclExt, and fallback paths, so extract that repeated setup
into a shared helper. Move the common creation of the prototype object plus the
"prototype" and "constructor" descriptors into one place, and have the affected
branches call it from builtins_function.mbt so the spec-sensitive attributes
stay consistent if one branch changes. Use the existing function/class symbols
around the function-creation flow to centralize this logic without altering
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f47893d-f629-4173-8e46-1469856bc354
📒 Files selected for processing (4)
interpreter/runtime/async.mbtinterpreter/runtime/generator.mbtinterpreter/stdlib/builtins_function.mbtparser/stmt.mbt
… params Four targeted fixes from review feedback on PR #476: 1. parse_object_pattern: use expect_ident_name() for property keys so that `yield` is accepted as a LiteralPropertyName (ES §12.2.6) even in generator context. Fixes GF("{yield: x}", "return x") incorrectly throwing SyntaxError. 2. parse_generator_decl: wrap parse_params_ext() and parse_block_body() calls in inline catch blocks so push_generator_context / push_async_context are always popped on error, eliminating the parser context-stack leak. 3. pattern_contains_yield: new helper that recursively walks ArrayPat, ObjectPat, DefaultPat, and PropPat (computed_key + default_val) so that yield expressions nested inside destructuring defaults (e.g. {x = yield} or {[yield]: x}) are correctly caught. Property name strings are intentionally excluded. 4. builtins_function.mbt: extract make_function_prototype / wire_constructor_to_prototype helpers to deduplicate the identical prototype-object creation and constructor back-reference wiring across the FuncDecl, FuncDeclExt, and fallback return paths. GeneratorFunction: 42/42 test262, 2249/2249 unit tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e13e186b7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // §15.5.1: params and body share generator context so that `yield` in | ||
| // default-parameter expressions is parsed as YieldExpression, enabling | ||
| // the §20.2.1.1.1 step 17a Contains check via the AST. | ||
| self.push_generator_context(true) |
There was a problem hiding this comment.
Because this context is now pushed before parsing the outer parameter list, it also leaks into default-value subexpressions. parse_func_expr() and object-method parsing do not reset generator context until after their own parameters, so valid code such as GeneratorFunction("a = function(yield) {}", "") or GeneratorFunction("a = {m(yield){}}", "") now throws while parsing the nested non-generator parameter even though that nested function has its own yield context.
Useful? React with 👍 / 👎.
| // `yield` in generator context) is valid as an object binding property key. | ||
| _ => | ||
| ( | ||
| self.expect_ident_name(), |
There was a problem hiding this comment.
This correctly allows keywords as property keys for forms like {yield: x}, but the no-colon branch below reuses the same string as IdentPat(key). For shorthand object binding patterns the key is also a BindingIdentifier, so invalid code such as var {if} = {}; or function* g({yield}) {} is now accepted instead of throwing; only the explicit keyword: binding form should accept arbitrary IdentifierName keys.
Useful? React with 👍 / 👎.
| | AsyncGeneratorExprExt(_, _, _, _, _, _) | ||
| | ArrowFunc(_, _, _, _) | ||
| | ArrowFuncExt(_, _, _, _, _) | ||
| | ClassExpr(_, _, _, _, _) => false |
There was a problem hiding this comment.
A class expression is not a nested function boundary for the parameter Contains(YieldExpression) check: its heritage and computed member names are evaluated in the surrounding parameter scope. Returning false for the entire ClassExpr lets cases like GeneratorFunction("x = class extends (yield) {}", "") or computed class keys bypass the new params_contain_yield check and be accepted instead of raising the required SyntaxError at construction.
Useful? React with 👍 / 👎.
1 participant
Summary
Gap 1 (GeneratorFunction/AsyncFunction coercion order): Fix param/body coercion to apply left-to-right per §20.2.1.1.1 step 5a, and use
interp.to_js_string()(ESToString) instead of MoonBit.to_string()for non-string args.Gap 2 (GeneratorFunction yield-in-params): Detect
YieldExpressionin generator function params per §20.2.1.1.1 step 17a. Two-part fix: (a) push generator context before parsing params inparse_generator_declsoyieldin defaults is parsed asYieldExprnot an identifier; (b) walk the AST viaparams_contain_yieldto raiseSyntaxErrorwhen found.Gap 3 (Function
.prototype.constructorwiring): Add.prototypeobject with back-pointer.constructorto dynamically created functions per §20.2.1.1.1 step 27a. Applied to FuncDecl, FuncDeclExt, and the fallback arm inbuiltins_function.mbt.Gap 4 (AsyncFunction
[[Prototype]]): SetAsyncFunction.[[Prototype]] = Function(the constructor) per §27.7.2, soObject.getPrototypeOf(AsyncFunction) === Function.Test results
built-ins/GeneratorFunction: 42/42 (was 40/42 before;instance-yield-expr-in-param.jsnow passes in both modes)built-ins/AsyncFunction: 34/34 (was 32/34;AsyncFunction-is-subclass.jsnow passes)built-ins/Function: 796/824 (remaining failures are pre-existing issues unrelated to this PR)Test plan
make test262-filter FILTER="built-ins/GeneratorFunction"→ 42/42make test262-filter FILTER="built-ins/AsyncFunction"→ 34/34built-ins/Functionfailures🤖 Generated with Claude Code
Summary by CodeRabbit
yieldusage in default values.prototypeandconstructorbehavior.