Repository navigation
js_parser: reject yield/await/this/return that leak into TS enum and namespace bodies - #34250
Conversation
…espace bodies An enum or namespace body is lowered into a non-async, non-generator arrow IIFE, so yield/await/this/return from an enclosing function or the module top-level must not be accepted there. Bun parsed them with the inherited context and emitted output that is a load-time SyntaxError in the consuming engine. Match esbuild: save fn_or_arrow_data_parse, replace it with a default struct (is_this_disallowed for both, is_return_disallowed for namespaces), restore after the body.
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 4:03 PM PT - Jul 15th, 2026
❌ @autofix-ci[bot], your commit 3b34d0f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34250That installs a local version of the PR into your bun-34250 --bun |
|
Arrived at the identical src/ fix from a separate handoff (entry point was |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the save/replace/restore of fn_or_arrow_data_parse mirrors esbuild and matches the existing pattern in parse_property.rs and parse_fn.rs. Deferring to a human because this changes parser accept/reject behavior in a core path.
Checked: FnOrArrowDataParse::default() sets allow_await/allow_yield to AllowIdent as the PR relies on; the non-RAII restore skipping on ? early-return matches how the sibling has_non_local_export_declare_inside_namespace save/restore already behaves (parser is bailing anyway); nested functions overwrite the field themselves so the positive test cases hold; enum omits is_return_disallowed because initializers are expression-position.
Extended reasoning...
Overview
Two-site parser fix in src/js_parser/parse/parse_typescript.rs: parse_type_script_namespace_stmt and parse_typescript_enum_stmt now save p.fn_or_arrow_data_parse, replace it with a fresh FnOrArrowDataParse { is_this_disallowed: true, [is_return_disallowed: true,] ..Default::default() }, and restore it after parsing the body. This prevents the enclosing function/module's allow_yield/allow_await/is_top_level context from leaking into enum initializers and namespace bodies, which are lowered into arrow IIFEs where yield/await/this/return are invalid. 18 new assertions in test/bundler/transpiler/transpiler.test.js cover both the newly-rejected inputs and the still-accepted nested-function cases.
Security risks
None. This is a parser context-tracking fix; no untrusted-input allocation, no I/O, no boundary crossings.
Level of scrutiny
Medium-high. The change itself is small and mechanical — the exact save/clone/replace/restore idiom already appears at parse_property.rs:499-513 and parse_fn.rs:494-517, and it directly mirrors esbuild's ts_parser.go. But it lives in the core TS parser and changes which inputs are accepted vs. rejected. Code that previously transpiled (to broken output) will now hard-error, and one previously-rejected valid input (namespace x { let await = 1; }) is now accepted. That's the intended behavior and matches tsc/esbuild, but accept/reject changes in the parser are the kind of thing a maintainer should sign off on.
Other factors
- I confirmed
FnOrArrowDataParse::default()(parser.rs:1568) setsallow_await/allow_yieldtoAllowIdent, which is what the error paths inparse_prefix.rskey on for the quoted diagnostics. - The restore is skipped on
?-propagated errors between save and restore (e.g. thereturn Err(SyntaxError)in the enum member-name branch). This is consistent with the pre-existinghas_non_local_export_declare_inside_namespacesave/restore in the same function and with the otherfn_or_arrow_data_parsesave/restore sites in the codebase — on a syntax error the parse is aborting, so stale state is inconsequential. - Enum initializers are expression-position, so omitting
is_return_disallowedthere (vs. setting it for namespaces) is correct and matches esbuild. - Test coverage is thorough: yield-in-generator, await-in-async, TLA,
thisin class method,declare enum, dotted namespace,for await, plus positive cases proving nested generators/async arrows andreturninside exported namespace functions still work. - The bug-hunting system reported no findings.
…2e59e6 Arrow-inherited this, module alias, declare forms, and class method/field positive cases proving is_this_disallowed does not over-reject.
|
Folded in the complementary cases from |
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the restore of fn_or_arrow_data_parse is skipped on ? early-returns, but that matches the existing has_non_local_export_declare_inside_namespace save/restore in the same function and parser errors are terminal so the unrestored state is never read. The other fields zeroed by Default::default() (allow_super_*, is_constructor, track_arrow_arg_errors, allow_missing_body_for_type_script, allow_ts_decorators) are per-function-entry transients not consulted across statement boundaries, so is_top_level is the only cross-statement consumer affected.
Extended reasoning...
The src/ change is a faithful port of esbuild's fnOrArrowDataParse reset in ts_parser.go and is well-covered by 26 new assertions (both rejection and positive nested-function cases). I'm not approving because this is parser hot-path code and the inline nit documents a real — if narrow — behavioral change to the Bun-specific React-hooks-suppression consumer of is_top_level; a maintainer should decide whether to carry is_top_level across the reset or accept the nit as-is.
parse_fn.rs reads is_top_level from the enclosing context to decide whether to consume a next-line react-hooks suppression after a namespace member function. Every other is_top_level consumer is gated on allow_await == AllowExpr, which the reset forces to AllowIdent, so preserving the flag only keeps the react-hooks behavior at parity with main.
There was a problem hiding this comment.
LGTM — the is_top_level carry-over in 3820fe6 addresses the react-hooks-suppression concern from my earlier pass, and the author's audit of the other three is_top_level readers (all gated on allow_await == AllowExpr) checks out.
Checked: save/restore of fn_or_arrow_data_parse mirrors esbuild and the neighboring has_non_local_export_declare_inside_namespace pattern (restore-on-error is not needed — the P struct is not reused after a parse error). Default::default() sets allow_await/allow_yield to AllowIdent and allow_super_* to false, which is what the lowered arrow IIFE requires. Tests cover both new rejections and the positive cases (nested generator/async, class this, let await binding) proving no over-rejection.
Extended reasoning...
Overview
Two-site parser fix in src/js_parser/parse/parse_typescript.rs: on entry to a TS namespace or enum body, save p.fn_or_arrow_data_parse, replace it with a fresh struct (is_this_disallowed: true, plus is_return_disallowed: true for namespaces and is_top_level carried over from the enclosing context), and restore after the body. This prevents yield/await/this/return/for await from the enclosing function/module context leaking into the arrow IIFE that the enum/namespace lowers to. 26 assertions added to test/bundler/transpiler/transpiler.test.js.
Security risks
None. This tightens the parser to reject inputs that previously produced load-time-SyntaxError output, and loosens one case (namespace x { let await = 1; }) to match esbuild/tsc. No untrusted-size arithmetic, no allocation changes, no FFI.
Level of scrutiny
Medium. The parser is production-critical, but the change is a direct port of esbuild's ts_parser.go save-assign-restore pattern, applied at exactly two call sites, and follows the local convention already established one line above by the has_non_local_export_declare_inside_namespace save/restore. I audited every field of FnOrArrowDataParse against its Default impl and every reader of is_top_level in src/js_parser/ — the author's claim that the three non-react-hooks readers are gated behind allow_await == AllowExpr (which the reset forces to AllowIdent) is accurate, so carrying is_top_level over affects only the intended parse_fn.rs consumer.
Other factors
- My previous inline nit about
is_top_levelzeroing was addressed in commit 3820fe6 exactly as suggested; the thread is resolved. - The enum reset intentionally does not carry
is_top_level— the author's justification (enum initializers cannot contain function declarations; function expressions there aren't top-level for React Compiler purposes) is sound and was explicitly considered. - Not restoring on the
?error path is consistent with the adjacenthas_non_local_export_declare_inside_namespacehandling and is safe because the parser does not resume after a returnedError. - Test coverage is thorough: error cases across enum/namespace/
module/dotted/declarevariants, plus positive cases (nested generator, nested async arrow, class method/fieldthis,let awaitbinding,returninside a namespace-member function) proving the reset does not over-reject. Full-file andesbuild/ts.test.tsruns pass.
…rage Carry is_top_level into the enum reset as well: enum initializers can contain arrows and function expressions, which also reach the react-hooks suppression consume in parse_fn.rs. Tests now also cover super in enum initializers (allow_super_* is zeroed by the reset), const enum and async-arrow enclosures, and the restore of the enclosing context after an enum/namespace body (sibling yield/await/ super). A react-compiler case pins that a next-line react-hooks suppression inside a namespace member does not bail the compiler out of a following top-level component.
There was a problem hiding this comment.
LGTM — the is_top_level carry-through addresses the react-hooks-suppression concern from the earlier review, and the new SuppressionInsideTSNamespaceDoesNotLeak test locks it in.
Re-checked after the follow-up commits:
- The three other
is_top_levelreaders (parse_prefix.rs:166,parse_stmt.rs:510,mod.rs:879) are all insideallow_await == AllowExprbranches, which the reset forces toAllowIdent— so preservingis_top_levelcannot spuriously settop_level_await_keywordfrom inside a namespace/enum body. - The remaining
FnOrArrowDataParsefields zeroed byDefault(async_range,is_constructor,track_arrow_arg_errors, decorator flags, etc.) are all per-function state that should not leak into the lowered arrow — matches esbuild's zero-valued reset. - Enum restore placement before
expect(TCloseBrace)is fine; the unrestored-on-?error path matches the neighboringhas_non_local_export_declare_inside_namespacepattern.
Extended reasoning...
Overview
Two save/assign/restore blocks around fn_or_arrow_data_parse in parse_type_script_namespace_stmt and parse_typescript_enum_stmt, so the enclosing function's allow_yield/allow_await/allow_super_*/is_return_disallowed context does not leak into bodies that lower to arrow IIFEs. This is a direct port of esbuild's ts_parser.go behavior, with one Bun-specific deviation: is_top_level is carried across the reset to keep the react-hooks-suppression consume in parse_fn.rs working. ~21 lines of src/ change plus ~90 lines of tests across transpiler.test.js (error/positive/restore matrix) and one itBundled regression test in react-compiler.test.ts.
Security risks
None. Pure parser-context manipulation of a POD struct via .clone() and reassignment; no allocation, no untrusted-input arithmetic, no FFI.
Level of scrutiny
Moderate — js_parser is a hot, correctness-critical path, but the change is a small, localized save/restore that mirrors the reference implementation. The one non-obvious interaction (Bun's react-hooks-suppression consumer of is_top_level) was flagged in my prior review, fixed exactly as suggested, and now has a dedicated bundler test. I re-verified against the FnOrArrowDataParse struct definition that no other zeroed field has a Bun-specific consumer that would regress: the other three is_top_level readers all sit behind allow_await == AllowExpr, which the reset makes unreachable, and the remaining fields are per-function-signature state that correctly resets.
Other factors
Test coverage is thorough: yield/await/this/super/return rejections across enum, const enum, declare enum, namespace, dotted namespace, module x, and declare namespace; positive cases proving nested functions/arrows/class-methods re-establish their own context; and explicit restore assertions (yield after an enum, TLA after a namespace, super.foo() after an enum). The namespace x { let await = 1; } case covers the opposite-polarity fix. Verified fails-on-main / passes-on-PR in the description. My earlier inline comment is resolved and the thread is closed.
|
Gate has passed on every push. Review surfaced four concerns, all addressed in 9548516:
35 assertions in CI has run three times (73322 / 73345 / 73393) and the only reds are |
### What `#34249` changed TypeScript enum lowering so that only **module-scope** enums emit `var`; an enum in a function, method, or block body now emits `let`. `#34250` merged four minutes earlier and added `it("rejects yield/await/this/super in enum initializers")`, whose expectations still assert `var x` for **block-scoped** enums. Each PR was green against a `main` that lacked the other's change, so the collision only appeared once both had landed — and since `main` pushes run no test shards, nothing caught it. The result is that `main` asserts output its own parser no longer produces. `test/bundler/transpiler/transpiler.test.js` currently fails on every PR that merges `main`. ### The fix Updates the five stale expectations to `let`. All five are enums nested in a function or method body: | Line | Case | |---|---| | 771 | `function *f() { enum x { y = (function*() { yield 1 })() } }` | | 775 | `async function f() { enum x { y = (async () => await 1)() } }` | | 785 | `function *f() { enum x { y = 1 } yield 1; }` | | 789 | `async function f() { enum x { y = 1 } await 1; }` | | 793 | `class C extends B { m() { enum x { y = 1 } super.foo(); } }` | Deliberately unchanged: - **Line 779** — `enum x { y = (function() { return this })() }` is top-level, so `var` is still correct. - **Namespace expectations** — namespaces only appear at module scope or nested in another namespace, where both the old and new predicates agree. ### Note for reviewers Only the line-771 failure is visible in CI: `expectPrinted_` throws at the first mismatch, masking the other four. A fix touching only the reported line would go red again on the next one, so all five are updated together. ### Verification Ran `test/bundler/transpiler/transpiler.test.js` against a build containing `#34249`: - pristine `main`: 176 pass / 4 fail - with this change: 177 pass / 3 fail The change flips exactly the one enum test and touches nothing else. The 3 remaining failures are unrelated to this diff — they are skew between that build and two commits that landed after it (`#34254`, `#34258`), and build from source in CI. Also swept the repo for any other assertion of enum-lowering text (the closure IIFE shape `(x ||= {})` / `(x = x || {})`). Only two files assert it: this one, and `test/js/node/module/require-extensions.test.ts:129`, whose fixture declares a top-level enum and is correctly `var`.
2 participants
Repro
yieldinside a non-generator arrow is a load-timeSyntaxErrorin every engine. esbuild and tsc both reject the input:Siblings that emit the same kind of invalid output on main:
async function f() { enum x { y = await 1 } }await 1enum x { y = await 1 }(top-level await)await 1class C { m() { enum x { y = this } } }this(wrong binding)namespace x { await 1; }await 1namespace x { return 1; }return 1namespace x { for await (const y of []); }for awaitand one case of the opposite polarity:
namespace x { let await = 1; }is valid TS that esbuild accepts but bun rejects, because the module-levelForbidAllforawaitleaked into the body.Cause
parse_typescript_enum_stmtparses initializers withp.parse_expr(Level::Comma)andparse_type_script_namespace_stmtparses the body withp.parse_stmts_up_to(...), neither resettingp.fn_or_arrow_data_parsefirst.allow_yield,allow_await,allow_super_*,is_top_level,is_this_disallowedandis_return_disallowedall inherit from the enclosing function / module, soyield/awaitparse as expressions and end up inside the lowered arrow.esbuild saves
fnOrArrowDataParse, assigns a fresh zero-valued struct (isThisDisallowed: true, plusisReturnDisallowed: truefor namespaces), and restores it after the body (ts_parser.go).Fix
Mirror esbuild in both sites: save
fn_or_arrow_data_parse, replace withFnOrArrowDataParse { is_this_disallowed: true, ..Default::default() }(andis_return_disallowed: truefor namespaces), restore after the body. TheDefaultsetsallow_await/allow_yieldtoAllowIdent, so the existing recovery inparse_prefix.rsreportsCannot use "yield" outside a generator function/"await" can only be used inside an "async" function.Nested functions inside an initializer establish their own context (they overwrite
fn_or_arrow_data_parsethemselves), soenum x { y = (function*() { yield 1 })() }andnamespace x { export const y = async () => await 1; }remain valid.Verification
Two new
it()blocks intest/bundler/transpiler/transpiler.test.js(18 assertions) covering the enum and namespace cases above, plus the dottednamespace x.y { ... }form anddeclare enum.test/bundler/transpiler/transpiler.test.jsfull file: 173 pass, 0 fail.test/bundler/esbuild/ts.test.ts: 57 pass, 0 fail.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 0
evidence per changed file