transpiler: preserve TDZ for top-level class declarations referenced earlier in the file - #34933
transpiler: preserve TDZ for top-level class declarations referenced earlier in the file#34933robobun wants to merge 6 commits into
Conversation
…earlier in the file
The runtime transpiler (target=bun, tree_shaking, non-bundle) moves
top-level class declarations to the start of the module to smooth over
some cyclic-import cases. That reorder was unconditional on any class
Class::can_be_moved() accepts, so a file like
console.log(typeof K);
class K {}
was rewritten with class K first, and typeof K evaluated to 'function'
instead of hitting the temporal dead zone ReferenceError that Node and
bun's own bundled output produce.
At this point in part assembly, earlier top-level statements have
already been visited, so the class symbol's use_count_estimate reflects
exactly those earlier references. Skip the move when that count is
non-zero for a class (and for the named class inside an export default)
so the declaration stays where it was written and the TDZ is preserved.
Classes that are not referenced before their declaration are still
hoisted, so the cyclic-import workaround keeps working.
|
Updated 8:25 AM PT - Jul 21st, 2026
✅ @robobun, your commit fc9f02f8e8e946788afe265c2a07b497f5ef45b2 passed in 🧪 To try this PR locally: bunx bun-pr 34933That installs a local version of the PR into your bun-34933 --bun |
WalkthroughChangesTop-level class and named default-export class hoisting now checks for earlier name references, while runtime transpiler tests cover TDZ behavior and retained hoisting. Class hoisting TDZ preservation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/js_parser/parse/parse_entry.rs`:
- Around line 1055-1063: Compress the rationale comment near the class-statement
relocation logic in src/js_parser/parse/parse_entry.rs:1055-1063 to three lines
or fewer while preserving both the cyclic-import purpose and the TDZ safeguard.
Also shorten the regression comment in
test/bundler/transpiler/runtime-transpiler.test.ts:256-260 to three lines or
fewer while retaining the behavior it protects.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f7bd678-69b7-4a05-bbfe-e276662b747d
📒 Files selected for processing (2)
src/js_parser/parse/parse_entry.rstest/bundler/transpiler/runtime-transpiler.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/bundler/transpiler/runtime-transpiler.test.ts`:
- Around line 256-258: Update the regression-test comments at
test/bundler/transpiler/runtime-transpiler.test.ts lines 256-258 and 304-305 to
remove the explanatory prose and leave only the relevant issue URL, with no
other test changes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3cb6442-ed1f-4c4b-b387-468495129300
📒 Files selected for processing (2)
src/js_parser/parse/parse_entry.rstest/bundler/transpiler/runtime-transpiler.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/parse/parse_entry.rs:1064-1074—use_count_estimateis a textual, source-order count — not an evaluation-time check — so this gate both over- and under-applies relative to the comment's claim. Over:export function make() { return new K(); }beforeclass K {}disables the hoist even though the reference is inside a function body and no TDZ is at stake (narrowing the #1961/kysely#412 workaround more than the description says; the "still hoisted" test'sconst unrelated = 1never mentionsA, so it doesn't cover this). Under:foo(); class K {}; function foo() { return new K(); }still hoistsKabovefoo()becauseuse_count_estimate(K)==0when the SClass arm runs — pre-existing, but the same bug class this PR targets. Neither should block merge (over-applying fails safe toward spec, under-applying needs look-ahead), but it'd be worth qualifying the comment/description and adding tests that pin these two shapes so the behavior is deliberate.Extended reasoning...
What the gate actually checks
The new guard reads
p.symbols[class_name.ref_].use_count_estimateat the moment the top-level loop reaches theSClassstatement. By that point every textually earlier top-level statement has been sent throughappend_part→visit_stmts_and_prepend_temp_refs, which walks the full subtree — including function/arrow bodies — and callsrecord_usage(p.rs:1745-1754) for every identifier encountered. Souse_count_estimateis a count of textual mentions in earlier statements, not of references that execute at module-evaluation time before the class initializes. The code comment ("an earlier top-level statement already references the class name") and the PR description ("the cyclic-import workaround is preserved") both describe the second thing while the code implements the first.Over-application: function-before-class disables the hoist with no TDZ at stake
Concrete trace for:
export function make() { return new K(); } export class K {}
- Loop iteration 0:
SFunction(make)falls into the default arm (line 1142) →append_part→visit_stmts_and_prepend_temp_refs.visit_funcwalks the body, visitsnew K(), andrecord_usagebumpsK.use_count_estimateto 1. - Loop iteration 1:
SClass(K)arm readsuse_count_estimate == 1→used_before_decl = true→should_move = false. The class stays in place.
But
make's body doesn't run at module-evaluation time — there is no TDZ to preserve here. Prior to this PR the class was hoisted; after it, it isn't. This is exactly the shape the kysely/luxon workaround targets (helper functions/factories declared above the class they construct), so "the cyclic-import workaround is preserved" only holds when nothing textually before the class mentions its name. The "still hoisted" test happens to satisfy that (const unrelated = 1;never touchesA), so it can't catch this narrowing.The enum-preprocessing pass at lines 979-1000 has the same effect: a TS
enumthat appears after the class but references it is visited before the main loop, so its reference is already counted when the class is reached.This is not a correctness bug — being more conservative never re-erases a TDZ — but it is an untested behavioral narrowing of a workaround the PR description asserts is preserved.
Under-application: later-declared hoisted function still lets the class jump the TDZ
The mirror case — the gate misses references it should count:
foo(); // calls hoisted foo → new K() → K in TDZ per spec class K {} function foo() { return new K(); }
Loop trace:
SExpr(foo())→append_partvisits it → bumpsuse_count(foo);Kuntouched.SClass(K)→use_count_estimate(K) == 0→should_move = true→ moved tobefore.SFunction(foo)(default arm, line 1142) →append_partvisits body → bumpsuse_count(K), but the move already happened.
Output order:
class K {}; foo(); function foo() { return new K(); }— succeeds. Node throwsReferenceError: Cannot access 'K' before initialization. This is the same bug class the PR targets (top-level class TDZ erased by the runtime-transpiler hoist), and REVIEW.md asks that same-class variants be covered in the same PR. It is pre-existing, not a regression, and fixing it correctly needs look-ahead into later hoisted-function bodies — a materially different mechanism than the source-orderuse_count_estimateheuristic.Why existing code doesn't prevent it
use_count_estimateis by design a coarse textual counter incremented during the visit pass; it has no notion of "executes at module-evaluation time" vs "inside a nested function body", and it is populated in source order as the top-level loop iterates. Both gaps are intrinsic to using it as the signal.Impact and suggested action
Over-application fails safe toward spec (a class that could have been hoisted simply isn't), so the worst case is that a cyclic-import file that previously benefited from the workaround stops benefiting — which per the tests still passing hasn't been demonstrated for a real package. Under-application is pre-existing behavior this PR strictly improves on; code hitting it always throws in Node, so it's rare in practice.
Given both fail non-blocking, the concrete asks are: (a) soften the code comment / PR description from "references" to something like "any earlier statement mentions the class name (including inside function bodies)" so the guarantee isn't overstated; (b) add a test with
function f() { return new A(); }before the class showing it is not hoisted, so the narrowed scope is pinned as deliberate; and optionally (c) add a.todotest for thefoo(); class K {}; function foo() { new K(); }shape so the remaining gap is tracked. If preserving the hoist for the function-before-class shape matters, distinguishing body references from top-level references would need a different signal thanuse_count_estimate— e.g. a separate counter bumped only outsidefn_stmts. - Loop iteration 0:
The '// @Bun' tests in runtime-transpiler.test.ts fail on a second release-build run when the entry module is served from the runtime transpiler cache (the has_loaded flag is not set on that path; #33371 tracks it). Keep that file untouched and put the class-hoist TDZ tests in a sibling file so they can be verified independently. Also document that the use_count_estimate guard counts textual mentions in already-visited statements (including function bodies), and extend the 'still hoisted' test to pin that a class mentioned by a preceding function body is left in source order while an unmentioned class is still moved ahead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/bundler/transpiler/runtime-transpiler-class-hoist.test.ts`:
- Around line 13-15: Strengthen the assertions in the runtime transpiler
class-hoist test’s catch blocks to validate the complete TDZ error message,
including “Cannot access 'K' before initialization,” rather than only
e.constructor.name. Apply the same assertion update to the additional catch
block around lines 39–42, preserving the existing error-capture 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d937ebe-a0c0-40e1-a0be-efa6d649189b
📒 Files selected for processing (2)
src/js_parser/parse/parse_entry.rstest/bundler/transpiler/runtime-transpiler-class-hoist.test.ts
There was a problem hiding this comment.
No issues found in the latest revision — all prior nits (concurrent tests, order-token presence guard, full TDZ error message, combined-object assertion) are addressed. Deferring to a human because this narrows the cyclic-import class-hoisting heuristic in the core parser path: the new guard is conservative (declining to hoist is always spec-safe), but as noted in the resolved thread on parse_entry.rs:1067, it also suppresses the hoist when the class name appears inside a preceding function body or a later top-level enum. Worth a maintainer confirming the kysely #412 / luxon #1961 repros still evaluate cleanly.
Extended reasoning...
Overview
The PR touches two files: a ~35-line change in src/js_parser/parse/parse_entry.rs gating the existing "move top-level class / export-default to before" heuristic on use_count_estimate == 0 for the class name symbol, and a new dedicated test file test/bundler/transpiler/runtime-transpiler-class-hoist.test.ts with five concurrent subprocess tests. The parser change reads p.symbols.as_slice()[ref.inner_index() as usize].use_count_estimate before append_part() visits the class statement itself, using the exact same indexing pattern already used at lines 1148/1150/1397/etc. of the same file, so there's no new panic surface.
What was checked
- The
use_count_estimateread happens beforeappend_part()on the class statement, so it counts only references from already-visited top-level statements (plus pre-visited enums). Declining to hoist leaves statements in source order, which is always spec-correct — the change can only move behavior toward spec, never away. - The
SExportDefaultarm only inspectsStmtOrExpr::Stmt(SClass); functions and expression defaults fall through withused_before_decl = false, preserving the existing hoist for those. - The symbol-table indexing pattern matches the established idiom in this file;
class_name.ref_is created during parse of the same source, soinner_index()is in-bounds. - Tests:
describe.concurrent,toMatchObjecton{stdout, exitCode}(no exact-empty stderr assertion), fullReferenceError: Cannot access 'K' before initialization.message asserted, and the order-check test now assertsmissing: []so a missing token fails visibly. All prior inline comments from me and coderabbit are resolved.
Security risks
None. This is a statement-reordering heuristic in the transpiler; no untrusted input parsing, no allocation sizing, no FFI.
Level of scrutiny
Medium-high. The change itself is small and mechanically sound, but it lives in the tree-shaking path that runs for every file the runtime transpiler processes, and it narrows a heuristic that was deliberately added (and later broadened in c3dc64d) to make real-world packages (kysely, luxon) evaluate under cyclic imports. The guard is broader than "earlier top-level statement references the name": append_part() walks into function bodies, and the enum pre-pass visits later enums first, so both shapes suppress the hoist. That's arguably the right call (the counter can't tell whether the function is invoked at top level), but it's a behavioral tradeoff a maintainer should sign off on — ideally by re-running the original kysely/luxon repros.
Other factors
The mechanical gate in the PR description shows the ASAN debug lane fails 5/5 without the fix and passes 5/5 with it, and the release lane passes both (release doesn't take this path the same way). Existing transpiler.test.js, runtime-transpiler.test.ts, decorators.test.ts, and export-default.test.js are stated to pass unchanged.
|
Verified the two packages the hoist was added for still load under this change (release build, fc9f02f): $ bun -e 'import { DateTime } from "luxon"; console.log(DateTime.now().toISO().slice(0,10))'
2026-07-21
$ bun -e 'import { Kysely } from "kysely"; console.log(typeof Kysely)'
functionluxon@3.4.4, kysely@0.27.3. |
1 participant
Fixes #25569
Problem
At module top level,
bun runevaluates a class declaration as if it were hoisted, so use-before-declaration succeeds instead of throwing:Block/function scope,
let/const, and class expressions are unaffected; only top-level class declarations lose their TDZ in the runtime transpiler.bun buildoutput of the same file is correct when evaluated in Node, so code that works underbun runcan start throwing once bundled.Cause
src/js_parser/parse/parse_entry.rsmoves top-levelclassandexport defaultstatements into thebeforepart list whenever!bundle && Class::can_be_moved(), as a workaround for some cyclic-import evaluation-order issues (kysely-org/kysely#412, #1961). The move is not conditioned on whether the class binding is already referenced by an earlier statement, so the declaration is reordered above the first use and the TDZ vanishes. The original change in eec1a07 gated this onclass.is_export; that guard was dropped in c3dc64d and the reorder has applied to every movable top-level class since.Visible with
--target=bun(which takes the same tree-shaking path as the runtime loader):Fix
At the point where the move is decided, every earlier top-level statement has already been visited, so the class symbol's
use_count_estimatecounts the textual mentions in those statements (including inside function bodies). Skip the move when that count is non-zero for the class name (and for the named class inside anexport default). Classes that are not mentioned before their declaration are still hoisted, so the cyclic-import workaround is preserved for the cases it was added for. The check is intentionally conservative: a reference inside a preceding function body also suppresses the hoist, because the counter cannot tell whether that function is called before the class initializes.Verification
New tests in
test/bundler/transpiler/runtime-transpiler-class-hoist.test.ts:class K {},export class K {},export default class K {}and a CommonJS top-level class each referenced before their declaration now throwReferenceError(previously printedtypeof=function | constructed=K).--target=bun, a class with no prior mention is still emitted ahead of the preceding statement, while a class mentioned inside a preceding function body stays in source order.Existing
transpiler.test.js,runtime-transpiler.test.ts,decorators.test.tsandexport-default.test.jspass unchanged.The tests live in a sibling file rather than
runtime-transpiler.test.tsbecause the// @buntests in that file currently fail on a second release-build run when the entry module is served from the runtime transpiler cache (#33371); touching that file makes the release gate unreliable until that lands.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file