Implement static import defer (TC39 Stage 3)#30975
Conversation
wtf/Int128.h dropped its <cassert> include in the latest WebKit bump, which was the only thing declaring assert() for uv__tty_make_raw() in the unified build.
Implements the static form of the Stage 3 Deferred Module Evaluation
proposal. The module graph is loaded and linked eagerly; evaluation of
the deferred module (and its synchronous dependencies) is postponed
until a non-symbol property of the namespace object is accessed.
JavaScriptCore already carries the full runtime semantics in the
pinned WebKit (ModulePhase::Defer, deferred namespace objects,
GatherAsynchronousTransitiveDependencies). This change threads the
phase through Bun's pipeline:
- js_parser: recognise `defer` as a contextual phase keyword when
followed by `*`; `import defer from "x"` and `import defer, {x}`
keep treating `defer` as an ordinary default-binding identifier.
- S::Import / ImportRecord: carry a phase_defer bit.
- js_printer: emit `import defer` back out so JSC's bytecode parser
sees it; record ImportInfoNamespaceDefer and a per-request phase in
ModuleInfo.
- ModuleInfo wire format: dedup requested modules by (specifier, phase)
to match ModuleAnalyzer, serialise a parallel phase-byte array, bump
the transpiler-cache version.
- BunAnalyzeTranspiledModule: pass ModulePhase::Defer to
appendRequestedModule() and set ImportEntry.phase on the deferred
namespace import entry; include phase in the debug record diff.
- ZigGlobalObject: enable JSC::Options::useImportDefer.
Re-exporting a deferred namespace (`import defer * as ns; export {ns}`)
stays a local export, matching ModuleAnalyzer::exportVariable.
Dynamic `import.defer()` is intentionally out of scope.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds TC39 "import defer * as ns from 'path'" support: parser recognizes the syntax, AST gains phase flags, printer and binary format track per-request phase bytes, bundler/JSC wiring and FFI propagate phase_defer to runtime module registration, runtime option and cache bumped, and tests added. ChangesImport Defer Feature
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🤖 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/bundler_jsc/analyze_jsc.rs`:
- Around line 100-108: The loop zips requested_modules_keys,
requested_modules_values and requested_modules_phases but only uses
debug_assert_eq!, so in release a short phases buffer will truncate items or
accept invalid phase bytes; before the zip (around the loop using
requested_modules_keys/requested_modules_values/requested_modules_phases and the
phase_defer calculation), validate that requested_modules_keys.len() ==
requested_modules_values.len() == requested_modules_phases.len() and that every
byte in requested_modules_phases is either 0 or 1; if these checks fail, return
or propagate a clear error (do not rely on debug asserts) to reject malformed
phase data rather than silently truncating or mis-tagging module phases.
In `@src/bundler/analyze_transpiled_module.rs`:
- Around line 362-364: Add a debug assertion that the length of the bytes
returned by self.requested_modules_phases() matches the number of rm_keys before
writing them: compute let phases = self.requested_modules_phases(); then
debug_assert_eq!(phases.len(), rm_keys.len(), "requested_modules_phases() length
must match rm_keys"); finally write phases with writer.write_all(&phases)? and
keep the existing pad calculation based on rm_keys.len().
In `@src/js_parser/lower/lower_esm_exports_hmr.rs`:
- Line 564: The deduplication currently hardcodes phase_defer: false which
collapses deferred imports into eager ones; update the dedup logic to carry the
original import's phase_defer flag through (use the existing phase_defer field
rather than false) and ensure callers pass the flag (notably change calls that
construct deduped imports such as SImport(st) to forward st.phase_defer into the
dedup/new import creation). Locate the deduplication site in
lower_esm_exports_hmr.rs and replace the literal false with the propagated
phase_defer variable, and update all call sites that create deduped imports to
accept and forward the phase_defer value.
In `@src/js_printer/lib.rs`:
- Around line 494-509: rename_key currently only updates self.keys and rebuilds
self.index, which leaves duplicate rows in self.keys/self.values/self.phases
when replace_string_id collapses distinct specifiers to the same (StringID,
phase); change rename_key (or callsite after replace_string_id) to
re-deduplicate requested modules: when touched is true, iterate through
self.keys/self.values/self.phases in order, keep the first occurrence for each
(key, phase) pair (use a temporary HashSet of seen (StringID, phase)), push
unique entries into new_keys/new_values/new_phases, then replace the originals
and rebuild self.index from those deduped vectors so the (specifier, phase)
invariant holds.
In `@test/js/bun/resolve/import-defer.test.ts`:
- Around line 36-44: Replace the unconditional stderr assertions with the
house-style conditional check in every test that expects exitCode.toBe(0):
before the expect(exitCode).toBe(0) line, add if (exitCode !== 0) {
expect(stderr).toBe(""); } and remove or replace the existing
expect(stderr).toBe(""); lines; update each test in this file that currently
asserts exitCode === 0 (they use stdout, stderr, exitCode variables) so stderr
is only asserted conditionally to surface stderr on failures.
🪄 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: b0a798af-da57-4025-84c5-41f5505ee763
📒 Files selected for processing (14)
src/ast/import_record.rssrc/ast/s.rssrc/bundler/analyze_transpiled_module.rssrc/bundler_jsc/analyze_jsc.rssrc/js_parser/lower/lower_esm_exports_hmr.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parse/parse_stmt.rssrc/js_printer/lib.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/bindings/BunAnalyzeTranspiledModule.cppsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/wtf-bindings.cpptest/js/bun/resolve/import-defer.test.ts
Reject the deserialized module-info record in release builds if the three requested-module arrays have mismatched lengths or a phase byte is neither Evaluation (0) nor Defer (1). The buffer can originate from the on-disk transpiler cache, so this matches the existing treatment of unknown RecordKind bytes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Compare the raw token bytes (not the decoded identifier) when recognising `import defer`, so escape sequences cannot spell the phase keyword — matching the treatment of `as`/`from`/`async`. Also fix the stale "six views" comment in ModuleInfoDeserialized::create().
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_stmt.rs`:
- Around line 1481-1497: The defer-phase branch handling `if default_name_raw ==
b"defer" && p.lexer.token == T::TAsterisk` allows `import defer * as ...`
without enforcing the same scope restrictions as regular namespace imports; add
the same gating logic used for regular imports (check `is_module_scope ||
(is_namespace_scope && is_typescript_declare)`) before accepting the defer-star
import, and reject or error out in the same way as the non-defer branch if the
scope check fails; update the block around `p.lexer.next()`,
`p.lexer.expect_contextual_keyword(b"as")`, and the eventual `return
p.process_import_statement(stmt, path, loc, false)` to run only when the scope
check passes.
🪄 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: bc1918f6-48f8-4018-9890-7ea34a43054e
📒 Files selected for processing (3)
src/bundler/analyze_transpiled_module.rssrc/js_parser/parse/parse_stmt.rstest/js/bun/resolve/import-defer.test.ts
Apply the same scope gate as regular namespace imports: the
declaration is only valid at module scope (or inside a TypeScript
`declare namespace`). Previously `namespace X { import defer * as ns
from "y" }` slipped through because the defer branch returned
early before the TIdentifier arm's namespace-scope fall-through to
parse_type_script_import_equals_stmt.
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_stmt.rs`:
- Around line 1481-1506: The early branch treating `default_name_raw == b"defer"
&& p.lexer.token == T::TAsterisk` as an ESM defer-import should also check
`!opts.is_export` so `export import defer * as ...` does not take the ESM path;
update the if condition that contains `default_name_raw == b"defer" &&
p.lexer.token == T::TAsterisk` (inside the parse/import handling where `stmt =
S::Import { ... phase_defer: true, ... }`) to include `&& !opts.is_export`,
ensuring export-import-equals cases fall through to the TypeScript import-equals
handling.
🪄 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: deff69b8-b4e9-4ee2-9fb4-9b1280459c6d
📒 Files selected for processing (2)
src/js_parser/parse/parse_stmt.rstest/js/bun/resolve/import-defer.test.ts
…mport trimming When the deferred namespace was only referenced from a dead branch, scan_imports would clear `star_name_loc` (both in the dead-code trimmer and in the star-to-clause converter). That left the printer with `PHASE_DEFER` but no `CONTAINS_IMPORT_STAR`, producing `import defer"./x";` — a hard SyntaxError in JSC. Keep the `* as ns` binding for phase-defer imports so the module stays linked-but-unevaluated (the intended semantics), and as a belt-and-suspenders measure gate the printed `defer` token on `CONTAINS_IMPORT_STAR` too. Also: make the test suite concurrent; fix a stale '0..=8' comment.
`export import` in TypeScript is the import-equals form (`export import X = Y.Z`); the defer branch was returning early before the `opts.is_export` fall-through to parse_type_script_import_equals_stmt, silently dropping the `export` and parsing it as a plain deferred import.
There was a problem hiding this comment.
Thanks — all four points from my last pass are addressed (scan-side phase_defer guard + printer belt-and-suspenders, scope/is_export checks with tests, stale comment, describe.concurrent). Nothing further from me; deferring final sign-off to a human given this threads a new language feature through parser → serialized ModuleInfo (cache format bump) → Rust/C++ FFI.
Extended reasoning...
Overview
Implements TC39 Stage 3 import defer * as ns from across 15 files: parser recognition (parse_stmt.rs), AST flag (S::Import.phase_defer, ImportRecordFlags::PHASE_DEFER), printer emission + ModuleInfo phase tracking with a new RecordKind and parallel u8 phase array, on-disk serialization changes in analyze_transpiled_module.rs (cache version 20→21), Rust↔C++ FFI signature extensions in analyze_jsc.rs / BunAnalyzeTranspiledModule.cpp, and JSC::Options::useImportDefer() = true. 19 tests cover evaluation timing, error semantics, TLA interaction, identifier-vs-keyword, and the syntax-error edge cases raised in review.
Prior review follow-up
Since my 07:22 review, commits 810e569 / baef48d / bfdda47 landed:
- Unused-import trimming (the 🔴 item):
scan_imports.rsnow skips strippingstar_name_locand the star→clause conversion whenst.phase_defer; the printer additionally gates thedefertoken onCONTAINS_IMPORT_STAR. New test "namespace only referenced in dead code keeps the deferred binding" covers it. - Scope /
export import: defer branch now guarded by!opts.is_exportand the sameis_module_scope / is_namespace_scope / is_typescript_declarecheck as theT::TAsteriskarm; two new syntax-error tests. - Stale
0..=8comment: rephrased to "out-of-range bytes are invalid". describe.concurrent: applied.
All bot threads (mine and CodeRabbit's) are resolved or addressed.
Security risks
None identified. No auth/crypto/permissions surface; the deserialization path validates phase bytes and array lengths in release builds (added in 34c4f6b), returning null_mut() on malformed cache data rather than trusting it.
Level of scrutiny
High — warrants human review. This is a new language-level feature touching production-critical code paths: the JS parser (correctness of contextual-keyword handling), the printer's ModuleInfo binary format that is persisted to the transpiler cache, and FFI signatures that must agree byte-for-byte between Rust and C++ (extern "C" with appended bool phaseDefer). The serialization-format change and the global useImportDefer option flip both have blast radius beyond the immediate diff.
Other factors
Test coverage is solid and targeted at the spec semantics plus every regression raised in review. The implementation looks correct to me after re-reading the updated diff, but the combination of FFI ABI changes, on-disk format changes, and parser grammar additions is the kind of change a maintainer should sign off on.
There was a problem hiding this comment.
All my earlier feedback has been addressed and I found nothing new on re-review — but given this threads a new phase flag through parser → printer → ModuleInfo serialization → FFI → JSModuleRecord and bumps the on-disk cache format, it's worth a human sign-off.
Extended reasoning...
Overview
This PR implements static import defer * as ns from "..." (TC39 Stage 3) end-to-end: parser recognition (parse_stmt.rs), AST/import-record flags (s.rs, import_record.rs), printer emission and ModuleInfo phase tracking (js_printer/lib.rs), binary (de)serialization with a new parallel u8 phase array and 4-byte padding (analyze_transpiled_module.rs, js_printer/lib.rs), the Rust→C++ FFI bridge to JSModuleRecord with widened signatures (analyze_jsc.rs, BunAnalyzeTranspiledModule.cpp), enabling JSC::Options::useImportDefer, a transpiler-cache version bump (20 → 21), and a 384-line test suite covering evaluation timing, error propagation, TLA, re-exports, attributes, identifier disambiguation, and the syntax-error edge cases that came up during review.
Security risks
None apparent. No auth/crypto/permissions surface. The deserialization path for the new phase byte (which can come from the on-disk transpiler cache) validates lengths and rejects bytes outside {0,1} in release builds (added in 34c4f6b after CodeRabbit's note), so a corrupted cache entry fails closed rather than mis-tagging modules. The cache version bump prevents old-format records from being misread.
Level of scrutiny
High. This is a new language feature touching the hot module-loading path: parser grammar, printer output that JSC re-parses, a binary wire format consumed across an FFI boundary, and C++ extern "C" signature changes that must agree with the Rust declarations. A mismatch in any of these layers would break module loading or silently change evaluation order. The finalize() local→indirect export rewrite and the RequestedModules dedup-by-(specifier, phase) change are subtle correctness points that benefit from review by someone familiar with the JSC ModuleAnalyzer semantics this is mirroring.
Other factors
- I left six inline comments across two earlier review passes (escape-spelled
def\u0065r, stale "six views" / "0..=8" comments, the TS unused-import-trimming → invalidimport defer"./x";printer bug, the TS namespace /export import deferscope-guard gaps, anddescribe.concurrent). All were addressed in daced86, 810e569, baef48d, and bfdda47 with regression tests added for each. - All CodeRabbit threads are resolved (two accepted, three declined with rationale the bot accepted).
- The two CI failures (
node-http-backpressure,v8-heap-snapshotSIGKILLs) are unrelated flakes. - Test coverage is thorough for the runtime path; the explicitly out-of-scope bundler/HMR degradation is documented.
This is well-executed and I have no outstanding concerns, but it is not the kind of mechanical change I should approve without a human reviewer.
CI statusBuild #56044 at On that same failing shard, Already spent the one re-roll earlier, so handing off to a maintainer to re-run the darwin-x64 shard or merge over it. The diff is green. |
| /// "import defer * as ns from 'path'" — the TC39 Deferred Module Evaluation | ||
| /// proposal. Only valid with a namespace import (`star_name_loc` is set, | ||
| /// `default_name`/`items` are empty). | ||
| pub phase_defer: bool, // = false |
There was a problem hiding this comment.
Should we turn this into an enum of phases? to support source phase later
There was a problem hiding this comment.
Reasonable — I went back and forth on this. A few notes on why I landed on the bool here but kept the enum where it matters for forward-compat:
- The serialized
ModuleInfoalready uses aModulePhaseenum (Evaluation = 0,Defer = 1) for the per-requested-module phase byte, so the wire format is trivially extensible toSource = 2without another cache-version bump. import source x from "..."has a different grammar shape (default binding only, no* as), so it wouldn't share thisS::Importbranch inparse_stmtanyway — it'd get its own handling and probably its ownS::Importfield (or a sharedphase: ImportPhaseat that point).ImportRecord::Flagsis now full (u16, bit 15 isPHASE_DEFER). AddingPHASE_SOURCEwould mean widening tou32or moving phase to a separateu8on the record — either is fine but felt like it belonged with the change that actually needs it.
Happy to switch phase_defer: bool → phase: ImportPhase { Evaluation, Defer } on S::Import now if you'd rather have it in place — it's a small mechanical change. Let me know.
There was a problem hiding this comment.
Pretty reasonable - how can we make sure we are not introducing strange bugs by theoretically allowing an invalid state though?
There was a problem hiding this comment.
The invariant (phase_defer ⟹ star_name_loc.is_some() ∧ default_name.is_none() ∧ items.is_empty()) is held by construction right now — the only place that sets phase_defer: true is the parser branch that builds a fresh S::Import { star_name_loc: Some(_), ..Default::default() }, and every other S::Import constructor (7 of them) explicitly passes phase_defer: false.
For defence-in-depth:
- Printer already gates the
defertoken onPHASE_DEFER && CONTAINS_IMPORT_STAR, so even a malformedS::Importcan't produceimport defer"./x";. - scan_imports refuses to strip the star binding when
phase_deferis set. - Added a
debug_assert!inprocess_import_statement(1a98bc5) that checks the full shape invariant at theS::Import→ import-record boundary, so any future producer that violates it trips immediately in debug builds rather than surfacing as odd output downstream.
An enum wouldn't actually prevent the cross-field invalid state (you could still have phase: ImportPhase::Defer with default_name: Some(_)), so the assert is the more direct guard. When import source lands and needs its own shape invariant (default-binding-only), that's the natural point to lift phase_defer: bool → phase: ImportPhase and widen the record flags.
phase_defer implies star-only (no default binding, no named clause). The parser guarantees this by construction; the debug assertion makes the invariant explicit at the S::Import -> ImportRecord boundary so any future producer that violates it is caught immediately.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Add a test for what happens when bytecode compilation is used
Verifies that `import defer * as ns from "..."` in the source bytecode-compiles cleanly under both --format=cjs and --format=esm, and that the compiled binary loads from the bytecode cache. The bundler inlines the deferred module into the entry chunk (documented limitation), so the test asserts the inlined-evaluation order.
|
Added Since the bundler inlines the deferred module into the entry chunk (documented limitation), the test asserts the inlined-evaluation order — the point is that |
4 participants
Implements the static form of the Stage 3 Deferred Module Evaluation proposal:
JavaScriptCore already carries the full runtime for this (landed upstream in WebKit/WebKit#64053 and present in Bun's pinned WebKit
3167a44fb9). This PR threads theModulePhase::Deferflag through Bun's own parser → printer →JSModuleRecordbridge so JSC actually receives it, and flips onuseImportDefer.What changed
Parser / AST —
src/js_parser/parse/parse_stmt.rs,src/ast/{s,import_record}.rsdeferis a contextual phase keyword only when immediately followed by*. In every other position (import defer from "x",import defer, {a} from "x",import { defer } from "x") it remains an ordinary identifier.S::Import.phase_deferandImportRecord::Flags::PHASE_DEFER.Printer / ModuleInfo —
src/js_printer/lib.rsimport deferso JSC's own parser sees it during bytecode generation.RecordKind::ImportInfoNamespaceDefer;requested_modulesnow dedups by(specifier, phase)— matching JSC'sModuleAnalyzer::appendRequestedModule— and serialises a parallelu8phase array. Transpiler-cache version bumped (20 → 21) for the format change.finalize()'s local→indirect export rewrite intentionally ignores deferred namespace imports soimport defer * as ns; export { ns }stays a local export, matchingModuleAnalyzer::exportVariable.JSC bridge —
src/bundler/analyze_transpiled_module.rs,src/bundler_jsc/analyze_jsc.rs,src/jsc/bindings/BunAnalyzeTranspiledModule.cppappendRequestedModule(..., ModulePhase::Defer)andaddImportEntry({ Namespace, phase: Defer, ... })for deferred requests.dumpRecordInfoincludes phase so theBUN_DEBUGfallback-parse diff stays in agreement.Runtime —
src/jsc/bindings/ZigGlobalObject.cppJSC::Options::useImportDefer() = true.Unrelated build fix —
src/jsc/bindings/wtf-bindings.cppwtf/Int128.hdropped its transitive<cassert>in the latest WebKit bump, which was the only thing declaringassert()foruv__tty_make_raw()in that unified-build unit. Added the include directly.Tests (
test/js/bun/resolve/import-defer.test.ts, 16 cases)ns[Symbol.toStringTag] === "Deferred Module"without triggering evaluationin/Object.keystrigger evaluationGatherAsynchronousTransitiveDependencies) so on-access eval stays synchronousimport defer * as ns; export { ns }exports the deferred namespace as a local bindingimport defer * as d from "./x.json" with { type: "json" }.tsdeferstill a valid identifier (import defer from,import defer, {x} from,import { defer } from)Bun.Transpilerround-tripsimport deferimport defer { x } from/import defer x fromare syntax errorsOut of scope
import.defer(...)(left for a follow-up; JSC's upstream PR also excluded it).bun buildinlines a deferred dependency into the same chunk the defer semantics are lost (module body runs at chunk load). External imports (--external) correctly preserveimport deferin the output.References
import defersemantics WebKit/WebKit#64053