Repository navigation
Conversation
|
Updated 10:52 PM PT - Aug 14th, 2026
⏳ @robobun, your commit ccae661 is still building in
|
|
Status: reproduced on the released build ( CI for ccae661 (build 97432, finished): 177 jobs passed, including every test lane that ran and the debug/ASAN lanes that exercise the new assertion; no test failed outside of known flaky ones that passed on retry. The two non-green jobs are infrastructure: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughTypeScript enum visitation now captures declarations hoisted from enum initializers, restores the previous parser context, and prepends the declarations to generated enum statements. Decorator transpiler tests cover class expressions, isolation, runtime behavior, and temporary placement. ChangesEnum initializer lowering
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small and follows the existing visit_stmts pointer pattern exactly, but since it touches the parser's raw-pointer nearest_stmt_list machinery and changes where lowered temporaries are scoped, a human look would still be worthwhile.
Checked: no ?/early return between installing the pointer and restoring it (the member-initializer loop is infallible); hoisted_stmts is never touched directly while the pointer is live, so the addr_of_mut! tag stays valid; the pointer is restored before hoisted_stmts is moved into value_stmts. Confirmed StmtList<'a> and ListManaged<'a, Stmt> are both BumpVec<'a, Stmt>, so the NonNull type lines up. The other nearest_stmt_list consumer (react refresh) would now emit into the enum closure too, which is the correct scope for anything inside an enum body.
Extended reasoning...
Overview
Fixes a codegen bug where the standard-decorator lowering's hoisted var (for _dec, _init, _class, accessor WeakMaps) is dropped or emitted in the wrong scope when the class expression appears inside a TypeScript enum member initializer. The fix has s_enum install a local BumpVec as p.nearest_stmt_list while visiting member initializers, then prepends that list to the closure the enum body compiles to. Touches src/js_parser/visit/visit_stmt.rs (~15 lines), doc comments in src/js_parser/p.rs, and adds 4 tests to test/bundler/transpiler/es-decorators.test.ts (plus moves runDecoratorTS to module scope so the new describe block can reuse it).
Security risks
None. This is a transpiler codegen fix; no untrusted-input parsing changes, no allocation sizing driven by user data, no FFI surface changes.
Level of scrutiny
Medium-high. The change is small and follows the exact save/install/restore pattern already used in visit_stmts (src/js_parser/visit/mod.rs:1343-1540), but it lives in the JS parser — a hot, correctness-critical path — and manipulates a raw NonNull back-pointer whose safety depends on there being no early return or direct reborrow of the local while the pointer is installed. I verified those invariants hold (the member loop between install and restore contains no ? and never names hoisted_stmts), and that the restore happens before the local is moved. The scoping decision (emit into the enum closure rather than the enclosing list) is well-argued in the PR description and matches how namespace bodies already behave, but it is a semantic choice a maintainer should confirm.
Other factors
The PR description is unusually thorough — it names the mechanism (preprocessed_enums runs before nearest_stmt_list is installed at visit/mod.rs:1348), explains why the enum closure is the right scope (matches the Kind::Entry scope the temporaries are registered in, and how namespaces already work), and lists the regression suites run. Tests cover the top-level case (dropped var), the nested-function case (shared temporaries across calls), sibling enums, and a Bun.Transpiler shape assertion that pins the var inside the closure and asserts it is absent at module level. Tests follow harness conventions (tempDir, drained pipes, stderr-before-exitCode, test.concurrent for the subprocess cases). No prior reviews or outstanding comments on the PR.
…e the enum closure Lowering a class expression (standard decorators or an accessor field) appends a var declaring its temporaries to p.nearest_stmt_list. While an enum's members are visited that pointer still refers to the list around the enclosing visit_stmts, because enums are visited before their sibling statements: at the top level it is None and the declaration was dropped (ReferenceError: _dec is not defined at runtime), and inside a function it pointed at the list enclosing the function, so every call shared one set of temporaries. s_enum now collects those declarations itself and emits them at the top of the closure the enum body compiles to, which gives each evaluation of each enum its own bindings.
0b0a02e to
aea29e9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the earlier comment nit is addressed (single-line, no past-tense narration).
What was reviewed:
s_enumsave/restore ofnearest_stmt_listmatches the existingvisit_stmtspattern (addr_of_mut!on a stack local, restored before the local is moved); no early return /?between install and restore.- Confirmed the
preprocessed_enumsloop invisit/mod.rsruns beforenearest_stmt_listis installed, which is why the enclosing list can't be reused here. p.rschanges are comment-only;runDecoratorTSis a mechanical hoist to file scope; new tests cover top-level / nested-in-function / sibling enums plus a transpiler output-shape check.
Extended reasoning...
Overview
The PR fixes a runtime ReferenceError when a class expression that requires standard-decorator lowering (a decorated class or one with an accessor field) appears inside a TypeScript enum member initializer. The lowering hoists a var for its temporaries into p.nearest_stmt_list, but enums are pre-visited in visit_stmts before that pointer is installed, so at the top level the pointer is None and the declaration is silently dropped, and inside a function it targets the wrong (outer) scope so all evaluations share one set of temporaries.
The Rust change in s_enum (src/js_parser/visit/visit_stmt.rs:2194-2327) is ~10 lines: allocate a local hoisted_stmts list, point nearest_stmt_list at it via addr_of_mut! while enum member initializers are visited, restore the previous pointer, then reuse hoisted_stmts as the head of value_stmts so the declarations land at the top of the enum closure passed to generate_closure_for_type_script_namespace_or_enum. The p.rs change is a one-line doc-comment update on nearest_stmt_list (adds s_enum alongside visit_stmts) plus removal of a redundant doc paragraph on nearest_stmt_list_mut (the SAFETY block already carries the invariant). The test file hoists runDecoratorTS to file scope (mechanical move, unchanged body) and adds 4 tests.
Security risks
None. This is transpiler visit-order logic; no user-controlled input reaches new unsafe code, no network/filesystem/auth surface. The one unsafe this touches is the pre-existing nearest_stmt_list_mut accessor, and the new install site follows the exact same pattern already used in visit/mod.rs:1343-1348 — stack-local BumpVec, addr_of_mut! (no intermediate &mut so the pointer's provenance survives), saved/restored around the loop. I traced lines 2199-2319 for early exits: no ?, no return, so the restore always runs before hoisted_stmts is moved into value_stmts. The local is not reborrowed directly while the pointer is installed.
Level of scrutiny
Medium. The parser is a hot/critical path, but the change is small, targeted (only the enum-member-initializer code path), and copies an established idiom line-for-line from visit_stmts. The PR description's argument for placing the var inside the enum closure (matching the enum body's Kind::Entry scope, the same behavior a namespace body already has) is sound and confirmed by reading visit/mod.rs — the preprocessed_enums loop indeed runs before nearest_stmt_list is set. No CODEOWNERS entry gates src/js_parser.
Other factors
- All prior review threads (comment-cop bot × 5, my earlier past-tense-narration nit) are resolved; the current diff has the one-line comment without the "used to" clause.
- The 4 new tests cover the variant matrix REVIEW.md asks for: top-level enum (both a decorated class and an
accessorclass in one enum), enum inside a function called twice (asserts fresh temporaries per call — the WeakMap-sharing failure mode), two sibling top-level enums, and aBun.Transpileroutput-shape test asserting thevaris inside the closure and not at module level for both top-level and nested cases. Tests usetempDir, drain stdout/stderr/exited concurrently, assert stderr before exitCode, and the three subprocess tests aretest.concurrent. - The PR description reports the tests fail on the released build and pass with the fix, and lists the existing suites that were run (es-decorators, esbuild/ts, react-compiler-fixtures — the other
nearest_stmt_listconsumer, etc.).
…ct refresh in enums With s_enum installing its own list there is no longer a visit in which nearest_stmt_list is None, so reading it through nearest_stmt_list_mut() now debug-asserts instead of letting a future caller silently drop the declaration again; the react refresh sites go through the accessor too (their release fallback is unchanged). Tests: the react refresh signature of a hook function inside a top-level enum is declared inside the enum closure (it used to be referenced but never declared), and the enum tests use decorated accessors so they keep exercising the hoisting path if undecorated accessors stop hoisting.
There was a problem hiding this comment.
LGTM — the save/install/restore of nearest_stmt_list in s_enum mirrors the existing pattern in visit_stmts exactly, and the hoisted declarations land in the closure that owns the scope the temporaries were registered in.
What was reviewed:
- Traced the pointer lifetime in
s_enum: no?/early return between install (line 2197) and restore (line 2321);hoisted_stmtsis only moved after the pointer is restored, so no dangling. - Confirmed the
visit_expr.rschange (routing.is_none()throughnearest_stmt_list_mut()) is intentional so the newdebug_assert!covers the react refresh reader too; the guard remains in release builds. - Checked that the preprocessed-enum loop in
visit/mod.rs:1319-1336runs beforenearest_stmt_listis installed, matching the PR's root-cause analysis. - Tests cover the full class: top-level, function-nested (per-call temporaries), sibling enums, react refresh, and negative
Bun.Transpilerassertions on where thevaris NOT emitted.
Extended reasoning...
Overview
The PR fixes a bug where the standard-decorator lowering's hoisted var declarations (_dec, _init, _class, accessor WeakMaps) were silently dropped when a class expression appeared inside a top-level TypeScript enum initializer, causing ReferenceError at runtime. The mechanism: visit_stmts visits enum statements in a preprocessing loop (mod.rs:1319-1336) before it installs its before list into p.nearest_stmt_list (mod.rs:1348), so lower_decorators.rs:2531 found None (top level) or the wrong enclosing list (nested).
The fix has s_enum install a local hoisted_stmts list as nearest_stmt_list around the member-initializer visit and prepend that list to the enum closure's body statements. Plus: a debug_assert! in nearest_stmt_list_mut() so any future path that reads it while unset panics in debug builds, and the two react-refresh readers in visit_expr.rs are routed through the accessor so they're covered by the assert.
Security risks
None. This is a compile-time transform of user source; the change only affects where a generated var statement is placed in emitted output. No new inputs are parsed and no untrusted data flows into new code paths.
Level of scrutiny
Medium-high — parser/visitor changes affect every transpiled file, and the mechanism uses a raw NonNull back-pointer to a stack local. However, the pattern is a byte-for-byte copy of the existing one in visit_stmts (addr_of_mut!, save/restore, no direct use of the local while the pointer is installed, no early return in between). I traced the loop body (lines 2200-2319) — no ? operators, and hoisted_stmts is not touched directly until after the pointer is restored at line 2321 and then moved into value_stmts at line 2326. The pointer targets the stack BumpVec struct, not its buffer, so pushes during visit_expr don't invalidate it.
Other factors
- All prior review threads (comment-cop bot on comment length, and my own earlier nit about past-tense narration in the visit_stmt.rs comment) are resolved; the current diff has the one-line comment.
- Tests are thorough per REVIEW.md's "cover the variant matrix": top-level enum (the repro), enum-in-function called twice (proves per-evaluation temporaries), two sibling enums (proves no cross-enum sharing under the renamer), react refresh inside an enum (the other
nearest_stmt_listconsumer), and aBun.Transpilertest with both positive (var declared inside((E) => {) and negative (^varnot at module scope) assertions.runDecoratorTSwas hoisted to file scope unchanged. - The
debug_assert!is a good hardening move — it converts the previously-silent-drop into a debug-build panic, and REVIEW.md's "new invariant checks on existing code default to debug-only" is satisfied. - The fix is at the layer that owns the invariant (the enum body is its own
Kind::Entryhoisting scope, so declarations hoisted out of its initializers belong in its closure), matching how namespace bodies already behave. - No CODEOWNERS entry for
js_parser.
Problem
accessorfield) inside a member initializer of a top-level TypeScript enum fails at runtime:ReferenceError: _dec is not defined(or_x is not definedfor an accessor namedx). The same class expression outside the enum works.vardeclaring the temporaries it assigns in the comma expression (_dec,_init,_class, the accessor's WeakMap) and appends it top.nearest_stmt_list(src/js_parser/lower/lower_decorators.rs:2531), silently doing nothing when that isNone. React refresh appends itsvar _s = $RefreshSig$()the same way (src/js_parser/visit/visit_expr.rs), and for a hook function inside a top-level enum it emitted the_s()call in the body without ever declaring_s.visit_stmtsvisits the enum statements of a list before the list's other statements, ahead of installing its own list innearest_stmt_list(src/js_parser/visit/mod.rs, thepreprocessed_enumsloop runs before the assignment a few lines below it). So while enum members are visited the pointer still refers to the list around the enclosingvisit_stmts:None, and the declaration is dropped;varis emitted at module level and every call of the function shares one set of temporaries. Calling such a function twice and then constructing an instance of the first class throwsTypeError: Cannot read from private field: the constructor reads the second call's WeakMap while the accessor installed by__decorateElementstill uses the first.Fix
s_enum(src/js_parser/visit/visit_stmt.rs) pointsnearest_stmt_listat a local list while the member initializers are visited and emits that list at the top of the closure the enum body compiles to, ahead of the member assignments.Kind::Entry, the scope the temporaries are registered in) and compiles to a closure that runs once per evaluation of the enum declaration, so declaring the temporaries there gives every evaluation of every enum its own bindings, the same way a namespace body (visited with its own statement list) already behaves. Declaring them in the enclosing list instead would keep thevaroutside the scope the symbols belong to, and two top-level enums would share one_xeven under the renamer (bun build), since sibling scopes legitimately receive the same names.nearest_stmt_list_mut()(src/js_parser/p.rs) nowdebug_assert!s that; the two react refresh sites go through the accessor as well. The release fallbacks are unchanged. This is what turns the next stale-pointer producer into a test failure instead of a silently dropped declaration, which is how this one went unnoticed.visit_stmts: a stack local, save/restore around the visit,addr_of_mut!so the pointer is not derived from a temporary&mut; nothing touches the local while the pointer is installed and there is no early return in between.function f(K = class { @dec accessor x = 0 }) {}) or in a class field initializer (class Outer { inner = class { ... } }) still gets itsvarhoisted to the enclosing statement list, so calling the function / constructingOutertwice shares the temporaries. Neither has a per-evaluation statement list, so that fix belongs in the lowering's expression-mode emission, not here.varis emitted and this one does not change how the temporaries are named; js_parser: give standard decorator lowering temporaries unique names #38734 registers them in theKind::Entryscope, i.e. the closure this PR declares them in. Two lowered classes inside one enum still share names until it lands, exactly like two classes in one function today. The tests use decorated accessors, so they keep exercising the hoisting path if Lower accessor-only classes in place instead of relocating static elements #31926 / js_parser: lower undecorated auto-accessors to a native #-private storage field #35708 (undecorated accessors lowered in place) land first.test/bundler/transpiler/es-decorators.test.ts, describe "declarations hoisted out of enum initializers" (5 tests: top-level enum with a decorated class, a decorated accessor and a plain accessor; enum in a function called twice; two sibling top-level enums;Bun.build({ reactFastRefresh: true })declaring_sinside the closure;Bun.Transpileroutput declaring the temporaries inside the closure and not at module level, top-level and nested). All 5 fail on the released build with the errors above, pass with the fix.test/bundler/transpiler/,test/bundler/esbuild/, bundler_edgecase, bundler_jsx, bundler_decorator_metadata, bundler_cjs2esm, bundler_naming, bundler_minify, bundler_regressions, bun-build-api, regression/issue/25716 (react refresh), and the bake dev suites react-spa, hot, esm, bundle (all of which are also transpiled by the debug build themselves); all pass.cargo clippy -p bun_js_parserclean.bun buildof the sibling-enum case runs correctly with the fix; the released build's output throws.Background
class { ... }in expression position into(_init = ..., _class = class { ... }, ..., _class)and hoists avar _init, _class, ...;for the temporaries into the nearest statement list, because an expression cannot declare variables. The class body keeps referring to those temporaries at runtime (constructors run__runInitializers(_init, ...), accessors look their value up in a hoisted WeakMap), so where thevarlands decides which code shares them.nearest_stmt_listis a raw pointer to the statement list currently being built;visit_stmtspoints it at thebeforelist that is emitted ahead of the visited statements and restores it when done. Its two consumers are the decorator lowering above and react refresh, which declares one_s = $RefreshSig$()per hook-calling function next to that function.visit_stmtsvisits every enum in a list before the list's other statements so that enum members can be inlined into code that precedes the declaration (TypeScript itself relies on this). That is why the enum's members are visited while the enclosing list is not installed yet.var E; ((E) => { E[E["A"] = ...] = "A"; })(E ||= {});; the parser gives the body aKind::Entryscope, the same kind a namespace body gets, andgenerate_closure_for_type_script_namespace_or_enumwraps whatever statements it is handed into that closure.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file