Skip to content

Implement TC39 Stage 3 import defer#30015

Open
robobun wants to merge 14 commits intomainfrom
farm/4846b6a5/import-defer
Open

Implement TC39 Stage 3 import defer#30015
robobun wants to merge 14 commits intomainfrom
farm/4846b6a5/import-defer

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented Apr 30, 2026

Implements the Stage 3 deferred module evaluation proposal:

import defer * as ns from "./mod.js";
// mod.js and its deps are fetched and linked now, but NOT evaluated.

console.log("before");
console.log(ns.value);   // <-- mod.js top-level runs here, synchronously
const ns = await import.defer("./mod.js");

Depends on oven-sh/WebKit#206 for the JavaScriptCore runtime semantics. The Bun-side C++ is guarded with #ifdef JSC_HAS_IMPORT_DEFER so this PR builds against the current prebuilt WebKit; the runtime test suite probes for support at startup and skips itself until WEBKIT_VERSION is bumped.

What works

import defer * as ns from "..." — full spec semantics

  • Module graph is loaded + linked eagerly (per spec — loading is not deferred, only execution).
  • Synchronous portions of the deferred subgraph do not evaluate until a string-key property of ns is accessed.
  • Modules with top-level await in the deferred subgraph are eagerly evaluated via GatherAsynchronousTransitiveDependencies, so the on-access evaluation is guaranteed synchronous (ReadyForSyncExecution).
  • The deferred namespace is a distinct object from the regular namespace: @@toStringTag === "Deferred Module", then is hidden (so it never acts as a thenable), and if the module throws during evaluation every subsequent access re-throws the same error.
  • import defer from "x" is still a default import named defer (back-compat); only import defer * engages the phase syntax.

import.defer("...") — partial

  • Parses, resolves, and returns a Deferred Module namespace.
  • Currently still evaluates the module body eagerly (only the returned object is the deferred namespace). Fully deferring requires threading ModulePhase through ContinueDynamicImport and the moduleLoaderImportModule embedder hook — tracked with a test.todo. This matches the scope of upstream WebKit's in-flight PR ([JSC] Add core semantics of import defer WebKit/WebKit#40453), which also excludes the dynamic form.

