runtime transpiler: attach module_info to ESM sources, fixing TypeScript type-only re-exports - #35689
runtime transpiler: attach module_info to ESM sources, fixing TypeScript type-only re-exports#35689robobun wants to merge 1 commit into
Conversation
The printer already builds a ModuleInfo record (imports, exports, var
declarations, TypeScript-erased names) that lets JSC skip re-parsing the
transpiled output during the module analyze phase. It was only attached
under `bun test --isolate`, so `bun run` / `bun test` still had JSC
re-parse the stripped JavaScript and throw on any name that was erased
as a TypeScript type:
// EventTypes.ts
export type ValueOf<T> = T[keyof T];
// utils.ts
export { ValueOf } from './EventTypes';
// -> SyntaxError: export 'ValueOf' not found in './EventTypes'
With ModuleInfo attached, the source provider becomes `BunTranspiledModule`
and `Bun__analyzeTranspiledModule` builds the JSModuleRecord directly,
marking the module `m_isTypeScript` and the erased import
`SingleTypeScript`. CyclicModuleRecord/AbstractModuleRecord then tolerate
a NotFound resolution for those entries instead of throwing.
- Drop the `use_isolation_source_provider_cache` gate on both the async
(RuntimeTranspilerStore) and sync (jsc_hooks) load paths, for both the
fresh-transpile and on-disk-cache-hit branches.
- Add `BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO` as an escape hatch.
- Bump the RuntimeTranspilerCache version so old entries (which carry an
empty `esm_record`) are invalidated and users get the fix on cache hit.
- Un-skip the "run"-mode tests in type-export.test.ts (18 cases); they
were gated on this being enabled. Add the #7384 repro for both load
paths and a test for the disable flag.
Fixes #7384
Fixes #8439
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #35605, which is the same fix and was opened first. Closing this one. One finding from verifying related issues: the actual #8439 reproduction (import used only in decorator metadata, no re-export) still throws |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| dump_source_string(vm, specifier, entry.output_code.byte_slice()); | ||
| } | ||
|
|
||
| let module_info: *mut c_void = if use_isolation_source_provider_cache | ||
| let module_info: *mut c_void = if generate_module_info | ||
| && entry.metadata.module_type != CacheModuleType::Cjs | ||
| && !entry.esm_record.is_empty() | ||
| { |
There was a problem hiding this comment.
🟡 BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO shapes the cached esm_record payload but doesn't participate in hash_for_runtime_transpiler, so a v24 entry written with the flag ON (empty esm_record) is served as a hit after the flag is unset — sending the module back down the JSC re-parse path and reinstating #7384 for that file until the source changes or the cache is cleared. Consider folding the flag into the features hash, or treating generate_module_info && module_type == Esm && esm_record.is_empty() as a cache miss on the hit branches here and in jsc_hooks.rs.
Extended reasoning...
What the bug is
The new escape hatch BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO gates whether the printer serializes an esm_record into the on-disk RuntimeTranspilerCache entry. When the flag is set, module_info is None, so have_module_info is false in js_printer/lib.rs and cache.put(..., &srlz_res) writes an entry with esm_record_byte_length == 0. But the flag does not participate in hash_for_runtime_transpiler (src/js_parser/parser.rs:362, called from parse_entry.rs:223) — that hasher only covers parser-option bools, JSX config, and bundler_feature_flags. So entries written with the flag ON and OFF share the same features_hash and cache filename.
Code path
On a subsequent run with the flag unset, RuntimeTranspilerCache::from_file accepts the entry (same input_hash, same features_hash, same cache_version = 24). The cache-hit branch at RuntimeTranspilerStore.rs:958 (and the mirror at src/runtime/jsc_hooks.rs:2810) then evaluates:
let module_info = if generate_module_info
&& entry.metadata.module_type != CacheModuleType::Cjs
&& !entry.esm_record.is_empty() // ← FALSE for the flag-ON entry
{ ... } else { ptr::null_mut() };module_info comes back null, ZigSourceProvider::create leaves the source type as plain Module, JSC re-parses the stripped output, and a type-only re-export throws SyntaxError: export 'X' not found — exactly the #7384 regression.
Why the version-24 bump doesn't cover this
The EXPECTED_VERSION bump comment at RuntimeTranspilerCache.rs:47-50 calls out precisely this failure mode ("Old entries have an empty esm_record, which would regress TypeScript type-only re-exports (#7384) to the JSC re-parse path on cache hit") — but it only invalidates pre-v24 entries. A v24 entry written under the escape hatch is indistinguishable from a normal v24 entry at load time, so the bump doesn't help.
Note also that cache.put runs at the end of the printer, before JSC module linking, so the poisoned entry is written even though the flag-ON run then fails with the SyntaxError.
Step-by-step proof
- User sets
BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO=1and runs a >4 KiB TypeScript barrel file that re-exports a type withoutexport type(the file must exceedMINIMUM_CACHE_SIZE = 4096to be cached). generate_module_info = false→module_info = None→ printer writes a v24 cache entry withesm_record_byte_length = 0. The run then fails at link time with the export not found when executing typescript file #7384 error (expected — the escape hatch restores the old behavior), but the cache entry is already on disk.- User unsets the flag and re-runs.
features_hashis unchanged, sofrom_filereturns the entry from step 2. - The hit branch checks
!entry.esm_record.is_empty()→ false →module_info = null. - JSC re-parses the transpiled output; the erased type name still appears in
export { … }; module linking throwsSyntaxError: export 'ValueOf' not found. export not found when executing typescript file #7384 recurs on a run where the fix is nominally enabled.
A serialized ModuleInfo is never zero bytes (it always writes u32 length prefixes even for an empty module), so esm_record.is_empty() on a v24 ESM entry reliably signals "written under the escape hatch" — which makes the miss-on-empty option robust.
Impact
Low. Requires a user to deliberately toggle a debug escape-hatch env var between runs on a >4 KiB file, and the failure they see (#7384's error) is the same one the escape hatch is meant to reproduce — it just persists one extra run. Clearing the cache or touching the source file recovers. Filed as a nit because it's the exact cache-coherence hole the PR's own version-bump rationale describes and it violates REVIEW.md "Cache keys cover every input that shapes the output", but it should not block the PR.
How to fix
Either:
- Fold
BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFOinto the features hash (e.g., mix it into theWyhashinRuntimeTranspilerCache::getafterhash_for_runtime_transpiler), or - On the cache-hit branches in
RuntimeTranspilerStore.rsandjsc_hooks.rs, treatgenerate_module_info && module_type == Esm && entry.esm_record.is_empty()as a miss and fall through to a fresh transpile.
|
|
||
| let is_commonjs_module = parse_result.ast.has_commonjs_export_names | ||
| || parse_result.ast.exports_kind == bun_ast::ExportsKind::Cjs; | ||
| // Collect the ESM record while printing, for the isolation | ||
| // source-provider cache (same shape as `RuntimeTranspilerStore`). | ||
| // SAFETY: per fn contract — `jsc_vm` is the live per-thread VM. | ||
| // Collect the ESM record while printing (same shape as | ||
| // `RuntimeTranspilerStore`). | ||
| let mut module_info: Option< | ||
| Box<bun_bundler::analyze_transpiled_module::ModuleInfo>, | ||
| > = if unsafe { &*jsc_vm }.use_isolation_source_provider_cache() | ||
| > = if !bun_core::env_var::feature_flag::BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO::get() | ||
| .unwrap_or(false) | ||
| && !is_commonjs_module | ||
| && loader.is_java_script_like() | ||
| { |
There was a problem hiding this comment.
🟡 There's a third runtime print_with_source_map caller — AsyncModule::resume_loading_module (src/jsc/AsyncModule.rs:1312) — that still passes None for module_info. It's reached via the branch just above this block (jsc_hooks.rs:2906, parse_result.pending_imports.len() > 0 → auto-install / --install=fallback / async plugin onResolve), so a TS type-only re-export in such a module still hits the JSC re-parse path. Not a regression (it was None before too) and the trigger is narrow, but per REVIEW.md's "grep for every sibling site" it'd be worth wiring the same gate there (loader and is_commonjs_module are already at hand at that call site) or noting the intentional exclusion.
Extended reasoning...
What
This PR drops the use_isolation_source_provider_cache gate at two of the three runtime print_with_source_map call sites so module_info is attached for every JS-like ESM source: RuntimeTranspilerStore::run (src/jsc/RuntimeTranspilerStore.rs:1113, async transpiler thread) and jsc_hooks::transpile_source_code_inner (src/runtime/jsc_hooks.rs:3028, sync path). The third caller — AsyncModule::resume_loading_module at src/jsc/AsyncModule.rs:1301–1313 — still passes None as the module_info argument (line 1312).
Code path
transpile_source_code_inner parses and links the source, then at jsc_hooks.rs:2906 checks parse_result.pending_imports.len() > 0. When true — i.e., linker.link produced deferred imports (auto-install / --install=fallback, or an async Bun.plugin onResolve on the sync path) — it enqueues the parse result into (*jsc_vm).modules (jsc_hooks.rs:2933) and returns Err(AsyncModule) before reaching the block this PR modified at jsc_hooks.rs:2955–2977. The queued parse result is later printed by AsyncModule::resume_loading_module, which calls print_with_source_map(..., None). The resulting ResolvedSource therefore has module_info == null, so ZigSourceProvider leaves sourceType as plain Module instead of BunTranspiledModule, JSC re-parses the stripped output, and a TypeScript type-only re-export still throws SyntaxError: export 'X' not found.
Step-by-step
utils.tscontainsexport { ValueOf, SomeConst } from './EventTypes'andimport 'pkg-not-yet-installed', run underbun --install=fallback(or with an async pluginonResolve) via a path that hits the sync transpiler (e.g.,BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1orrequire(esm)).transpile_source_code_innerparsesutils.ts;linker.linkrecordspkg-not-yet-installedas a pending import.- jsc_hooks.rs:2906 sees
pending_imports.len() > 0, enqueues to the AsyncModule queue, returns early. - After the pending import resolves,
resume_loading_moduleprints withmodule_info: None(AsyncModule.rs:1312). ResolvedSource.module_info == null→ JSC re-parses the transpiled output, seesexport { ValueOf }whereValueOfwas erased from./EventTypes, throws — the exact export not found when executing typescript file #7384 symptom.
Why existing code doesn't cover it
Both fixed call sites live after the pending_imports early return, so a module that takes the AsyncModule queue never reaches them. Grepping the three runtime print_with_source_map callers confirms AsyncModule.rs:1301 is the only one left passing None.
Impact and severity
This is not a regression — the AsyncModule path passed None before this PR too, so it was equally broken (only fixed under --isolate, which never routes through this queue anyway). The trigger is narrow: a module must both (a) contain a TypeScript type-only re-export without export type and (b) have a deferred/pending import on the sync transpile path. The PR strictly improves on the status quo and fixes the overwhelmingly common paths. Filing as a nit per the rubric; REVIEW.md's "Fix the whole class in the same PR — grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR" makes it worth flagging, not blocking.
Fix
Mechanical: at AsyncModule.rs:1256–1260, is_commonjs_module is already computed and self.loader (stored via InitOpts at jsc_hooks.rs:2938, recoverable as bun_ast::Loader::from_api(self.loader)) is available. Wire the same !BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFO && !is_commonjs_module && loader.is_java_script_like() gate + has_tla propagation there, and pass the resulting module_info through to print_with_source_map and the ResolvedSource. Alternatively, note the intentional exclusion in the PR description.
1 participant
Fixes #7384
Fixes #8439
Repro
Cause
The printer already builds a
ModuleInforecord while printing (imports, exports, var declarations, and which import entries were erased TypeScript types). When present on aResolvedSource,ZigSourceProvidertags the providerBunTranspiledModule, andJSModuleLoader::makeModuleroutes toBun__analyzeTranspiledModuleinstead of re-parsing the stripped JavaScript; the resultingJSModuleRecordcarriesm_isTypeScript=trueandImportEntryType::SingleTypeScriptfor the erased names, andCyclicModuleRecord/AbstractModuleRecordtolerate aNotFoundresolution for those entries instead of throwing.That record was only attached under
bun test --isolate(via theuse_isolation_source_provider_cache()gate inRuntimeTranspilerStore/jsc_hooks). Plainbun run/bun teststill took the default path, where JSC re-parses the transpiled output: theexport { ValueOf }is preserved verbatim,ValueOfdoes not exist in the stripped./EventTypes, and module linking throws.The same mechanism is what #15758 / #16296 were building toward; the infrastructure landed for
--compileand--isolatebut was never enabled for the runtime.Fix
use_isolation_source_provider_cachegate on both load paths (asyncRuntimeTranspilerStoreand syncjsc_hooks::transpile_source_code_inner), for both the fresh-transpile branch and the on-disk-cache-hit branch.module_infois now attached for every JS-like ESM source.BUN_FEATURE_FLAG_DISABLE_RUNTIME_MODULE_INFOas an escape hatch (falls back to JSC re-parsing).RuntimeTranspilerCacheversion (23 -> 24): entries written before this change carry an emptyesm_record, which would keep sending cache hits down the JSC re-parse path and reinstate the bug.The C++ side already handles non-isolation correctly:
Bun__analyzeTranspiledModuleeagerly frees the record after building theJSModuleRecordwhenIsolatedModuleCache::canUseis false, andIsolatedModuleCacheitself remains gated on--isolate. InBUN_DEBUGbuilds,fallbackParsecontinues to verify the Bun-derived record against JSC's own re-parse of the printed output.Verification
test/js/bun/typescript/type-export.test.ts:"run"-mode cases (re-export viaexport from/import then export/export */ star-merge, consumed viarequire/import */await import/ named import). All 18 fail on released bun (USE_SYSTEM_BUN=1: 10 fail, 8 pass) and pass with the debug build.check ownkeys from a star import > runandimport only used in decorator (#8439) > run.BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1), plus a test that the disable flag restores the old error.Sanity:
test/cli/test/isolation.test.ts(19/19),test/bundler/transpiler/runtime-transpiler.test.ts(15/15),test/js/bun/resolve/import-meta.test.js(32/32),test/js/bun/resolve/esModule-annotation.test.js(8/8) all pass on the debug build; noImports different between parseFromSourceCode and fallbackParsediagnostics observed.Error message for
SyntaxError: export 'ValueOf' not found in './EventTypes'is preserved for dedup search.