Implement source phase imports (TC39 Stage 3)#31677
Conversation
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. 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 ImportPhase and threads import phase through AST, parser, scanner, printer, bundler/CSS, transpose/REPL, runtime JSC wasm handling, and tests to implement TC39 source-phase imports for WebAssembly. ChangesSource-Phase Imports Feature
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 2:27 AM PT - Jun 20th, 2026
❌ @robobun, your commit 39c996b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31677That installs a local version of the PR into your bun-31677 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/runtime/jsc_hooks.rs`:
- Around line 3148-3235: The source-phase `.wasm` branch returns an
OwnedResolvedSource without registering the module with the file-watcher, so add
the same watcher registration used by the non-source `.wasm`/Loader::File path
before returning: invoke the existing auto-watch registration helper (the same
code path that registers resolved sources with the watcher) with the resolved
specifier and path.text (use input_specifier and path.text) and guard it with
the same watch/hot check the other branch uses, then return the
OwnedResolvedSource as before.
In `@test/js/bun/resolve/import-source-phase.test.ts`:
- Around line 390-391: Swap the two assertions so stderr is checked before the
exit code: replace the sequence expect(exitCode).not.toBe(0);
expect(stderr.toLowerCase()).toContain("error"); with
expect(stderr.toLowerCase()).toContain("error"); expect(exitCode).not.toBe(0);
in the failing-syntax tests (the blocks containing expect(exitCode).not.toBe(0)
and expect(stderr.toLowerCase()).toContain("error")), and make the same swap for
the second occurrence of those assertions referenced in the comment (the other
test with the same two expects).
🪄 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: 3840fbeb-c892-4b29-851b-441de6a0f60f
📒 Files selected for processing (25)
src/ast/e.rssrc/ast/expr.rssrc/ast/import_record.rssrc/ast/lib.rssrc/ast/s.rssrc/bundler/AstBuilder.rssrc/bundler/HTMLScanner.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/css/css_parser.rssrc/js_parser/lower/lower_esm_exports_hmr.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parse/parse_import_export.rssrc/js_parser/parse/parse_stmt.rssrc/js_parser/parser.rssrc/js_parser/repl_transforms.rssrc/js_parser/scan/scan_imports.rssrc/js_parser/visit/visit_expr.rssrc/js_printer/lib.rssrc/jsc/ModuleLoader.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/ModuleLoader.cppsrc/runtime/jsc_hooks.rstest/js/bun/resolve/import-source-phase.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/p.rs:4177-4204— The source/evaluation phase conflict check only runs inprocess_import_statement(called forS::Import), butexport { x } from "..."andexport * from "..."addImportKind::Stmtrecords directly int_exportwithout going through it. Soimport source mod from "./a.wasm"; export { default as x } from "./a.wasm";(in that order) bypasses the check, while the reverse order is caught — making the guard order-dependent. Consider running the same scan whent_exportadds an export-from/export-star record (gated onhas_source_phase_import_stmtso the common case stays a single bool check).Extended reasoning...
What the bug is
The PR adds a parse-time check in
process_import_statement(src/js_parser/p.rs:4177-4204) that rejects a file requesting the same specifier at both source phase and evaluation phase. The PR description explains why: JSC dedups a module record's requested modules by(specifier, ModulePhase)ignoring import attributes, and the source phase is lowered onto an Evaluation-phase request withtype: "webassembly"attributes — so if both requests appeared in one file, whichever came first would win and the other binding would silently receive the wrong value.However, this check only runs inside
process_import_statement, which is only called forS::Importstatements (fromt_importin parse_stmt.rs). It does not run forexport { x } from "..."orexport * from "...". Those are handled int_export'sT::TOpenBraceandT::TAsteriskarms, which callp.add_import_record(ImportKind::Stmt, ...)directly and returnS::ExportFrom/S::ExportStarwithout ever enteringprocess_import_statement.add_import_recorditself defaultsphasetoImportPhase::Evaluationand performs no conflict scan.The specific code path
Consider:
import source mod from "./add.wasm"; export { default as x } from "./add.wasm";
t_importparses the first statement, callsprocess_import_statement. This setshas_source_phase_import_stmt = true, adds record 0 withphase = Source, and scansrecords[..0](empty) — no conflict.t_exportparses the second statement, hits theT::TOpenBracearm, seesfrom, and callsp.add_import_record(ImportKind::Stmt, parsed_path.loc, parsed_path.text). This adds record 1 withphase = Evaluationandkind = Stmt, then returnsS::ExportFrom.process_import_statementis never called, so the conflict scan never sees record 1.
The file parses successfully with two
ImportKind::Stmtrecords for./add.wasmat conflicting phases — exactly the case the check was added to prevent.Why the reverse order IS caught (order-dependence)
If the statements are swapped:
export { default as x } from "./add.wasm"; import source mod from "./add.wasm";
t_exportadds record 0 (Evaluation,kind = Stmt) with no check.t_importcallsprocess_import_statementfor theimport source. This setshas_source_phase_import_stmt = true, adds record 1 (Source), then scansrecords[..1]. Record 0 haskind == Stmtand(Evaluation, Source)matches the conflict pattern → parse error.
So the check catches one ordering and misses the other — it's order-dependent, which a safety guard shouldn't be.
Why existing code doesn't prevent it
The check is structured as: on each
S::Import, after appending the new record, scan all prior records for a phase conflict. This works when both conflicting records flow throughprocess_import_statement(the second one's scan sees the first). But export-from records are appended outside this function with no scan of their own, so when the export-from comes second, nothing ever scans it against the earlier source-phase record. Thehas_source_phase_import_stmtflag is set, but no code path checks it for export-from statements.Impact
At runtime, both records lower to Evaluation-phase requests of
./add.wasm(the source-phase one via the printedwith { type: "webassembly" }attribute, the export-from via no attribute). Per the comment at p.rs:4165-4173 and the ModuleInforequest_module_with_phaseinsert-if-absent logic, JSC keeps only the first request's fetch parameters. The export-from's re-export bindingxwould then resolve against whichever module-map entry won — silently re-exporting aWebAssembly.Moduleinstead of the file-loader path string (or vice versa), with no error. This is precisely the "silently wrong value" the PR's "Errors instead of wrong values" section says it guards against.That said, the trigger is contrived: a single file that both
import sources a.wasmand re-exports the same specifier at evaluation phase viaexport ... from, in that specific order. This is unlikely in practice, which is why this is filed as a nit rather than a blocking issue.How to fix
Run the same conflict scan when
t_exportadds anImportKind::Stmtrecord forexport { ... } from/export * from. The cheapest change is to factor the scan into a small helper onP(e.g.check_phase_conflict(new_index)) and call it from bothprocess_import_statementand the twot_exportarms afteradd_import_record, gated onself.has_source_phase_import_stmtso files without source-phase imports still pay only a single bool check. Since export-from/export-star records are always Evaluation-phase, the gate means the scan only runs in the rare case a source-phase import has already been seen — preserving the PR's stated perf goal.
|
Addressed the review note about the phase-conflict guard being order-dependent: |
There was a problem hiding this comment.
No bugs found, but this is a substantial feature implementation (new TC39 syntax across parser/printer/module-loader plus a new C++ FFI binding) with several design choices worth a human look — and CI shows build-rust failures on 90ac099 that should be green before merge.
Extended reasoning...
Overview
This PR implements TC39 Source Phase Imports (Stage 3) — both import source x from "..." and import.source(...) — across 25 files. It generalizes the prior phase_defer: bool into an ImportPhase enum on S::Import, E::Import, and ImportRecord; adds contextual-keyword parsing with one-token lookahead for the import source from from ambiguity; lowers the source phase onto with { type: "webassembly" } import attributes for the Bun target (pass-through for others); adds a new C++ FFI entry point Bun__createJSWebAssemblyModuleFromBytes that calls Wasm::Module::validateSync; threads an is_source_phase_import flag through the module loader; refactors the file-loader auto-watch block into a shared helper; and adds a 455-line spawn-based test suite. The bundler/CSS/HTML changes are mechanical phase: Default::default() field additions.
Security risks
The new FFI surface compiles arbitrary bytes as WebAssembly via JSC's Wasm::Module::validateSync — the same engine path as new WebAssembly.Module(bytes), gated on a wasm-magic check and with proper throw-scope handling, so the attack surface is equivalent to existing user-reachable APIs. The type: "webassembly" attribute forces the wasm loader regardless of extension, which is consistent with how other type: loader overrides already work. I don't see novel security exposure, but a human should confirm the FFI lifetime/exception-scope handling in ModuleLoader.cpp.
Level of scrutiny
High. This is a new language feature touching the parser (contextual keyword + snapshot/restore lookahead), the printer (target-conditional lowering that JSC must round-trip), the module-map keying strategy (riding on ScriptFetchParameters::Type::WebAssembly), and runtime FFI. Several design decisions warrant maintainer sign-off: the attribute-based lowering approach, rejecting a second argument to import.source(), the same-file source/evaluation phase-conflict parse error, and rejecting rather than supporting source phase in bun build.
Other factors
CodeRabbit's two findings (watcher registration for source-phase wasm, test assertion ordering) were addressed in c6b96c8 and confirmed resolved. The PR description is thorough and the test suite covers identity, errors, contextual-keyword edge cases, transpiler output, and bundler rejection. However, robobun reports build-rust failures across 5 platforms for commit 90ac099 (the latest non-autofix commit), which need to be green. The bug-hunting system found no correctness issues in the diff.
|
Note on the "build-rust failures on 90ac099" mentioned above: those statuses all read Canceled, not failed — build 59703 was auto-cancelled when autofix-ci pushed the formatting commit (956b05f) right after. The build on the current head has no failures (66 checks passed so far, remainder still running). |
|
CI status for maintainers: the diff is green — every build lane and test lane passes except one flaky, unrelated failure.
One retrigger has already been used; not pushing more. Ready for review/merge pending a maintainer's judgment on the flaky lane. |
|
Updated CI status for maintainers (build 59776, head This is the second build where the darwin test queue starved (also 59706); on the one build where those lanes did run (59739) they passed. The previous build's The allowed retrigger has been used. The PR is review-complete — all bot findings addressed and threads resolved — and just needs a maintainer to retry the expired darwin jobs (or merge over them, at your discretion). |
|
CI update (build 59776, final): a retried
The other two darwin lanes remain "Expired" (never got agents). This PR touches no SQL, WebSocket, or Docker code. Summary: the diff is green on every lane and every test that has ever executed, across Linux/Windows/musl/ASAN/alpine and now the darwin suite itself. The remaining red is mac-agent infrastructure. Ready for maintainer retry/merge. |
robobun
force-pushed
the
farm/60b4175a/import-source-phase
branch
from
a56bf7b to
8684632
Compare
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/ast/lib.rs`:
- Around line 1409-1458: In range_of_identifier(), the private-name branch
currently advances past '#' before setting r.len, causing malformed names like
"`#1`" to yield an empty span; set r.len = 1 (seed the range to cover the '#')
before calling iter.next(&mut cursor) so any invalid private-name keeps the '#'
highlighted, and add a regression test alongside the existing private-name tests
to cover a lone-`#` followed by invalid characters (e.g., "`#1`") to ensure the
span is non-empty.
🪄 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: 8d628f6c-d9da-467f-ad4d-775b60c5b535
📒 Files selected for processing (10)
src/ast/e.rssrc/ast/expr.rssrc/ast/import_record.rssrc/ast/lib.rssrc/ast/s.rssrc/bundler/AstBuilder.rssrc/bundler/HTMLScanner.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/css/css_parser.rs
💤 Files with no reviewable changes (5)
- src/bundler/AstBuilder.rs
- src/bundler/HTMLScanner.rs
- src/bundler/linker_context/prepareCssAstsForChunk.rs
- src/bundler/linker_context/postProcessJSChunk.rs
- src/css/css_parser.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/ast/lib.rs`:
- Around line 1409-1458: In range_of_identifier(), the private-name branch
currently advances past '#' before setting r.len, causing malformed names like
"`#1`" to yield an empty span; set r.len = 1 (seed the range to cover the '#')
before calling iter.next(&mut cursor) so any invalid private-name keeps the '#'
highlighted, and add a regression test alongside the existing private-name tests
to cover a lone-`#` followed by invalid characters (e.g., "`#1`") to ensure the
span is non-empty.
🪄 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: 8d628f6c-d9da-467f-ad4d-775b60c5b535
📒 Files selected for processing (10)
src/ast/e.rssrc/ast/expr.rssrc/ast/import_record.rssrc/ast/lib.rssrc/ast/s.rssrc/bundler/AstBuilder.rssrc/bundler/HTMLScanner.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/css/css_parser.rs
💤 Files with no reviewable changes (5)
- src/bundler/AstBuilder.rs
- src/bundler/HTMLScanner.rs
- src/bundler/linker_context/prepareCssAstsForChunk.rs
- src/bundler/linker_context/postProcessJSChunk.rs
- src/css/css_parser.rs
🛑 Comments failed to post (1)
src/ast/lib.rs (1)
1409-1458:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep malformed private names anchored to
#.If
range_of_identifier()is called on#1, the private-name branch consumes#, sees1, and then falls through withr.len == 0. That yields an empty diagnostic span for invalid private-name syntax, even though the lone-#case already returns length 1. Seed the range with#before advancing so malformed private names still highlight the token start, and add a regression case alongside the existing private-name tests.Suggested fix
// Handle private names if cursor.c == '#' as i32 { + r.len = 1; if !iter.next(&mut cursor) { - r.len = 1; return r; } }#[test] fn private_names() { assert_eq!(len_of(b"#"), 1); + assert_eq!(len_of(b"`#1`"), 1); assert_eq!(len_of(b"`#foo` = 1"), 4); assert_eq!(len_of(b"`#foo`"), 4); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Handle private names if cursor.c == '#' as i32 { r.len = 1; if !iter.next(&mut cursor) { return r; } } // Skip over bracketed unicode escapes such as "\u{10000}". The cursor is // positioned on a `\`; advance `cursor.i` one past the closing `}` and // zero `cursor.width` so the next decode starts exactly there (the manual // advance already consumed the backslash's width — leaving `width` set // would skip the byte after `}`). let skip_bracketed_escape = |cursor: &mut bun_core::strings::Cursor| { if cursor.i + 2 < end && text[cursor.i as usize + 1] == b'u' && text[cursor.i as usize + 2] == b'{' { cursor.i += 2; while cursor.i < end { if text[cursor.i as usize] == b'}' { cursor.i += 1; break; } cursor.i += 1; } cursor.width = 0; } }; if bun_core::identifier::is_identifier_start(cursor.c) || cursor.c == '\\' as i32 { // An identifier may *start* with an escape (e.g. `\u{61}bc`); skip it // here too, or the loop below reads the `u`/`{` as ordinary codepoints // and truncates the range at the `{`. if cursor.c == '\\' as i32 { skip_bracketed_escape(&mut cursor); } while iter.next(&mut cursor) { if cursor.c == '\\' as i32 { skip_bracketed_escape(&mut cursor); } else if !lexer_tables::is_identifier_continue(cursor.c) { r.len = i32::try_from(cursor.i).expect("int cast"); return r; } } // EOF inside the identifier: the cursor holds the *start* offset and // width of the last codepoint read — include that codepoint. r.len = i32::try_from(cursor.i + cursor.width as u32).expect("int cast"); }#[test] fn private_names() { assert_eq!(len_of(b"#"), 1); assert_eq!(len_of(b"`#1`"), 1); assert_eq!(len_of(b"`#foo` = 1"), 4); assert_eq!(len_of(b"`#foo`"), 4); }🤖 Prompt for 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. In `@src/ast/lib.rs` around lines 1409 - 1458, In range_of_identifier(), the private-name branch currently advances past '#' before setting r.len, causing malformed names like "`#1`" to yield an empty span; set r.len = 1 (seed the range to cover the '#') before calling iter.next(&mut cursor) so any invalid private-name keeps the '#' highlighted, and add a regression test alongside the existing private-name tests to cover a lone-`#` followed by invalid characters (e.g., "`#1`") to ensure the span is non-empty.
|
Regarding the CodeRabbit finding on
If tightening that span is wanted, it belongs in a separate change that updates both Rust copies together, not a rider on this PR. |
|
CI result (build 63586, head 39c996b): 72 checks pass; the only error-level failure is |
robobun
force-pushed
the
farm/60b4175a/import-source-phase
branch
from
cf07ef1 to
855db6d
Compare
robobun
force-pushed
the
farm/60b4175a/import-source-phase
branch
from
73ba7be to
3014f3f
Compare
There was a problem hiding this comment.
Both remaining findings (macro-namespace records in the phase-conflict scan, and the watcher fd close/open gate mismatch) are addressed in 673a2fc — nothing further from me. This is a substantial language-feature implementation spanning the parser, printer, module loader, and JSC C++ bindings, so it warrants a maintainer look.
Extended reasoning...
Overview
This PR implements TC39 Source Phase Imports (Stage 3) — both static import source x from "..." and dynamic import.source(...) — for the Bun runtime. It touches 26 files across the AST (new ImportPhase enum replacing the PHASE_DEFER flag bit, threaded through S::Import, E::Import, ImportRecord), the JS parser (contextual-keyword disambiguation with one-token lookahead, phase-conflict detection, macro-import interaction guards), the printer (lowering onto with { type: "webassembly" } for the Bun target, pass-through for others), the module loader (fetchESMSourceCode in C++ rejecting builtins/virtual modules at source phase, a new Bun__createJSWebAssemblyModuleFromBytes FFI entry point calling Wasm::Module::validateSync), and the runtime transpile hooks (is_source_phase_import plumbing, factored-out maybe_auto_watch_file). 54 new tests plus 3 in import-defer.test.ts.
Security risks
Low. The new surface is parsing/lowering of an import form and a wasm-compilation FFI helper that wraps the same Wasm::Module::validateSync path as new WebAssembly.Module(bytes). No auth/crypto/permissions code is touched. The C++ helper copies bytes into a WTF::Vector before validation (no aliasing of caller memory), checks Wasm::isSupported(), and propagates OOM/CompileError via the throw scope. The module-loader changes add rejection paths (errors instead of wrong values) rather than new capability.
Level of scrutiny
High. This is a new language feature with non-trivial grammar disambiguation (import source from from "x"), a lowering strategy that piggybacks on JSC's ScriptFetchParameters::Type::WebAssembly to key a separate module-map entry, and several explicitly-flagged design decisions left to maintainers (whether mock.module should override phase semantics; whether unflagged eval-phase wasm imports should change; whether this closes #12434). It also intersects with #31244 (dynamic import.defer()) which the description notes will need a small rebase. The C++ binding adds a new [[ZIG_EXPORT]] entry point. None of this is mechanical.
Other factors
Over eight rounds of bot review every finding was addressed promptly with tests, including the two most recent (excluding macro-namespace records from check_source_phase_conflict, and gating the watcher fd close on REQUIRES_FILE_DESCRIPTORS to match the open) which are fixed in head commit 673a2fc with a new order-independent test.each covering the macro+source-phase case. The bug-hunting system found nothing new on this revision. CI on prior heads was green except for unrelated infrastructure flakes (darwin queue starvation, serve-body-leak ASAN threshold, fetch-leak RSS sample). Test coverage is thorough (54 spawn-based cases). Given the scope and the open maintainer-call design questions, deferring to human review rather than approving.
`import source mod from "./a.wasm"` and `import.source("./a.wasm")`
evaluate to the compiled `WebAssembly.Module` of a WebAssembly file
without instantiating it.
JSC has no source phase support (its parser rejects both forms and
`ModulePhase` is {Evaluation, Defer} only), so Bun's transpiler lowers
the syntax onto the existing import-attributes channel: the printed
output requests the module with `with { type: "webassembly" }`, which
JSC maps to `ScriptFetchParameters::Type::WebAssembly` — a distinct
module-map key — and Bun's module loader answers that request with a
synthetic module whose default export is the `WebAssembly.Module`
(compiled via `Wasm::Module::validateSync`). Non-Bun transpile targets
print the real syntax verbatim.
The `phase_defer: bool` plumbing from the static `import defer`
implementation is generalized into an `ImportPhase { Evaluation,
Defer, Source }` enum on `S::Import`, `E::Import`, and `ImportRecord`
(the record flag bit it replaces was the last free u16 bit).
Not supported (clear errors instead of wrong values): `bun build`,
a second argument to `import.source()`, and importing one specifier
at both source and evaluation phase from the same file (JSC dedups a
module's requested modules by (specifier, phase), ignoring
attributes, so one of the two bindings would silently get the other
phase's value).
…e in tests The module-source path returned before the file-loader's auto-watch registration, so `--watch`/`--hot` never saw changes to a wasm file imported with `import source`. Factor the file loader's inline auto-watch block into `maybe_auto_watch_file` and call it from both paths.
The source/evaluation phase conflict check only ran for import
declarations, so `import source mod from "./a.wasm"` followed by
`export { default } from "./a.wasm"` slipped past it (the reverse
order was caught), leaving the guard order-dependent. Factor the scan
into check_source_phase_conflict and run it for `export ... from` and
`export * from` records too.
The dead-code-binding test asserted output that would be identical if the scan_imports guard were removed (the printer downgrades a binding-less source import to a bare evaluation-phase import, which the file loader accepts silently). Use a non-wasm fixture so losing the phase becomes observable: with the guard, the module source is still requested and loading fails; without it, the file loader succeeds. Verified the test fails with the guard removed. Keep a second case asserting valid wasm still loads with an unused binding.
The scan_imports guard only protects bindings whose references sit in dead code; a source phase import with zero syntactic references in a TypeScript file is still elided wholesale by the is_unused_in_typescript check — the same elision tsc/esbuild apply to plain imports and the same behavior as import defer, with verbatimModuleSyntax as the escape hatch. Make that scope explicit in the comment and pin both sides in tests: JavaScript preserves the phase (invalid wasm fails to load), TypeScript elides it (never fetched).
import.source() is part of the ImportCall production and is valid with the Script goal, like plain import(). Only import.meta implies the Module goal. Setting esm_import_keyword unconditionally on the dot branch made a CommonJS file using only import.source() lose its top-level this binding (rewritten as if the file were an ES module).
`import source x from "bun"` (and "node:fs", "bun:sqlite", etc.)
silently bound the builtin instead of erroring:
- The printer's Tag::Bun fast path rewrote literal-"bun" imports to
globalThis.Bun before the source-phase lowering ran, so the module
loader was never consulted. Both short-circuits (print_require_or_import_expr
and the SImport arm) are now gated on phase != Source.
- For every other builtin and for non-literal specifiers, the lowered
`with { type: "webassembly" }` reached fetchESMSourceCode, which
called Bun__fetchBuiltinModule without the type attribute and served
the evaluation-phase builtin. fetchESMSourceCode now rejects a
successfully-found builtin with the same "only WebAssembly modules
have a module source" TypeError the file-backed non-wasm path emits.
6 new test.each cases cover static+dynamic, literal+non-literal, and
three builtin namespaces.
Both runVirtualModule paths in fetchESMSourceCode (the isBunTest branch before the builtin check and the !isBunTest branch after) returned through handleVirtualModuleResult without consulting the type attribute, so a mock.module() or build.module() registration matching a source-phase specifier silently bound the mock's default export. The virtual-module loader whitelist (js/jsx/ts/tsx/json/toml/yaml/md/ object) cannot produce wasm bytes, so a virtual module can never satisfy a source-phase request. Hoist the isSourcePhase computation above both virtual-module lookups and reject a match with the same error the builtin and file-backed non-wasm paths produce; evaluation- phase import of the same specifier keeps working.
process_import_statement's is_macro fast path (with { type: "macro" }
or the macro: prefix) ran before stmt.phase was ever consulted,
registering the binding as a macro ref and silently dropping the
phase: `import source mod from "./m" with {type:"macro"}; mod()`
executed the macro. No macro can produce a module source, and there
is no evaluation to defer for a compile-time binding, so reject both
combinations with a clear error before the macro path fires.
Also pin the static `import source x from "./d.json" with {type:
"json"}` case: the source-phase lowering overrides the user's type
attribute with "webassembly", so the module loader still errors on
non-wasm content as expected.
The phase+macro guard covered with {type:"macro"} and the macro:
prefix, but bunfig.toml [macros] / package.json bun.macros per-
specifier remapping is a third trigger computed later in
process_import_statement (macro_remap via macro_context.get_remap).
A [macros]."pkg".default entry would register the binding as a
macro ref and silently drop the source phase.
Hoist the macro_remap computation above the guard and extend it to
(is_macro || macro_remap.is_some()). import defer is structurally
unaffected (macro_remap is only consumed for default_name and items,
which defer has neither of), but the unified guard keeps it covered.
macro_remap.is_some() fired on the mere presence of any [macros]
entry for the specifier, so import defer * as ns from "pkg" with
a [macros].pkg entry keyed only on named exports (never applied to
star bindings) was rejected. Likewise import source mod from "pkg"
with a remap lacking a "default" key was rejected when it would
have proceeded correctly.
The bunfig remap only contributes to the guard when it would
actually be consumed for that phase's binding shape: Source checks
remap.get(b"default").is_some(); Defer never consults macro_remap
(star bindings are not remapped). is_macro (explicit with
{type:"macro"} or macro: prefix) still rejects both phases.
…atcher fd close to open
check_source_phase_conflict matched macro-import records (kind: Stmt,
default phase: Evaluation, same path.text) even though their
statements become S::Empty and the records are IS_UNUSED /
Macro::NAMESPACE and never reach JSC. A macro-first
`import {x} from "./y" with {type:"macro"}; import source m from "./y"`
therefore raised a false "both phase" error while the reverse order
did not. The predicate now skips Macro::NAMESPACE records; both
orders proceed to the load-time "only WebAssembly modules" error.
maybe_auto_watch_file opened an fd when REQUIRES_FILE_DESCRIPTORS
(macOS and FreeBSD) but the close-on-add_file-failure was gated on
cfg(target_os = "macos") only, leaking the fd on FreeBSD. The
cleanup now gates on the same REQUIRES_FILE_DESCRIPTORS constant so
open and close cannot drift apart. The leak predated this PR; the
factored helper and its new source-phase caller made it visible.
robobun
force-pushed
the
farm/60b4175a/import-source-phase
branch
from
673a2fc to
2289020
Compare
check_source_phase_conflict ran before the bunfig [macros] remap
path had a chance to drop a fully-remapped statement to S::Empty,
so in source-first order `import source mod from "pkg";
import { foo } from "pkg"` (with [macros].pkg.foo remapped, no
default key) raised a false "both phase" error against a record
that would never be emitted. Reverse order worked because the
prior record's namespace was already Macro::NAMESPACE when
scanned. The check now runs after the remap_count > 0 early-return,
where the statement is known to survive emission; both orders now
reach the accurate load-time "only WebAssembly modules" error.
Genuine conflicts (non-remapped bindings of the same specifier)
are still caught at parse time.
1 participant
Implements the TC39 Source Phase Imports proposal — both the static and dynamic forms — for the Bun runtime:
This is the companion to static
import defer(#30975) for the other TC39 import phase, and the syntax Node ships for wasm (--experimental-wasm-modules' source phase is unflagged in Node ≥ 24).How it works
JSC has no source phase support — its parser rejects both forms,
AbstractModuleRecord::ModulePhaseis{Evaluation, Defer}only, and there is no module-source object. But Bun transpiles every module before JSC sees it, so the transpiler lowers the syntax onto machinery JSC already has:parse_stmt.rs,parse_import_export.rs):sourceis a contextual phase keyword only when followed by anImportedBindingthat is followed byfrom.import source from "x"stays a default import namedsource;import source from from "x"is a source phase import bindingfrom(one token oflexer.snapshot()lookahead).import.source(expr)is accepted afterimport.alongsidemeta. Escaped spellings (sourc\u0065) are rejected via raw-token comparison, same asdefer.phase_defer: boolplumbing from Implement staticimport defer(TC39 Stage 3) #30975 is generalized intoImportPhase { Evaluation, Defer, Source }onS::Import,E::Import, andImportRecord(the flag bit it replaces was the last freeu16bit).import mod from "./a.wasm" with { type: "webassembly" }and the dynamic form toimport(expr, {with:{type:"webassembly"}}).then((m)=>m.default). JSC parsestype: "webassembly"toScriptFetchParameters::Type::WebAssembly, which is part of the module-map key — so the source phase gets its own module entry, distinct from the evaluation-phase (file-loader) entry. The ModuleInfo fast path records the same request via the pre-existingFetchParameters::Webassembly, so no cache-format change. Non-Bun targets print the real syntax verbatim (pass-through, likeimport defer).jsc_hooks.rs+ newBun__createJSWebAssemblyModuleFromBytesinModuleLoader.cpp): a fetch carrying thewebassemblytype attribute forces the wasm loader and produces a synthetic module whose default export is theWebAssembly.Module, compiled synchronously withWasm::Module::validateSync— the engine path behindnew WebAssembly.Module(bytes). Works for files,blob:URLs, andrequire(esm)graphs.Evaluation-phase wasm imports are untouched:
import p from "./a.wasm"still resolves to the file path string (test/regression/issue/16476), andbun run foo.wasmstill uses the WASI runner.Errors instead of wrong values
import source x from "./mod.js"(no source representation):Source phase import of "…/mod.js" failed: only WebAssembly modules have a module source. Corrupt wasm (valid magic) rejects withWebAssembly.CompileError.bun build:"import source" is not supported when bundling— the bundler treats wasm as a copied asset, so inlining the record would bind the asset path string.import.source(specifier, options): rejected; the lowering injects its own import attributes and cannot merge an arbitrary options expression at compile time.(specifier, phase)and ignores attributes (pre-existing: two imports of one specifier with differenttypeattributes already merge silently), so one of the two bindings would get the other phase's value. Cross-file and static-source + dynamic-import()combinations work and are tested.Known limitations
import deferdoes (module lowering predates phases).node:vm/evalsource text goes straight to JSC's parser, which has no source phase (same as dynamicimport.defer).WebAssembly.Module.prototypedoes not inherit%AbstractModuleSource%(no such intrinsic in JSC).Note: #31244 (dynamic
import.defer()) touches the sameE::Importplumbing with aphase_defer: bool; whichever lands second has a small rebase — this PR'sImportPhaseenum subsumes that bool.Verification
test/js/bun/resolve/import-source-phase.test.ts— 58 spawn-based cases (32 in the original revision, 26 added during review) covering: Module identity (static/dynamic/cross-file), instantiation, eval-phase coexistence, TypeScript,blob:URLs, unused-binding preservation,source-as-identifier (includingimport source from from "x"), CommonJS/Script-goalimport.source()(no forced ESM or strict mode, matching plainimport()), source-phase imports of builtin modules (bun,node:fs,bun:sqlite; static+dynamic, literal+non-literal) and of virtual modules (mock.module()underbun test,Bun.pluginbuild.module()) rejecting with the same error as non-wasm files while evaluation-phase import keeps working, phase+macro-import combinations (with { type: "macro" },macro:prefix, and bunfig[macros]remapping, for bothsourceanddefer) rejected at parse time, all error paths above, transpiler round-trips for both targets, and the bundler rejections. Fails 21/32 on bun without this change; 32/32 pass with it.import-defer.test.ts(20),bundler/transpiler(188),esbuild/loader,esbuild/importstar,bundler_loader,transpiler-cache, wasi, and the fulltest/js/bun/resolve/suite still pass (the twoload-same-js-file-a-lottimeouts reproduce identically without this diff — slow-container perf test).Related issues
import x from "./file.wasm"to give a module instead of a path. This PR adds the spec-blessed way to get the compiledWebAssembly.Module(import source/import.source()); evaluation-phase wasm imports are intentionally unchanged, so whether that closes the issue or full wasm-ESM integration should stay open is a maintainer call.bun build#22026 (eval-phase wasm behavior differing betweenbun runandbun buildoutput) — source phase imports reject in the bundler rather than changing that behavior.Rebase resolution
Rebased onto main over 90 commits (head at 855db6d). Two non-trivial adaptations to type changes landed in the interim:
S::Import.star_name_locchanged fromOption<Loc>toLocwithLoc::EMPTYas the sentinel (part of ast: shrink Expr/Stmt/Binding 24→16 bytes #32507). Every phase-aware site was updated to use.is_empty()/Loc::EMPTYaccordingly, including the phase-shapedebug_assert!inp.rs, the REPL lowering branch inrepl_transforms.rs, and the three synthetic-import constructors.LocRef.ref_changed fromOption<Ref>toRefdirectly. Theimport sourcedefault-name constructor inparse_stmt.rsand the REPL direct-binding extractor inrepl_transforms.rsdrop the now-redundantSome(..)/.expect(..).bun_jsc::cppbecame codegen-driven (src/codegen/cppbind.ts). The[[ZIG_EXPORT(zero_is_throw)]]annotation already onBun__createJSWebAssemblyModuleFromBytesinModuleLoader.cppis picked up automatically, so the hand-written extern was dropped in favor of the generatedbun_jsc::cpp::Bun__createJSWebAssemblyModuleFromByteswrapper (returnsJsResult<JSValue>); the Rust call site injsc_hooks.rsis unchanged.import-source-phase.test.ts(41/41),import-defer.test.ts,import-meta.test.js,esModule.test.ts, andbundler/transpiler/transpiler.test.js(188/188) all pass on the rebased debug build;cargo clippy -p bun_js_parser -p bun_ast -p bun_js_printeris clean.