Transpiler / bundler

  • Bun.Transpiler parses and prints both forms, and import source / import.source(...) round-trip as pass-through (mirroring esbuild's ImportPhase).
  • Bundler / Bake / REPL lowering of phase imports is out of scope for this PR (same as esbuild's external-only restriction) — phase-aware lowering is a follow-up once the JSC side lands.

How

Bun

  • ImportPhase enum on ImportRecord and E.Import; parser + printer + ImportScanner + transposeImport thread it through.
  • New RecordKind.{import_info_namespace_defer, requested_module_defer} in the ModuleInfo bridge so pre-analysed records match JSC's ModuleAnalyzer output.

JavaScriptCore (oven-sh/WebKit#206)

  • AbstractModuleRecord::ModulePhase on ModuleRequest / ImportEntry; separate m_moduleDeferredNamespaceObject; getModuleNamespace(…, phase).
  • gatherAsynchronousTransitiveDependencies() (iterative DFS) + innerModuleEvaluation builds the spec's evaluationList (fast path preserved when no deferred imports are present).
  • CyclicModuleRecord::readyForSyncExecution(); initializeEnvironment passes ImportEntry.phase through.
  • JSModuleNamespaceObject: m_isDeferred, IsSymbolLikeNamespaceKey, ensureDeferredNamespaceEvaluation() hooked into [[GetOwnProperty]] / [[Delete]] / [[OwnPropertyKeys]] / [[DefineOwnProperty]].
  • Parser accepts import.defer(; ImportNode carries phase → third arg to @importModuleInternalMicrotask::ImportDeferModuleNamespace.
  • useImportDefer flipped to true.

Tests

test/js/bun/import-defer/import-defer.test.ts — 21 tests (10 unconditional transpiler/syntax, 10 runtime-gated on the WebKit bump, 1 todo) covering deferred evaluation order, transitive sync deps, re-evaluation dedup, @@toStringTag + hidden then, distinct namespace identity, error re-throw on every access, eager TLA-dep evaluation, import defer from back-compat, import defer {x} syntax error, and transpiler round-trips.

References

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 30, 2026

Updated 7:38 PM PT - May 6th, 2026

@robobun, your commit c4a63cd has 2 failures in Build #52317 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30015

That installs a local version of the PR into your bun-30015 executable, so you can run:

bun-30015 --bun

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds TC39 Stage 3 import-phase support (defer, source) across parsing, AST, analysis, printing, runtime bindings, module-info serialization, and tests; includes new import-phase enum, propagation of phase through transforms, defer-specific module-record APIs, and a comprehensive test suite.

Changes

Cohort / File(s) Summary
Import Phase Type & Records
src/import_record.zig, src/bun.zig
Defines ImportPhase (.evaluation, .defer_, .source) and adds phase: ImportPhase to ImportRecord; re-exports ImportPhase from src/bun.zig.
Parser: import.defer / import.source
src/ast/parseStmt.zig, src/ast/parseImportExport.zig
Detects import defer and import source syntaxes and sets the parsed import phase accordingly; updates error messages and contextual import handling.
AST & Transpose State
src/ast/E.zig, src/js_parser.zig, src/ast/P.zig, src/ast/visitExpr.zig
Adds phase to exported E.Import and TransposeState, threads phase through transposeImport, processImportStatement, and expression visitation.
Import Scanning & Analysis
src/ast/ImportScanner.zig, src/analyze_transpiled_module.zig
Makes unused-import trimming and star-to-clause lowering phase-aware (only apply in .evaluation); in analyze_transpiled_module.zig adds RecordKind variants import_info_namespace_defer and requested_module_defer, ModuleInfo storage for deferred records, and public APIs addImportInfoNamespaceDefer and addRequestedModuleDefer (with dedupe logic for requested modules).
Module-info Deserialization → JS dispatch
src/analyze_transpiled_module.zig
During deserialization-to-JS conversion, recognizes the new deferred record kinds and dispatches them to new JSModuleRecord extern wrappers (addImportEntryNamespaceDefer, addRequestedModuleDefer) instead of ignoring them.
JS Printer: emit phase-aware syntax & module registration
src/js_printer.zig
Emits import.defer( / import.source( for expression- and statement-level imports; routes deferred imports to addRequestedModuleDefer and addImportInfoNamespaceDefer when recording module metadata.
C++ Runtime Bindings
src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp
Adds externs JSC_JSModuleRecord__addImportEntryNamespaceDefer and JSC_JSModuleRecord__addRequestedModuleDefer, conditionally annotate entries with ModulePhase::Defer, and update debug/dump output to include phase info.
Tests
test/js/bun/import-defer/import-defer.test.ts
Adds extensive tests exercising static and dynamic import.defer/import.source parsing, transpiler output, runtime deferred-evaluation semantics, deduplication, namespace branding, transitive behavior, and compatibility checks.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Implement TC39 Stage 3 import defer' directly and clearly summarizes the main change—adding support for the import defer syntax as specified in the TC39 proposal.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description comprehensively explains the implementation of the TC39 Stage 3 deferred module evaluation proposal, including what works, limitations, implementation approach, testing strategy, and references.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/ast/ImportScanner.zig (1)

124-137: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Deferred namespace can still be stripped before this phase guard runs.

Line 204 protects only convert_star_to_clause, but Line 124–137 still clears st.star_name_loc for unused star imports regardless of phase. With trim_unused_imports, that can cascade into full import removal at Line 188–190, which breaks import defer * as ns ... semantics.

Proposed fix
-                        if (symbol.use_count_estimate == 0) {
+                        if (symbol.use_count_estimate == 0 and record.phase == .evaluation) {
                             // Make sure we don't remove this if it was used for a property
                             // access while bundling
                             var has_any = false;

Also applies to: 201-205

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ast/ImportScanner.zig` around lines 124 - 137, The code clears
st.star_name_loc for unused star imports without respecting deferred-import
semantics; only clear st.star_name_loc (and set did_remove_star_loc) when the
import is not deferred — i.e., apply the same phase/ defer guard used in
convert_star_to_clause. Update the block that checks
symbol.use_count_estimate==0 and the similar block at lines ~201-205 to first
check the import's defer/phase flag (e.g., st.is_deferred or whatever field
indicates "defer") before modifying st.star_name_loc or did_remove_star_loc, and
ensure these checks use the same condition as convert_star_to_clause and
trim_unused_imports to prevent premature stripping of deferred namespace
imports.
src/js_printer.zig (1)

1859-1871: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Preserve import phase in the bake-dev dynamic import path.

These branches gained phase-aware printing for normal output, but the .internal_bake_dev fallback still hard-codes hmr.dynamicImport(...). That means import.defer() and import.source() are downgraded to evaluation-phase imports in dev-server modules, so behavior now differs between bake-dev and non-dev builds. Thread record.phase / e.phase through the HMR runtime API, or reject those phases in dev mode until that support exists.

Also applies to: 2428-2437

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/js_printer.zig` around lines 1859 - 1871, The dev-server branch currently
prints a hard-coded HMR call (using p.printSymbol(p.options.hmr_ref);
p.print(".dynamicImport("); ...) which ignores record.phase and thus downgrades
import.defer/import.source to plain evaluation; update the .internal_bake_dev
branch to preserve the import phase by branching on record.phase (same cases
used above that call
p.print("import(")/p.print("import.defer(")/p.print("import.source(")) and then
emit the HMR runtime call with the phase passed through (e.g., include the phase
token or an explicit enum argument), or if the HMR runtime cannot accept phases
yet, explicitly reject .defer_/.source phases in .internal_bake_dev with a clear
error. Apply the same fix at the second occurrence (around the other block
corresponding to lines 2428-2437) to keep behavior consistent.
src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp (1)

290-311: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t collapse source phase into evaluation in debug serialization

The ternary maps every non-defer phase to "evaluation", which can hide phase mismatches in fallbackParse diffing.

Suggested fix
-            const char* phase = request.m_phase == AbstractModuleRecord::ModulePhase::Defer ? "defer" : "evaluation";
+            const char* phase = "evaluation";
+            switch (request.m_phase) {
+            case AbstractModuleRecord::ModulePhase::Evaluation:
+                phase = "evaluation";
+                break;
+            case AbstractModuleRecord::ModulePhase::Defer:
+                phase = "defer";
+                break;
+            case AbstractModuleRecord::ModulePhase::Source:
+                phase = "source";
+                break;
+            }
@@
-            const char* phase = importEntry.phase == AbstractModuleRecord::ModulePhase::Defer ? "defer" : "evaluation";
+            const char* phase = "evaluation";
+            switch (importEntry.phase) {
+            case AbstractModuleRecord::ModulePhase::Evaluation:
+                phase = "evaluation";
+                break;
+            case AbstractModuleRecord::ModulePhase::Defer:
+                phase = "defer";
+                break;
+            case AbstractModuleRecord::ModulePhase::Source:
+                phase = "source";
+                break;
+            }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp` around lines 290 - 311,
The debug serialization currently collapses any non-defer phase to "evaluation"
which hides the distinct "source" phase; update the phase-to-string logic used
for request.m_phase (in the loop building sortedDeps) and importEntry.phase (in
the importEntries serialization) to explicitly handle
AbstractModuleRecord::ModulePhase::Source and return "source", keep "defer" for
Defer and "evaluation" for the actual Evaluation phase so phase mismatches (e.g.
in fallbackParse diffs) are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ast/parseStmt.zig`:
- Around line 1012-1034: Early returns for the "defer" and "source"
import-patterns in parseStmt.zig bypass the existing TypeScript
namespace/import-assignment gate, letting `import defer/source ...` parse where
ESM imports should be blocked; update the code so the
namespace/import-assignment restriction that appears earlier in parseStmt is
invoked before handling the `.t_asterisk` ("defer") and `.t_identifier`
("source") branches (or move the branches after that gate) so you don’t return
early; ensure you still call the same helpers (p.storeNameInRef, p.parsePath,
p.processImportStatement, p.lexer.expectOrInsertSemicolon) only after the gate
check passes.

In `@src/js_printer.zig`:
- Around line 4727-4735: The deferred-import branch drops fetch parameters by
calling mi.addRequestedModuleDefer(irp_id) while the non-defer branch uses
mi.requestModule(irp_id, fetch_parameters), which causes loss of record.loader
metadata; either extend the defer API to accept and propagate fetch_parameters
or guard the defer case by asserting record.loader == null. Update the .defer_
branch in the code handling record.phase (where addRequestedModuleDefer and
requestModule are invoked) to pass fetch_parameters into the defer-recording API
if you implement the plumbing, or add an explicit assertion (or panic) that
record.loader == null for .defer_ until the defer API is extended so deferred
imports cannot silently drop loader info.

---

Outside diff comments:
In `@src/ast/ImportScanner.zig`:
- Around line 124-137: The code clears st.star_name_loc for unused star imports
without respecting deferred-import semantics; only clear st.star_name_loc (and
set did_remove_star_loc) when the import is not deferred — i.e., apply the same
phase/ defer guard used in convert_star_to_clause. Update the block that checks
symbol.use_count_estimate==0 and the similar block at lines ~201-205 to first
check the import's defer/phase flag (e.g., st.is_deferred or whatever field
indicates "defer") before modifying st.star_name_loc or did_remove_star_loc, and
ensure these checks use the same condition as convert_star_to_clause and
trim_unused_imports to prevent premature stripping of deferred namespace
imports.

In `@src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp`:
- Around line 290-311: The debug serialization currently collapses any non-defer
phase to "evaluation" which hides the distinct "source" phase; update the
phase-to-string logic used for request.m_phase (in the loop building sortedDeps)
and importEntry.phase (in the importEntries serialization) to explicitly handle
AbstractModuleRecord::ModulePhase::Source and return "source", keep "defer" for
Defer and "evaluation" for the actual Evaluation phase so phase mismatches (e.g.
in fallbackParse diffs) are preserved.

In `@src/js_printer.zig`:
- Around line 1859-1871: The dev-server branch currently prints a hard-coded HMR
call (using p.printSymbol(p.options.hmr_ref); p.print(".dynamicImport("); ...)
which ignores record.phase and thus downgrades import.defer/import.source to
plain evaluation; update the .internal_bake_dev branch to preserve the import
phase by branching on record.phase (same cases used above that call
p.print("import(")/p.print("import.defer(")/p.print("import.source(")) and then
emit the HMR runtime call with the phase passed through (e.g., include the phase
token or an explicit enum argument), or if the HMR runtime cannot accept phases
yet, explicitly reject .defer_/.source phases in .internal_bake_dev with a clear
error. Apply the same fix at the second occurrence (around the other block
corresponding to lines 2428-2437) to keep behavior consistent.
🪄 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: b9cc876f-b269-4317-948c-3ad58ab4e41f

📥 Commits

Reviewing files that changed from the base of the PR and between 360bbb5 and 9f1813c.

📒 Files selected for processing (14)
  • src/analyze_transpiled_module.zig
  • src/ast/E.zig
  • src/ast/ImportScanner.zig
  • src/ast/P.zig
  • src/ast/parseImportExport.zig
  • src/ast/parseStmt.zig
  • src/ast/visitExpr.zig
  • src/bun.js/WebKit-import-defer.patch
  • src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp
  • src/bun.zig
  • src/import_record.zig
  • src/js_parser.zig
  • src/js_printer.zig
  • test/js/bun/import-defer/import-defer.test.ts

Comment thread src/ast/parseStmt.zig Outdated
Comment thread src/js_printer.zig
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bun.js/WebKit-import-defer.patch`:
- Around line 353-391: The current
AbstractModuleRecord::gatherAsynchronousTransitiveDependencies uses native
recursion and should be rewritten as an explicit iterative traversal to avoid
C++ stack overflow; replace the recursive depth-first walk in
gatherAsynchronousTransitiveDependencies (and its use of
dynamicDowncast<CyclicModuleRecord>, module->requestedModules(), and
JSModuleLoader::getImportedModule) with an explicit stack-driven DFS that mimics
the original post-order semantics: maintain the existing seen
UncheckedKeyHashSet, push nodes to a stack for processing, when visiting a
CyclicModuleRecord check status and hasTLA() the same way (if hasTLA() append to
result and do not traverse its children), otherwise push its children (via
JSModuleLoader::getImportedModule) onto the stack for later processing, and
ensure modules are appended to result in the same order as the recursive version
while preventing revisits; you can follow the iterative pattern used in
readyForSyncExecution() or innerModuleEvaluation to preserve guarded recursion
behavior.

In `@test/js/bun/import-defer/import-defer.test.ts`:
- Around line 6-17: The run() helper currently returns raw trimmed stderr which
causes flaky failures when ASAN/debug lanes emit "WARNING: ASAN interferes..."
lines; update run (the async function run, returning { stdout, stderr, exitCode
} and using proc from Bun.spawn) to filter out ASAN warning lines from
proc.stderr before trimming/returning (e.g., remove lines matching /WARNING:
ASAN interferes/ or similar patterns) so tests expecting empty stderr won't fail
spuriously while leaving all other stderr content intact.
🪄 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: 95f0ee46-87b1-478f-bb72-8e8e33764291

📥 Commits

Reviewing files that changed from the base of the PR and between 9f1813c and 64450b5.

📒 Files selected for processing (2)
  • src/bun.js/WebKit-import-defer.patch
  • test/js/bun/import-defer/import-defer.test.ts

Comment thread src/bun.js/WebKit-import-defer.patch Outdated
Comment thread test/js/bun/import-defer/import-defer.test.ts
@alii
Copy link
Copy Markdown
Member

alii commented Apr 30, 2026

@robobun You committed a WebKit patch file. The patches should likely exist in a PR to oven-sh/WebKit directly.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/bun.js/WebKit-import-defer.patch (1)

353-391: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Replace this recursive traversal before merge.

gatherAsynchronousTransitiveDependencies() still walks user-controlled import depth with native recursion and without the stack guard used by innerModuleEvaluation(). A sufficiently deep deferred graph can overflow the C++ stack before the guarded path is reached. Please switch this to an explicit iterative DFS/BFS, similar to readyForSyncExecution().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/WebKit-import-defer.patch` around lines 353 - 391,
gatherAsynchronousTransitiveDependencies currently performs an unguarded
recursive DFS (walking user-controlled import depth) and can overflow the C++
stack; replace the recursion with an explicit iterative traversal using a
stack/queue and the same stack-guarding pattern used by
readyForSyncExecution/innerModuleEvaluation. Specifically, keep using the same
checks and symbols (CyclicModuleRecord::status(), hasTLA(), ModuleRequest loop,
JSModuleLoader::getImportedModule()) but implement the algorithm with an
explicit work stack of AbstractModuleRecord* and the seen UncheckedKeyHashSet,
pushing children to the stack and appending modules to result when encountering
a module with hasTLA() or otherwise finishing exploring its dependencies; ensure
you use the same guarded call pattern (stack guard) used by
readyForSyncExecution/innerModuleEvaluation so deep graphs cannot cause native
recursion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bun.js/WebKit-import-defer.patch`:
- Around line 623-639: The current isDeferred branch creates and returns a
branded deferred namespace (via JSPromise::create and
performPromiseThenWithInternalMicrotask with
InternalMicrotask::ImportDeferModuleNamespace) even when the module body has
already been evaluated, which is non‑spec for side‑effectful modules; change the
gating so import.defer() (the dynamic form) only takes this path when the
dynamic import path is actually lazy — i.e., when ContinueDynamicImport is
propagating ModulePhase through the loader — otherwise fall back to the normal
(non‑deferred) dynamic import behavior or reject the dynamic form until
ModulePhase support exists; in practice ensure the condition checking isDeferred
also verifies the loader/runtime supports lazy dynamic evaluation (or a feature
flag indicating ModulePhase threading), and only then create the JSPromise and
call performPromiseThenWithInternalMicrotask, leaving existing evaluated modules
to return the regular namespace.

---

Duplicate comments:
In `@src/bun.js/WebKit-import-defer.patch`:
- Around line 353-391: gatherAsynchronousTransitiveDependencies currently
performs an unguarded recursive DFS (walking user-controlled import depth) and
can overflow the C++ stack; replace the recursion with an explicit iterative
traversal using a stack/queue and the same stack-guarding pattern used by
readyForSyncExecution/innerModuleEvaluation. Specifically, keep using the same
checks and symbols (CyclicModuleRecord::status(), hasTLA(), ModuleRequest loop,
JSModuleLoader::getImportedModule()) but implement the algorithm with an
explicit work stack of AbstractModuleRecord* and the seen UncheckedKeyHashSet,
pushing children to the stack and appending modules to result when encountering
a module with hasTLA() or otherwise finishing exploring its dependencies; ensure
you use the same guarded call pattern (stack guard) used by
readyForSyncExecution/innerModuleEvaluation so deep graphs cannot cause native
recursion.
🪄 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: 2fb190b3-c674-48a0-a992-e3f64adbcf10

📥 Commits

Reviewing files that changed from the base of the PR and between 64450b5 and 4f56ffe.

📒 Files selected for processing (3)
  • src/bun.js/WebKit-import-defer.patch
  • src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp
  • test/js/bun/import-defer/import-defer.test.ts

Comment thread src/bun.js/WebKit-import-defer.patch Outdated
Comment thread test/js/bun/import-defer/import-defer.test.ts
@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 30, 2026

You committed a WebKit patch file. The patches should likely exist in a PR to oven-sh/WebKit directly.

Agreed — I don't have push access to oven-sh/WebKit, so I checked the patch in here as a handoff artifact. The commit is ready to go on a local branch in vendor/WebKit/ (farm/4846b6a5/import-defer, SHA 814ed7f62f, 19 files, +383/−39).

To land it:

cd vendor/WebKit
git am ../../src/bun.js/WebKit-import-defer.patch
git push origin HEAD:robobun/import-defer
gh pr create -R oven-sh/WebKit ...

Once that merges and a prebuilt tarball is published, this PR just needs WEBKIT_VERSION bumped in scripts/build/deps/webkit.ts and src/bun.js/WebKit-import-defer.patch deleted. The Bun-side C++ is already guarded with #ifdef JSC_HAS_IMPORT_DEFER so CI builds against the current prebuilt in the meantime; the runtime test suite probes for support and skips itself until the bump.

@alii
Copy link
Copy Markdown
Member

alii commented Apr 30, 2026

@robobun Do you have a fork you can push to? I've seen you make PRs before.

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 30, 2026

Found my fork — opened oven-sh/WebKit#206 with the JSC changes and dropped the checked-in .patch from this PR in b758f93.

Comment thread src/js_printer/js_printer.zig
Comment thread src/js_parser/ast/ImportScanner.zig
Comment thread src/js_parser/ast/parseImportExport.zig
Comment thread src/ast/parseStmt.zig Outdated
Comment thread src/analyze_transpiled_module.zig Outdated
Comment thread test/js/bun/import-defer/import-defer.test.ts Outdated
@robobun robobun force-pushed the farm/4846b6a5/import-defer branch from cb4dfdc to ca27913 Compare May 1, 2026 00:29
Comment thread src/bundler/analyze_transpiled_module.zig
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/analyze_transpiled_module.zig`:
- Around line 348-349: The current branch treats .import_info_namespace_defer
the same as .import_info_namespace and rewrites deferred namespace imports into
module re-exports; change the conditional so only .import_info_namespace
triggers the local_name_to_module_name.put(...) rewrite and do not rewrite
.import_info_namespace_defer here (leave the deferred namespace on the
local-export path until export records become phase-aware). Concretely, update
the if/else handling of k so .import_info_namespace_defer is excluded from the
put call (or handled separately), keeping the existing .is_namespace behavior
but preserving the deferred semantics.

In `@src/ast/ImportScanner.zig`:
- Around line 123-132: The current guard only exempts star-imports via
is_unused_in_typescript when record.phase != .evaluation, but default/named
bindings and the later whole-statement cull still run and can drop
non-evaluation imports; update the pruning logic so that any default/item
pruning and the whole-statement removal that checks symbol.use_count_estimate ==
0 also requires record.phase == .evaluation (i.e., gate those branches on
record.phase == .evaluation similarly to the is_unused_in_typescript check) so
non-evaluation imports are not removed; locate references to
is_unused_in_typescript, record.phase, and symbol.use_count_estimate in
ImportScanner.zig and add the same phase check around the default/item pruning
and the statement cull.
- Around line 209-212: The current convert_star_to_clause condition turns
deferred namespace imports into named bindings and thus breaks "import defer *
as ns" evaluation-on-first-property-access; update the condition used for
convert_star_to_clause to also require that the namespace is not a
deferred/evaluation-only namespace. Concretely, when computing
convert_star_to_clause (the const using p.options.bundle, record.phase, and
p.symbols.items[namespace_ref.innerIndex()].use_count_estimate), add a guard
that checks st.namespace_ref (or the record/st field that marks the import as
deferred) and that the corresponding symbol/item is not marked as
"deferred/evaluate-on-property-access" (use the existing flag on the symbol or
record such as a deferred/evaluation-only boolean if present); only allow
converting * to named clauses when that deferred/eval-only condition is false so
existing_items and namespace_alias lowering is skipped for deferred namespaces.

In `@src/js_printer.zig`:
- Around line 1860-1864: The switch that emits "import(", "import.defer(", and
"import.source(" only covers the direct ESM path (via record.phase) but other
code paths—specifically the branch that checks record.source_index.isValid(),
the .internal_bake_dev branch, and the printBundledImport()
implementation—currently ignore record.phase and degrade phase imports to eager
imports; update those paths to honor record.phase by emitting the correct
wrapped call (import, import.defer, import.source) or, where preserving
semantics is impossible (bundled/internal printer modes), detect phase imports
early and fail fast with a clear error; locate uses of record.phase, the
source_index.isValid() branch, the internal_bake_dev handling, and
printBundledImport() and either propagate phase into their logic so they emit
phase-aware imports or add explicit validation that rejects unsupported phase
imports.
🪄 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: 710b60d5-8418-40cc-a84d-47c7939fbb6a

📥 Commits

Reviewing files that changed from the base of the PR and between 4f56ffe and fc98254.

📒 Files selected for processing (13)
  • src/analyze_transpiled_module.zig
  • src/ast/E.zig
  • src/ast/ImportScanner.zig
  • src/ast/P.zig
  • src/ast/parseImportExport.zig
  • src/ast/parseStmt.zig
  • src/ast/visitExpr.zig
  • src/bun.js/bindings/BunAnalyzeTranspiledModule.cpp
  • src/bun.zig
  • src/import_record.zig
  • src/js_parser.zig
  • src/js_printer.zig
  • test/js/bun/import-defer/import-defer.test.ts

Comment thread src/analyze_transpiled_module.zig Outdated
Comment thread src/ast/ImportScanner.zig Outdated
Comment thread src/js_parser/ast/ImportScanner.zig
Comment thread src/js_printer/js_printer.zig
Comment thread src/js_parser/ast/parseImportExport.zig
Comment thread test/js/bun/import-defer/import-defer.test.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/analyze_transpiled_module.zig`:
- Line 183: ReplaceStringID() currently rewrites only requested_modules but not
requested_modules_defer, causing stale old_id entries and duplicate
.requested_module_defer records; update replaceStringID() to mirror the same
key-rewrite/reindex logic used for requested_modules so it also
rewrites/reindexes entries in requested_modules_defer (the same way
addRequestedModuleDefer() uses a second key store for dedupe), ensuring old_id
keys are removed and new_id keys are inserted/updated to keep both maps in sync.
🪄 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: d82899cd-ca3c-40ad-8e2b-076c6eb845b7

📥 Commits

Reviewing files that changed from the base of the PR and between fc98254 and 81922c4.

📒 Files selected for processing (4)
  • src/analyze_transpiled_module.zig
  • src/ast/ImportScanner.zig
  • src/ast/parseImportExport.zig
  • test/js/bun/import-defer/import-defer.test.ts

Comment thread src/bundler/analyze_transpiled_module.zig
Comment thread src/js_parser/ast/ImportScanner.zig
@robobun robobun force-pushed the farm/4846b6a5/import-defer branch from 949316e to 8217a2b Compare May 1, 2026 09:10
Comment thread src/ast/parseStmt.zig Outdated
Comment thread test/js/bun/import-defer/import-defer.test.ts
Comment thread src/js_parser/ast/E.zig
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All prior findings from earlier review rounds have been addressed — nothing new flagged on the current revision. That said, this is a substantial Stage 3 language-feature implementation spanning the parser, ImportScanner, printer, the ModuleInfo↔JSC bridge, and C++ bindings, with a hard cross-repo dependency on oven-sh/WebKit#206 and several documented known-limitations (dynamic import.defer() eager-eval, bundler/Bake degradation, mixed-phase [[RequestedModules]] ordering). Worth a human sign-off on the scope and the deferred-follow-up boundaries before merge.

Extended reasoning...

Overview

This PR implements TC39 Stage 3 import defer across 14 files: a new ImportPhase enum on ImportRecord/E.Import, parser support in parseStmt.zig/parseImportExport.zig, phase-aware unused-import handling in ImportScanner.zig, printer emission in js_printer.zig, new ModuleInfo record kinds and JSC extern bindings in analyze_transpiled_module.zig + BunAnalyzeTranspiledModule.cpp, plus threading through processImportStatement/transposeImport/visitExpr/repl_transforms. It depends on oven-sh/WebKit#206 for the JSC runtime semantics; the Bun-side C++ is #ifdef JSC_HAS_IMPORT_DEFER-guarded and the runtime test suite probes for support and skips until WEBKIT_VERSION is bumped. ~405 lines of new tests cover evaluation ordering, namespace branding, error re-throw, TLA eager-eval, dedup, re-export, syntax disambiguation, and transpiler round-trips.

Security risks

None identified. This is a language-feature/parser change with no auth, crypto, network, or filesystem-permission surface. The C++ additions are thin extern wrappers around existing JSModuleRecord APIs.

Level of scrutiny

High. This touches production-critical parser and module-loading code paths that every Bun user hits. The ModuleInfo bridge has a strict invariant (debug-build dumpRecordInfo() diff against JSC's ModuleAnalyzer) that several earlier review rounds caught violations of (dedup, re-export classification, ordering). The feature has documented partial-implementation boundaries (dynamic import.defer() evaluates eagerly; bundler/Bake silently degrade phase imports; mixed-phase [[RequestedModules]] ordering loses source interleaving) that are design decisions about acceptable scope for an initial landing.

Other factors

  • The PR has gone through ~9 rounds of bug-hunter findings, all addressed (defer-phase dedup, finalize() re-export classification, esm_import_keyword Script-context leak, ImportScanner phase guards at multiple sites, processImportStatement namespace-registration gate, parseStmt TS-namespace gate, REPL phase threading, test gating). All inline threads are resolved.
  • A human reviewer (alii) is already engaged on the cross-repo WebKit-PR coordination.
  • The known-limitations list (dynamic-form eager-eval, bundler/Bake, ordering) represents intentional scope boundaries that a maintainer should explicitly accept before merge, particularly whether bundler should fail-fast vs. silently degrade.
  • Runtime tests are skip-gated on WebKit support, so most CI coverage is transpiler-only until the version bump — the actual runtime semantics won't be CI-exercised on this PR's own build.

@robobun robobun force-pushed the farm/4846b6a5/import-defer branch from 383e592 to 0c48fca Compare May 4, 2026 17:22
Comment thread src/js_parser/ast/repl_transforms.zig
@robobun robobun force-pushed the farm/4846b6a5/import-defer branch 3 times, most recently from 3f3fb68 to ea63dc7 Compare May 5, 2026 22:09
Comment thread src/js_parser/ast/E.zig
robobun and others added 14 commits May 7, 2026 01:04
Adds support for the 'import defer' proposal
(https://github.com/tc39/proposal-defer-import-eval):

  import defer * as ns from './mod.js';
  const ns = await import.defer('./mod.js');

The module and its dependency graph are fetched and linked up-front,
but top-level evaluation of synchronous modules is delayed until the
first property access on the deferred namespace object. Modules with
top-level await (and their ancestors) are still evaluated eagerly so
that the on-access evaluation can complete synchronously.

Transpiler:
- ImportPhase enum on ImportRecord and E.Import (evaluation / defer /
  source). parseStmt recognises 'import defer *' and 'import source x';
  parseImportExpr recognises 'import.defer(...)' and
  'import.source(...)'. js_printer round-trips both forms.
- ImportScanner keeps the star binding for deferred namespace imports
  even when apparently unused; evaluation is a side effect.

ModuleInfo bridge:
- New RecordKind values import_info_namespace_defer and
  requested_module_defer, threaded through to
  BunAnalyzeTranspiledModule.cpp so the debug
  parseFromSourceCode/fallbackParse diff stays clean.

JavaScriptCore (vendor/WebKit patch at
src/bun.js/WebKit-import-defer.patch):
- ModulePhase on ModuleRequest/ImportEntry; deferred namespace object
  with @@toStringTag = 'Deferred Module', hidden 'then', and
  evaluate-on-access via ReadyForSyncExecution + module.Evaluate().
- innerModuleEvaluation builds the spec's evaluationList, using
  GatherAsynchronousTransitiveDependencies for defer-phase requests.
- Parser accepts 'import.defer(' and ImportNode carries the phase
  through to globalFuncImportModule as a third argument; a new
  InternalMicrotask::ImportDeferModuleNamespace swaps the result to
  the deferred namespace.
- useImportDefer flipped to true.

Known limitation: the dynamic form import.defer() currently still
runs the module body eagerly (only the returned namespace object is
the deferred one). Fully deferring evaluation for the dynamic form
requires threading ModulePhase through ContinueDynamicImport and the
moduleLoaderImportModule embedder hook; tracked with a test.todo.
Replaces marker-style comments with plain prose describing the
remaining work for full lazy evaluation of the dynamic form, and
regenerates the WebKit patch accordingly.
The prebuilt WebKit tarball used by CI does not yet have
AbstractModuleRecord::ModulePhase. Define JSC_HAS_IMPORT_DEFER in the
WebKit patch alongside the enum and gate the Bun-side C++ on it so
this translation unit compiles against both old and new headers.

When building against the old headers the defer externs fall back to
evaluation-phase entries; the printed source still contains
'import defer * as ...' and JSC's own parser will reject it, so the
runtime tests probe for support and skip themselves. The
transpiler-only tests remain unconditional and continue to fail on
system bun.
Convert the deferred-subgraph TLA collector to an iterative DFS so
deep import graphs don't risk native stack overflow before
innerModuleEvaluation's guarded recursion is reached. Children are
pushed in reverse to preserve source-order visitation.
The JavaScriptCore changes now live in
oven-sh/WebKit#206. The Bun-side C++ is
already guarded with JSC_HAS_IMPORT_DEFER so this PR builds against
the current prebuilt WebKit; once #206 merges and a tarball is
published, bump WEBKIT_VERSION here and the runtime tests will stop
auto-skipping.
- ModuleInfo.addRequestedModuleDefer now dedupes per specifier via a
  requested_modules_defer set, matching JSC's ModuleAnalyzer so the
  debug fallbackParse() diff stays clean for
  'import defer * as a from "x"; import defer * as b from "x";'.
  Added a runtime test for this case.
- ImportScanner: also guard the earlier star-import strip site and the
  TypeScript is_unused_in_typescript culling on record.phase so a
  truly-unused 'import defer * as ns' in a .ts file is preserved.
- parseImportExpr: mention defer/source in the 'import.<ident>'
  diagnostic.
- Test comment now points at oven-sh/WebKit#206 instead of the removed
  .patch file.
finalize()'s local_name -> module_name map is used to convert
export_info_local to export_info_indirect (dispatched to
addNamespaceExport when import_name is .star_namespace). It covered
.import_info_namespace but not .import_info_namespace_defer, so

    import defer * as ns from './x.mjs';
    export { ns };

produced a Local export where JSC's ModuleAnalyzer (which is
phase-agnostic in exportVariable()) produces a Namespace export,
failing the BUN_DEBUG dumpRecordInfo() diff once JSC_HAS_IMPORT_DEFER
is available.
- ModuleInfo.finalize: keep .import_info_namespace_defer out of the
  local→indirect rewrite. A deferred namespace object lives in this
  module's environment, so export { ns } must stay a Local export
  (proposal ParseModule 11.a.ii). Paired with the matching fix to
  JSC ModuleAnalyzer::exportVariable in oven-sh/WebKit#206.
- ImportScanner: hoist the non-evaluation phase guard so it also
  covers the default-binding prune (import source x) and the
  whole-statement TS cull, not just the star path. Gate the
  bundle-mode namespace-property lowering on evaluation phase.
- Tests: re-exported deferred namespace stays deferred; unused
  phase imports survive TS stripping.
import.defer() and import.source() extend ImportCall, which is valid
in Script context just like plain import(). Setting esm_import_keyword
for them wrongly marked the containing file as having ESM syntax,
flipping top-level this/exports_kind detection in otherwise-CJS code.

Test observes via Bun.Transpiler's top-level-this rewrite: plain
import() and import.defer()/import.source() both keep this->exports,
while import.meta rewrites it away.

Also: describe.concurrent for the subprocess-spawning runtime tests.
…orts

The bundle-path namespace-lowering guard added in fa26e6a sat in
ImportScanner, which runs post-visit. By then maybeRewritePropertyAccess
has already rewritten ns.foo -> E.ImportIdentifier (keyed purely on
import_items_for_namespace membership), so the guard just orphaned the
synthesized symbol (no namespace_alias, not in named_imports) and the
printer emitted a bare undeclared identifier.

Gate at the source instead: don't register non-evaluation-phase
namespaces in import_items_for_namespace, so maybeRewritePropertyAccess
never finds them and ns.foo stays an E.Dot through the pipeline. For
defer/source the skipped item_refs map is always empty (no named
clauses in those grammars), so nothing is lost. The downstream
ImportScanner guard becomes a no-op and is kept for clarity.
The TypeScript import-equals gate is
  token == .t_equals OR is_export OR (is_namespace_scope AND !is_typescript_declare)
not just .t_equals. The second and third terms fire regardless of the
current token and route to parseTypeScriptImportEqualsStmt which
immediately expects '=', producing the parse error for ESM imports
inside a non-declare TS namespace or after 'export import'.

The defer/source early-return branches now require the negation of
those non-token terms so they fall through to the gate instead of
emitting a real S.Import in contexts where every other import form
errors. Tests cover namespace Foo { import defer/source ... } and
export import defer/source ..., plus the declare-namespace exemption.
The REPL lowers static imports to dynamic ones but was constructing
E.Import{} without .phase, so import defer * as ns became plain
await import(...). Copy the record's phase so it lowers to
await import.defer(...).

Also: move the 'import defer from' back-compat and 'import defer {x}'
syntax-error tests out of the describeRuntime gate into their own
block — they exercise Bun's parser only (phase stays .evaluation /
error fires before JSC), so they run on prebuilt WebKit too.

Tests: REPL phase preservation; 10 pass / 10 skip / 1 todo.
The .e_import arm enumerated fields explicitly (pre-dating .phase);
without this a deep-cloned record-less import.defer()/import.source()
would silently revert to .evaluation. Currently unreachable (only
caller deep-clones JSON-parsed define values), but completes the
E.Import construction-site audit alongside repl_transforms.zig.
@robobun robobun force-pushed the farm/4846b6a5/import-defer branch from ebed833 to c4a63cd Compare May 7, 2026 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants