transpiler: preserve class TDZ by not hoisting declarations in the runtime path - #35648
transpiler: preserve class TDZ by not hoisting declarations in the runtime path#35648robobun wants to merge 6 commits into
Conversation
… path The runtime transpiler (target=bun, non-bundle) moved side-effect-free class declarations and export default statements to the top of the module via the 'before' parts list. This erased the TDZ for class bindings: 'typeof Foo' and 'new Foo()' would succeed before the declaration line instead of throwing ReferenceError, diverging from Node and browsers. The hoist was originally added (eec1a07, c3dc64d) to paper over early ESM cycle evaluation-order differences that hit kysely and luxon. Both packages now import cleanly without it, so drop the reordering and let SClass / SExportDefault fall through to the default per-statement part path, preserving source order and class TDZ semantics.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 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 |
|
Reproduced with CI (build 80920): 135 test shards pass including |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#34933 targets the same hoist but takes a different approach, so noting the tradeoff here rather than treating either as a pure duplicate:
The hoist was a 2023 workaround for cyclic-import evaluation order in kysely and luxon (eec1a07 / c3dc64d). With the hoist removed, Happy to close either one in favour of the other once a maintainer picks a direction. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This deletes the SClass/SExportDefault hoist that was added specifically to fix kysely-org/kysely#412 and #1961. The TDZ fix is spec-correct, but the claim that those cyclical-import cases no longer need the workaround is only verified manually — there's no automated test for luxon/kysely in this PR or in test/, so a regression there wouldn't be caught. Worth a maintainer look at whether the underlying ESM cycle-ordering fix that supersedes this is actually in place, and whether an integration test for the original repros should ship alongside.
Extended reasoning...
Overview
Two files: src/js_parser/parse/parse_entry.rs drops the two match arms that popped SClass and SExportDefault parts into before when !bundle && can_be_moved(); they now fall through to the default arm (append_part(&mut parts, ...)), which is exactly what the removed arms did minus the hoist, so the fallthrough is mechanically correct. test/bundler/transpiler/runtime-transpiler.test.ts adds three tests covering class, export class, export default class TDZ behavior at runtime plus a --no-bundle output-order check.
Security risks
None. Parser statement-ordering only; no untrusted input handling, no allocation changes.
Level of scrutiny
High. This is the runtime transpiler path that every bun run goes through, and it deliberately reverses commits eec1a07 / c3dc64d which were added to fix real-world package breakage (kysely, luxon). REVIEW.md is explicit: "Before deleting odd-looking code, git-blame why it was written — it is usually load-bearing." The author did the blame and states the original cycle-ordering problem "is no longer present," but that's a manual check — I found no automated test in test/ covering the kysely/luxon cyclical-import scenario, so nothing in CI would catch a regression. Also note SExportDefault::can_be_moved() covers more than classes (functions, arrows, literal exprs), so this stops hoisting export default function/export default <literal> too — semantically fine (function decls hoist at runtime anyway; literal defaults have no binding), but broader than the PR title suggests.
Other factors
The new tests follow harness conventions well (tempDir, bunEnv, concurrent pipe drain, test.concurrent). The one inline nit (vacuous expect.any(Number)) is cosmetic and wouldn't block on its own. I checked that the bundler-side hoisting in needs_wrapper_ref (p.rs:8518) and generateCodeForFileInChunkJS.rs is gated on options.bundle and unaffected. Deferring because reversing a documented workaround for ecosystem packages with only manual verification is a maintainer call, not something to auto-approve.
…import coverage Address review on #35648: - Drop the expect.any(Number) object check in the --no-bundle order test (indexOf always returns a number) and assert the sorted token order directly so a regression shows the actual emitted order in the diff. - Add a two-file mutual 'export default class' cycle (the luxon/kysely pattern the removed hoist was originally added for) to guard against regressing those imports.
There was a problem hiding this comment.
No issues found — the removed SClass/SExportDefault arms fall through to the default arm which does the identical append_part(&mut parts, ...) work minus the hoist, and the tests cover all three declaration forms plus the cyclic-import case the hoist originally papered over. Deferring only because this and #34933 are two competing fixes for the same code and the author has explicitly asked a maintainer to pick a direction.
What was reviewed:
- Fallthrough to the
_arm inparse_entry.rsis behaviorally identical to the deleted arms sans thebefore.push— no lost side effects. can_be_moved()still has live callers inp.rs,lower_esm_exports_hmr.rs, and the linker — not dead code.- No existing tests in
test/assert the old hoist-to-top behavior, so nothing should regress in CI on that axis. - The new
{ missing, order }assertion (2040c75) replaces the earlier vacuous check and now fails with a useful diff.
Extended reasoning...
Overview
Deletes two match arms in the tree-shaking per-statement loop of src/js_parser/parse/parse_entry.rs that popped SClass and SExportDefault parts into the before list when !bundle && can_be_moved(). With the arms gone both statement kinds hit the _ catch-all, which performs the same arena.alloc_slice_copy + append_part(&mut parts, ...) without the reorder. Adds four subprocess tests to runtime-transpiler.test.ts covering class / export class / export default class TDZ, printed source order under --no-bundle --target=bun, and a two-file mutual export default class cycle.
Security risks
None. This is a statement-ordering change in the transpiler; no untrusted-input parsing, allocation sizing, or privilege boundaries are touched.
Level of scrutiny
Medium-high. The diff is tiny and mechanically obvious, but it removes a deliberate 2023 workaround (eec1a07 / c3dc64d) that runs on every ESM file the runtime transpiler sees. The author verified luxon and kysely still import cleanly and added a reduced cycle test, but the blast radius is "every top-level class in every module executed by bun run", so ecosystem regressions that the two spot-checks miss are possible. More importantly, #34933 proposes a narrower guard-based fix for the same lines and the author has explicitly deferred the choice to a maintainer — that is a design decision a human should make, not something to auto-approve past.
Other factors
- The earlier vacuous-assertion nit was addressed in 2040c75; the replacement sorted-token
{ missing, order }check can fail and surfaces the actual emitted order on regression. - Confirmed
can_be_moved()remains referenced insrc/js_parser/p.rs,src/js_parser/lower/lower_esm_exports_hmr.rs,src/bundler/linker_context/generateCodeForFileInChunkJS.rs, andsrc/ast/{s,g,expr}.rs, so no dead-code cleanup is owed. - Grepped
test/for1961,kysely,luxon— no existing test asserts the hoist-to-top output, so CI shouldn't flip on that axis. - Tests follow harness conventions (
tempDir,bunEnv,test.concurrent, concurrent pipe drain, stderr asserted before exitCode).
|
Updated 7:44 AM PT - Jul 25th, 2026
⏳ @autofix-ci[bot], your commit 277884f is still building in
|
…d default classes svelte.test.ts segfaults on linux-aarch64 release when the SExportDefault hoist is dropped entirely (build 80714, both distros, identical trace; adjacent PR builds 80740/80750 pass the same shard). Restore the hoist so 'export default function' / 'export default <const>' keep the existing cycle-friendly ordering, but suppress it when the default is a named class whose name an earlier top-level statement already references, so 'export default class Named' still stays in TDZ when observed before its declaration. Plain 'class'/'export class' declarations remain in source order (SClass arm still removed). Test updates: - --no-bundle order test now covers 'class'/'export class' (both SClass) - cyclic default-class test still exercises the hoisted path
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The use_count_estimate guard only sees references in textually-earlier
statements, so a function declared after the default class (but
engine-hoisted and callable before it) could still observe the class
out of TDZ. Replace the heuristic with a simple is_named_default_class
check: a named default class always stays in source order; anonymous
'export default class {}', 'export default function', and constant
defaults have no module-scope TDZ binding and keep the #1961 hoist.
Extends the export-default TDZ test with a trailing 'function probe()'
that reads the class name, covering the forward-reference case.
There was a problem hiding this comment.
My earlier feedback (the vacuous assertion and the use-count guard missing hoisted-function references) is addressed — the is_named_default_class check is the right shape and the tests cover the variant matrix. I'm not approving because this needs a maintainer call on two points the PR itself flags: (1) it overlaps with #34933 and asks someone to pick a direction, and (2) the SExportDefault hoist is kept solely because removing it segfaults svelte.test.ts on linux-aarch64 release — that's an un-root-caused crash driving a design choice.
What was reviewed:
SClassarm removal falls through to the default_ =>branch, soclass/export classnow go straight toappend_part(&mut parts, ...)with no reordering.is_named_default_class: anonymousexport default class {}has no module-scope TDZ binding, so continuing to hoist it is spec-safe;export default functionbindings are var-like, also fine.- Cyclic test uses named default classes (A/B), so it exercises the no-hoist path and still evaluates — confirms the named-class hoist itself was never load-bearing for the luxon/kysely shape.
Extended reasoning...
Overview
The PR touches two files: src/js_parser/parse/parse_entry.rs (removes the SClass match arm from the per-statement tree-shaking loop and gates the SExportDefault hoist on the default not being a named class) and test/bundler/transpiler/runtime-transpiler.test.ts (four new tests: TDZ for class/export class, TDZ for export default class Named including the forward-reference-via-hoisted-function case I raised, --no-bundle source-order preservation, and a two-file mutual default-class import cycle).
Security risks
None. This is statement-ordering in the runtime transpiler; no input parsing, auth, or resource handling is touched.
Level of scrutiny
High. The tree-shaking loop in parse_entry.rs runs for every module Bun executes at runtime, so a regression here has a very wide blast radius. The change narrows an existing reorder (strictly less transformation than before), which limits the failure surface to code that was silently depending on the old hoist — exactly the cyclic-import case the hoist was originally added for. The new cycle test covers the named-class flavor of that pattern.
Other factors
- The PR description and the duplicate-PR bot both note #34933 takes a different approach (guard both arms on
use_count_estimate) and the author explicitly defers the choice to a maintainer. That is a design decision I shouldn't make. - The stated reason for retaining the
SExportDefaulthoist at all is that dropping it segfaultssvelte.test.tson linux-aarch64 release only (build 80714). Per REVIEW.md, "'the crash goes away' is not a root cause" — a maintainer should decide whether that's acceptable to land as-is or whether the segfault needs to be understood first. Notably, the PR's own cycle test uses named default classes, which are now excluded from the hoist, and it passes — so what the remaining hoist actually protects is not covered by a test in this PR. - Both of my earlier inline comments were addressed: the vacuous
expect.any(Number)block was replaced with a{ missing, order }assertion, and theuse_count_estimateheuristic was replaced with a structuralclass_name.is_some()check plus a trailing-function-declaration probe in the test. - All prior inline threads on the PR are resolved; the last comment-cop flag was a false positive on a two-line comment (shorter than what it replaces on
main).
2 participants
What
bun run(runtime transpiler,--target=bunwithout bundling) moves side-effect-freeclassdeclarations and movableexport defaultstatements to the top of the module. For class bindings that erases the TDZ:typeof Fooandnew Foo()succeed before the declaration line instead of throwingReferenceError, diverging from Node, browsers, andtscoutput. Behaviour is also inconsistent within a file, because a class with a static block /extends/ an outer-reading static initializer is not moved.Repro
bun build --no-bundle --target=bun repro.mjsonmainprintsclass Purephysically above theconst t = ...line.Cause
src/js_parser/parse/parse_entry.rsin the tree-shaking (per-statement part) branch popsSClassandSExportDefaultparts into thebeforelist when!bundle && can_be_moved(), andbeforeis later prepended toparts. Added in eec1a07 / c3dc64d to work around early ESM cycle evaluation-order issues that brokekyselyandluxonimports. The original change gatedSClassonis_export; that guard was dropped in the follow-up and the reorder has applied to every movable top-level class since.Fix
SClassmatch arm entirely soclass/export classdeclarations fall through to the default per-statement handling and keep source order. Their bindings now stay in TDZ until the declaration runs.SExportDefaulthoist (still needed: removing it segfaultssvelte.test.tson linux-aarch64 release, build 80714, while adjacent PR builds 80740/80750 pass the same shard) but suppress it when the default is a named class whose name an earlier top-level statement already references, soexport default class Namedstays in TDZ when observed before its declaration.export default function/export default <const>are unaffected.Verified
import { DateTime } from "luxon"andimport { Kysely } from "kysely"still succeed with the debug build.Relationship to #34933
#34933 applies the same
use_count_estimateguard to bothSClassandSExportDefault. This PR removes theSClasshoist outright instead (luxon / kysely import fine without it) and keeps the guarded hoist only forSExportDefaultwhere dropping it is observably load-bearing. Either is a valid direction; happy to close one once a maintainer picks.Tests
Added to
test/bundler/transpiler/runtime-transpiler.test.ts:class/export class/export default classall throwReferenceErrorwhen touched before their declarationbun build --no-bundle --target=bunkeepsclass/export classin source orderexport default classcycle (the luxon/kysely pattern) still evaluates[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file