runtime: attach ModuleInfo to ESM transpiles so TypeScript type-only re-exports resolve - #35605
runtime: attach ModuleInfo to ESM transpiles so TypeScript type-only re-exports resolve#35605robobun wants to merge 19 commits into
Conversation
|
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:
WalkthroughChangesRuntime ESM transpilation now controls Runtime ESM ModuleInfo
WebKit preview update
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
No bugs found, but this flips a runtime default that changes how every ESM module is analyzed by JSC (ModuleInfo attached on all transpiles instead of only under --isolate), so it warrants a human look before landing.
Checked: the four gate sites (sync/async × fresh/cache-hit) are updated consistently; ZigSourceProvider.cpp already keys on module_info != nullptr and frees it in the destructor; BunAnalyzeTranspiledModule.cpp frees the record after use when the isolation cache is inactive, so no per-module leak. Cache version bump to 24 correctly invalidates pre-existing entries with esm_record_byte_length == 0. Test changes un-skip the previously-TODO'd run-mode cases and add sync/async coverage plus an opt-out check.
Extended reasoning...
Overview
Replaces the use_isolation_source_provider_cache() gate with a new always-on use_module_info_for_esm() (opt-out via BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO) at four sites in RuntimeTranspilerStore.rs and jsc_hooks.rs, adds the flag to env_var.rs, adds the helper to VirtualMachine.rs, bumps RuntimeTranspilerCache version to 24, and un-skips/extends type-export.test.ts.
Security risks
None. No untrusted input parsing, no auth/crypto/permissions. The change routes existing printer output through an existing JSC consumer.
Level of scrutiny
High. Module loading is on the hot path of every bun run/bun test invocation, and this flips a default that #15758 deliberately left gated behind --isolate (the removed TODO comment explicitly deferred enabling it in the runtime transpiler). The infrastructure exists and is exercised under --isolate and --compile, and the PR description reports passing adjacent test suites, but enabling it unconditionally is a product-level decision with a large blast radius (any divergence between Bun's printer-emitted ModuleInfo and JSC's own analyzer now surfaces for all users, not just --isolate users).
Other factors
- The code change itself is small and mechanically correct; the inlined predicate in
RuntimeTranspilerStore.rsmatchesVirtualMachine::use_module_info_for_esmexactly. - Verified the C++ side (
ZigSourceProvider.cpp:178-181,BunAnalyzeTranspiledModule.cpp:188-194) already handlesmodule_info != nullptroutside isolation mode and frees it, so no leak is introduced. - The cache-version bump is necessary and correctly justified.
- Test additions follow harness conventions (
bunEnvspread, concurrent pipe drain, exit-code asserted last, debug-only timeout branch). - CI (#80362) is still building at time of review; a human should confirm it's green given the breadth of the behavior change.
Deferring because flipping a previously-gated runtime default across all module loads is exactly the kind of decision a maintainer should sign off on, even when the diff is small.
|
Heads-up from a parallel attempt (#35689, closed as duplicate): the actual #8439 reproduction still fails with this fix. The issue's repro is: // index.ts
import { TestInterface } from "./interface";
class TestClass {
@Decorator()
test?: TestInterface;
}
// interface.ts
export interface TestInterface {}With The existing |
ad3a50d to
029a1a6
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/third_party/astro/fixtures/.astro/actions.d.ts:1-5— These five files undertest/js/third_party/astro/fixtures/.astro/are Astro's auto-generated build artifacts and appear to have been committed by accident — they're unrelated to the ModuleInfo change, andactions.d.tseven hardcodes the machine-specific absolute path/workspace/bun/.... Sinceastro-post.test.js'sbeforeAllcallsbuild({ root: fixtureDir })and regenerates.astro/on every run, these files will show as dirty in git whenever the test runs on any other checkout; suggest removing them from the PR and adding.astro/to a local.gitignoreinstead.Extended reasoning...
What the issue is
Commit ad3a50d in this PR adds five new files under
test/js/third_party/astro/fixtures/.astro/:actions.d.tscontent.d.tstypes.d.tscontent-assets.mjscontent-modules.mjs
These are Astro's standard auto-generated type/module manifest files (
.astro/is Astro's equivalent of.next/ordist/), not hand-written test fixtures. They have nothing to do with theModuleInforuntime change — the PR otherwise touches onlysrc/bun_core/env_var.rs,src/jsc/*.rs,src/runtime/jsc_hooks.rs, andtest/js/bun/typescript/type-export.test.ts. They appear to have been committed accidentally after running the astro test locally.Why it's a problem
Machine-specific absolute path.
actions.d.tsline 2 contains:type Actions = typeof import("/workspace/bun/test/js/third_party/astro/fixtures/src/actions")["server"];
The
/workspace/bun/...path only exists on the author's machine.Regenerated on every test run.
test/js/third_party/astro/astro-post.test.jsline 29 does:beforeAll(async () => { await build({ root: fixtureDir, ... });
Astro's
build()regenerates the entire.astro/directory each time, embedding the current checkout's absolute path intoactions.d.ts.Not gitignored.
git check-ignore test/js/third_party/astro/fixtures/.astro/actions.d.tsreturns nothing, and there is no.gitignoreintest/js/third_party/astro/or itsfixtures/subdirectory.Step-by-step: how the churn manifests
- This PR merges with
.astro/actions.d.tscontainingimport("/workspace/bun/..."). - A developer at
~/dev/bun(or CI at/home/runner/work/bun/bun) runsbun bd test test/js/third_party/astro/astro-post.test.js. beforeAllcalls Astro'sbuild({ root: fixtureDir }), which rewrites.astro/actions.d.tswithimport("/home/runner/work/bun/bun/test/js/...")(their absolute path).git statusnow showstest/js/third_party/astro/fixtures/.astro/actions.d.tsas modified.- Every subsequent developer/CI runner with a different checkout path sees the same spurious diff, forever.
The other four files (
content.d.ts,types.d.ts,content-*.mjs) may or may not diff depending on the pinned Astro version, but they're equally generated and equally not source.Impact
Nothing breaks at runtime — the test regenerates and overwrites these files before using them, so the committed contents are never actually read. This is why the severity is nit, not normal. But committing machine-specific generated output creates permanent working-tree noise and risks accidentally landing in unrelated future PRs from anyone who ran the astro test.
Fix
git rm -r test/js/third_party/astro/fixtures/.astro echo '.astro/' >> test/js/third_party/astro/fixtures/.gitignore
(and commit the
.gitignore).
|
Status (66dc26b = 2886915 plus a merge of main that touches only CI config and an unrelated process fix): ready to merge once #37352 (the bare WebKit bump) is in, or directly since this carries the same pin. Re-verified on 2886915: inspector, type-export, plugins, isolation and import-defer suites 177/177, the terminate-while-importing test passes, and the LSAN repro is clean at 1/4/8 workers (the evaluate-time free does not collide with the holder / ~SourceProvider / Rust-drop paths).
|
|
One more finding from reviewing the closed duplicate: this change drops link-time detection for typo'd
// values.ts
export const realValue = 42;
// reexport.ts
export { typoedName } from './values';
// main.ts
import './reexport';
console.log('OK');
A consumer that actually Probably worth either a test pinning the new behavior (and a sentence in the PR body), or a per-entry |
|
@robobun get this mergable |
…resolve
export { SomeType } from './mod' in a .ts file produced
SyntaxError: export 'SomeType' not found in './mod'
at runtime because the re-export survived transpilation while the type
declaration in the target module was erased, and JSC's own module analyzer
has no way to know the missing binding was a TypeScript type.
record that encodes every import/export and marks the potentially-elided
bindings as ImportEntryType::SingleTypeScript plus m_isTypeScript on the
JSModuleRecord, which JSC already honours at link time. That record was
only attached under bun test --isolate (for the isolation source-provider
cache) and for --compile standalone bytecode.
Attach it on every runtime ESM transpile of a js/ts file (both the sync
jsc_hooks path and the async RuntimeTranspilerStore path), gated by a new
BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO escape hatch. Bump the on-disk
transpiler cache version so pre-existing entries without an esm_record are
invalidated.
test/js/bun/typescript/type-export.test.ts: un-skip the 18 'run' mode tests
that #15758 left behind a TODO for, add the #7384 repro covering both
transpile paths, and fix two pre-existing debug-build issues (compile tests
timing out behind the 4-slot semaphore, and ~18 x ~1 GB standalone binaries
leaking into the temp dir per run).
Fixes #7384
|
@robobun pushed four commits on top of 61db16a, please keep them when you rebase:
Dropped WebKit side: oven-sh/WebKit#405 is #345 rebased plus the |
|
Thanks, keeping all four. Heads up on overlap: build 91634 (the 61db16a push) flagged an ASAN leak in |
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 `@scripts/build/deps/webkit.ts`:
- Line 6: Update the WEBKIT_VERSION constant from the temporary
autobuild-preview-pr-345-b7c69395 pin to the immutable autobuild-<sha>
identifier for the merged WebKit commit. Do not add or modify any separate
process.versions.webkit assertion.
🪄 Autofix
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: b5f1c1d1-7dce-4f39-850e-8c0f055f5b21
📒 Files selected for processing (12)
scripts/build/deps/webkit.tssrc/bun_core/env_var.rssrc/bundler/analyze_transpiled_module.rssrc/js_printer/lib.rssrc/jsc/ResolvedSource.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunAnalyzeTranspiledModule.cppsrc/jsc/bindings/ModuleLoader.cppsrc/jsc/bindings/ZigSourceProvider.cppsrc/runtime/jsc_hooks.rstest/js/bun/plugin/plugins.test.tstest/js/bun/typescript/type-export.test.ts
RuntimeTranspilerStore::run_from_js_thread pops a whole batch of finished jobs off the queue, then returns as soon as draining microtasks between jobs reports termination. The jobs it had not reached yet were already off the queue, so release_queued_jobs_for_teardown never saw them and their transpiled source, log, promise and ModuleInfo were never dropped. Release them in place instead. Bun__analyzeTranspiledModule / fallbackParse also returned the null promise that rejectWithCaughtException yields under termination; JSModuleLoader's BunTranspiledModule arm downcasts the returned value before its caller checks the throw scope, which UBSan reports as a member call on a null JSCell. Return the still-pending promise with the exception left in place, and fulfill() the record like JSC's own makeModule does instead of resolve(). Regression test: terminate a worker while it has dynamic imports in flight, with LeakSanitizer enabled in the child. On the previous commit it reports ~12 KB of leaked ModuleInfo per run plus the UBSan error under the debug cross-check; with this it exits 0 with empty stderr.
|
oven-sh/WebKit#405 is merged as |
|
Rebuilt locally against 723cea6c: inspector, type-export and plugins suites all pass (136/136) and |
JSC keeps export entries in insertion order and std::sort()s them every time it builds a module namespace object. Its own ModuleAnalyzer inserts local exports in hash-table order; the printer-built ModuleInfo inserted them in source order, and on modules like the 10k `export const name<i>` fixture in require-cache.test.ts that order makes introsort fall back to heapsort, so import() of such a module got slower than letting JSC parse it (release-asan: ~45 ms per import on a 10k-export module, __adjust_heap at 13% of samples). Move local export records after everything else and sort them by export name in ModuleInfo::finalize, which is the sort's best case; the other records keep their relative order. With this the Bun-built record is faster than JSC's own analysis on that fixture. The transpiler cache test decodes the stored record and pins the order.
| /// JSC keeps export entries in insertion order and `std::sort`s them when | ||
| /// it builds the namespace object. Its own analyzer inserts local exports | ||
| /// in hash-table order; the printer sees them in source order, and on | ||
| /// inputs like `a0, a1, ..., a9999` introsort falls back to heapsort, | ||
| /// making `import()` of a wide module measurably slower than JSC's own | ||
| /// analysis. Hand JSC the sort's best case instead: local exports after | ||
| /// every other record, already in byte order of their export name. The | ||
| /// remaining records keep their relative order, so which unresolvable | ||
| /// indirect export gets reported first is unchanged. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Problem
bun buildandbun build --compilerun this fine;bun runandbun testdid not. The re-export inutils.tssurvives transpilation (Bun cannot knowValueOfis a type without looking at the other file), theexport typeinEventTypes.tsis erased, and JSC's own module analyzer then rejects the indirect export because the binding is gone.Cause
#15758 added the infrastructure to fix this: the printer emits a
ModuleInforecord alongside the transpiled source that encodes every import/export, marks the file as TypeScript, and flags re-exported single imports asImportEntryType::SingleTypeScript. JSC's linker (CyclicModuleRecord.cpp,AbstractModuleRecord.cpp) already toleratesResolution::NotFoundfor those entries and for indirect exports whenm_isTypeScriptis set.SourceProvider::createswitches toSourceProviderSourceType::BunTranspiledModulewhenevermodule_infois non-null, soBun__analyzeTranspiledModulebuilds theJSModuleRecordfrom Bun's record instead of re-parsing.That record was only attached when
use_isolation_source_provider_cache()was true, which isbun test --isolate/--parallel, and on the--compilestandalone path (where it is read back from the embedded graph). Plainbun run/bun testalways gotmodule_info = nulland fell back to JSC's analyzer.Fix
Replace the
use_isolation_source_provider_cache()gate withuse_module_info_for_esm(), which is always true (opt-out viaBUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO), at the four sites that constructmodule_info:runtime/jsc_hooks.rssync transpile (fresh print + cache hit)jsc/RuntimeTranspilerStore.rsasync transpile (fresh print + cache hit)Skipped when the parser emitted errors (
log.has_errors()) so a duplicate-export or similar error still surfaces as JSC's own syntax error instead of a later link-time failure.BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFOis the escape hatch. It disables ModuleInfo everywhere (under--isolatethe provider is then a plainModule, which the isolation cache already handles), andRuntimeTranspilerCachehashes that same decision intofeatures_hashso entries written with and without it never mix.ZigSourceProvider.cppalready keys offmodule_info != nullptr.BunAnalyzeTranspiledModule.cppused to free the record after the first analyze outside--isolate; JSC analyzes the same fetched source twice on therequire(esm)sync-replay path (a CJS entry requiring an ESM graph with a shared dependency), so the record now stays on the provider until~SourceProvider, and an abandoned load (worker teardown mid-transpile, early return before a provider adopts it) frees it fromOwnedResolvedSource::drop/ResolvedSourceCodeHolder. The printer records aBun.pluginnamespaced specifier as the samens:pathstring it prints, and only interns exported local names into the record.Two more teardown paths surfaced once every module carried a record (CI build 91634, ASAN leak in
worker_threads.test.ts):RuntimeTranspilerStore::run_from_js_threadpops a whole batch of finished jobs and used to return as soon as the microtask drain between jobs reported termination, abandoning the jobs it had not reached (already off the queue, so teardown never released them; this leaked their source, log, promise and record). It now releases the rest of the batch in place. AndBun__analyzeTranspiledModule/fallbackParsereturned the null promiserejectWithCaughtExceptionyields under termination, which JSC'sBunTranspiledModulearm downcasts (UBSan: member call on nullJSCell); they now hand back the still-pending promise with the exception left in place andfulfill()the record the way JSC's ownmakeModuledoes.RuntimeTranspilerCacheEXPECTED_VERSIONis bumped to 26 so pre-existing cache entries (written withesm_record_byte_length == 0) are invalidated; a hit on one of those would fall back to JSC's analyzer and reinstate the error.Record order (CI build 91765)
With every ESM import going through the Bun-built record,
require-cache.test.ts"via import() with a lot of long export names" (300 imports of a module with 10kexport const name<i>) timed out on the x64-asan lane on all four attempts, and its concurrent sibling starved along with it; the file passes on the other PR builds on that lane. Profiling the release-asan build (gdb sampling, that 10k-export module, steady-stateimport()):JSModuleNamespaceObjectconstructorstd::__adjust_heap(introsort's heapsort fallback)makeModuleparse, orBun__analyzeTranspiledModule)JSC keeps export entries in insertion order and
std::sorts them whenever it builds a namespace object.ModuleAnalyzerinserts local exports by iterating the variable environment, so in hash order, which introsort handles well; the printer inserted them in source order, and onname0 .. name9999that input drives libstdc++'s introsort past its depth limit into heapsort, roughly doubling the comparisons (each of which scans a 100-character common prefix).ModuleInfo::finalizenow moves the local export records after all other records and sorts them by export name, the sort's best case. The other records keep their relative order, so which unresolvable indirect export gets reported first is unchanged, and nothing else observes the order (JSC sorts the namespace itself). The same module with its exports shuffled or pre-sorted in the source was already 20 ms per import faster than main's path, which is what pointed at the order. The serialized format is unchanged, so no further cache version bump: older entries are just unsorted.The debug build cannot show this; there the difference is the debug-only
fallbackParsecross-check plus WTF's StringView lifetime tracking inside the same sort.WebKit dependency (oven-sh/WebKit#405)
Every runtime ESM provider now reaches JSC as
SourceProviderSourceType::BunTranspiledModule. Three switches in JSC still only matchedModule(DebuggerParseData.cppgatherDebuggerParseDataForSource,CachedTypes.cppCachedSourceProvider::encode/decode,Completion.cppgetSourceType); the first is user-visible:bun --inspectcould not resolve a breakpoint in any user module. (A fourth site,InspectorDebuggerAgent::didParseSource, was already fixed by the upstream merge in oven-sh/WebKit#383.) This was latent before because onlybun test --isolateand--compileoutput used that provider type.oven-sh/WebKit#405 (merged as
723cea6c; #345 was the earlier version of the same change) adds the missingcase BunTranspiledModule:arms.WEBKIT_VERSIONis pinned to that merged commit; #37352 carries the bare bump so it can land first.test/cli/inspect/inspect-module-breakpoints.test.tspins the behaviour. Matrix verified locally on this branch:type-export.test.ts#7384 casesinspect-module-breakpoints.test.tssrc/, WebKit 447082absrc/, WebKit 447082abCould not resolve breakpoint,locations: [](CI build 91627 reproduced this on every lane)src/, WebKit 723cea6c#35754 carried this bump for the
--isolatecase only; this PR supersedes it.Behaviour change: missing-value re-exports in .ts files
m_isTypeScripttolerates an unresolved indirect export at link time for the whole file; the re-exporting file has no local signal for which names are types. Sonow links without error when consumed via
import * as b from "./barrel"(b.fooo === undefined, the key is absent fromObject.keys(b)) and when imported for side effects. A directimport { fooo } from "./barrel"still errors. This matches whatbun buildand--compilealready produced and what ts-node/tsx do; Node with--experimental-strip-typesstill throws. The test file pins this so the trade-off is explicit.Tests
test/js/bun/typescript/type-export.test.ts:run-mode cases Skip javascriptcore's first parse step for ES Modules #15758 left behind the "module_info is not yet enabled in the runtime transpiler" TODO.BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1) and async transpile paths, plus the same through the on-disk transpiler cache hit path (padded source +BUN_RUNTIME_TRANSPILER_CACHE_PATH), plus a check thatBUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO=1restores the old error.--compilecases queue behind a 4-slot semaphore and the tail tests exceeded the 5 s default under debug+ASAN (now given a debug-only timeout), and each compile leaked a ~1 GB standalone executable into the temp dir (now deleted after the run).test/js/web/workers/worker-terminate-lifetime.test.tsgains an ASAN-only case that terminates a worker while it has dynamic imports in flight, with LeakSanitizer enabled in the child: on the commit before the teardown fixes it reports about 12 KB of leaked records per run plus the UBSan error under the debug cross-check; now it exits 0 with empty stderr.type-export.test.tsalso covers therequire(esm)shared-dependency graph andplugins.test.tsthe namespaced virtual module.test/cli/run/transpiler-cache.test.tsdecodes the record stored in a cache entry and pins the order above (the decoding constants it already had for the out-of-range test moved to file scope); on the released binary plainbun runstores no record at all, and with the ordering change alone reverted the records come out in source order.Also adds
test/js/third_party/astro/fixtures/.gitignorefor.astro/so running that test locally stops dirtying the worktree.The
.d.mtsre-export repro from #36751 and theexport { ... } fromshape in #20632 are the same bug and now run. The direct-import decorator-metadata variants (#15006, #17902) are a different mechanism and are not changed here.12 of the 94
type-exporttests fail on the released binary (the 8export from/import then exportrun cases,check ownkeys/#8439run cases, and the new #7384 cases); all 94 pass on this branch, as does the new inspector test against the pinned WebKit. Also ranimport-meta,transpiler-cache,isolation,import-defer,resolve, andimport-attributestest files with no failures.Fixes #7384
Fixes #36751
no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/typescript/type-export.test.ts test/js/web/workers/worker-terminate-lifetime.test.ts