Repository navigation
Conversation
… concurrent transpiler
require() and require.resolve() of a node builtin keep the specifier as
written on both transpile paths, so require.resolve("fs") returns "fs",
require.cache["fs"] is honored and Module._resolveFilename overrides see
"fs" in files loaded with import as well as with require(). The other
aliases (ws, undici, bun:*, and @jest/globals / vitest under bun test) are
rewritten for require() on both paths, which makes require("@jest/globals")
work in files transpiled on the JS thread again.
Bumps the runtime transpiler cache version: entries written by the
concurrent transpiler carry the rewritten node: specifier and were replayed
on both paths.
|
Status: reproduced on the released 1.4.0 binary and on main; fix and tests pushed (current revision: Reproduced with:
All three pass with this branch, as does |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesThe PR centralizes hardcoded alias rewriting for the linker and runtime transpiler. It preserves written Node builtin specifiers, updates the transpiler cache format, and adds integration coverage for aliases and builtin resolution. Hardcoded alias rewrite
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:06 AM PT - Aug 16th, 2026
✅ @robobun, your commit cb5c6dc62bcfbf6986591741f7f2607f547f7dad passed in 🧪 To try this PR locally: bunx bun-pr 39316That installs a local version of the PR into your bun-39316 --bun |
…liases when resolving at runtime The runtime resolves an alias by the name in the table, not by the path it maps to: internal/cluster/round_robin_handle maps to internal:cluster/RoundRobinHandle, which does not resolve, so rewriting require() records of non-node entries broke test-cluster-accept-fail.js. Keep every Tag::Builtin require() as written (what the JS-thread linker always did) and make VirtualMachine's resolve apply the @jest/globals / vitest entries under bun test, which is what lets require() of those work on both transpile paths.
…s for dead fields (#39327) ### Problem - `LoaderHooks` (src/jsc/ModuleLoader.rs) has a `resolve` field that nothing reads. - Its only dispatch site, `module_loader::resolve_maybe_needs_trailing_slash`, was deleted in #35002; the field and the body it pointed at survived. - The body installed for it, `resolve_hook` plus its inner `resolve` in src/runtime/jsc_hooks.rs (about 470 lines), is a second copy of the live resolver, `VirtualMachine::resolve_maybe_needs_trailing_slash` / `_resolve` in src/jsc/VirtualMachine.rs. Every real caller uses the live one: `Bun.resolveSync` and `Bun__resolveSync*` (src/runtime/api/BunObject.rs), the ESM loader's `Zig__GlobalObject__resolve` (src/jsc/JSGlobalObject.rs), and `Module._findPath` (src/jsc/NodeModuleModule.rs). - The two copies had already drifted and the dead one keeps attracting patches: #37157 added the `data:` ENAMETOOLONG exemption to the live copy only; #34097 and #35703 (open) patch both copies; #39316 was first attempted against the dead copy, where it had no effect. - rustc's `dead_code` lint cannot catch this shape: the field is `pub`, and the body is referenced by the `__BUN_LOADER_HOOKS` static initializer. ### Fix - Delete the `resolve` field of `LoaderHooks`, `resolve_hook`, its inner `resolve`, and the helpers only they used (`EVAL_SUFFIX` / `STDIN_SUFFIX` / `normalize_specifier_for_resolution` / `normalize_source` in jsc_hooks.rs), plus the `resolve: resolve_hook` entry in the static. - Delete `src/bundler_jsc/PluginRunner.rs`, a re-export module whose only user was the dead body, and make the function it re-exported (`plugin_runner_on_resolve_jsc`) `pub(crate)`, since its remaining caller is in the same crate. - No behavior change: the deleted code was unreachable, and the live resolver in VirtualMachine.rs is untouched (the alias lookups there are what #39316 changes). - Test: `test/internal/source-lints/dispatch-hook-tables.test.ts` finds every `safe static __BUN_*: XHooks;` table (`LoaderHooks`, `RuntimeHooks`, `SqlRuntimeHooks`, 48 fields) and fails on any field with no `(hooks.field)(..)` call site. On main it fails naming exactly `LoaderHooks ... resolve`; with this diff it passes. It also covers the next field that loses its dispatch site in any of the three tables. - Call sites are attributed per table: only files that call the table's accessor (`loader_hooks()`, `runtime_hooks()`, `hooks()`) or name its struct in code count, and a site in a file that can reach two tables sharing the field name counts for neither (so `RuntimeHooks::timer_insert` cannot vouch for `SqlRuntimeHooks::timer_insert`). Full-line `//` comments are blanked before matching. - Verified: `bun bd` builds; `bun bd test` on test/js/bun/resolve/{resolve,resolve-error,require,import-query,import-meta,import-meta-resolve,bun-main-entry-point}, test/js/node/module/node-module-module.test.js, module-resolve-filename-paths.test.js, test/js/node/missing-module.test.js, and test/js/bun/plugin (220 + 44 pass); `bun test test/internal/source-lints/` (all 25 lints pass, the existing dead-code inventories are unaffected); `cargo fmt --check` clean. ### Background - Crate tiers: `bun_jsc` (module loader, VM) sits below `bun_runtime` (resolver plumbing, plugins, blob registry) in the crate graph, so `bun_jsc` cannot call `bun_runtime` functions directly. - Hook table: the way around that. The lower crate declares a struct of fn pointers (`LoaderHooks`, `RuntimeHooks`, `SqlRuntimeHooks`) and an `extern "Rust" { safe static __BUN_*_HOOKS: ... }`; `bun_runtime` defines that `#[no_mangle]` static with one body per field; the lower crate calls a field as `(hooks.field)(..)`. A field is live only if such a call exists somewhere. - The live resolver can sit in `bun_jsc` because the two `bun_runtime` pieces it needs are already reachable from there: the blob URL registry through `RuntimeHooks::has_blob_url`, and the plugin `onResolve` runner through `plugin_runner_on_resolve_jsc`, which was moved down into `bun_jsc`. That leaves nothing for a `LoaderHooks::resolve` route to do. <details> <summary>Evidence the field is dead</summary> ``` $ grep -rn "hooks\.resolve)" src # no dispatch site $ grep -rn "loader_hooks()" src # all in src/jsc/ModuleLoader.rs: transpile_source_code, # fetch_builtin_module, transpile_file, get_hardcoded_module, # resolve_embedded_node_file, transpile_virtual_module $ git log --oneline -S "(hooks.resolve)" -- src 85ddc95 Remove ~39k lines of dead Rust across the workspace (#35002) 23427db Rewrite Bun in Rust (#30412) ``` Lint output with main's versions of the five src files checked out: ``` error: LoaderHooks (src/jsc/ModuleLoader.rs) has hook fields that nothing dispatches: resolve (fail) every field of LoaderHooks is dispatched (pass) every field of RuntimeHooks is dispatched (pass) every field of SqlRuntimeHooks is dispatched ``` Not changed here: `bun_bundler_jsc`'s Cargo.toml still lists `bun_resolver`, which this crate no longer uses; its manifest already lists a dozen other crates its sources do not reference, so that is left for a separate manifest cleanup. `VirtualMachine.entry_point` in bun_jsc (read only by the `bun:main` branch of `_resolve`, which the alias table short-circuits before it is reached) looks similarly vestigial but sits in the function #39316 edits, so it is also left alone. </details> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Problem
require.resolve("fs")returns"node:fs"in a file loaded withimport/import(), and"fs"in the same file loaded withrequire(). Node returns"fs"(and"node:fs"only forrequire.resolve("node:fs")).import:require("fs")ignores arequire.cache["fs"]entry (node and bun'srequire()path honor it), aModule._resolveFilenameoverride sees"node:fs"instead of"fs", andrequire("internal/cluster/round_robin_handle")(vendored node tests,--expose-internals) throwsCannot find module 'internal:cluster/RoundRobinHandle'.import, a later run thatrequire()s it replays the rewritten output and returns"node:fs"too (probe in the details below).Linker::link,src/bundler/linker.rs, skip added in module pr 2 #18266, bun 1.2.6) leavesrequire()/require.resolve()records of builtins as written. The runtime resolver (VirtualMachine::resolve_maybe_needs_trailing_slash,src/jsc/VirtualMachine.rs) then maps the written name to the module, returns the written name itself forrequire.resolveof a node builtin, andoverridableRequire(src/js/builtins/CommonJS.ts) looks uprequire.cacheunder the written name.TranspilerJob::run,src/jsc/RuntimeTranspilerStore.rs) never got that skip. It prints the table's path in place of the name, which defeats all of the above, and forinternal/cluster/round_robin_handleprints a path (internal:cluster/RoundRobinHandle) the resolver does not accept at all.@jest/globals/vitestaliases into the same table, its skip leavesrequire("@jest/globals")as written too, and the runtime resolver looked the name up without the test-mode entries. Underbun testthatrequire()fails withCannot find module '@jest/globals'in the test file itself and in anyrequire()d file (@jest/globalsnot rewritten tobun-testwhen using CommonJSrequireto import #4466 again); it only worked in files loaded withimport, where the store rewrote it.Fix
Alias::rewrite_import_record(src/resolve_builtins/HardcodedModule.rs), replaces both loop bodies; the linker's private adapter module overAlias::getis deleted. The rule is the linker's existing one: arequire()/require.resolve()record of anyTag::Builtinentry stays as written, everything else (import,import(), and"bun"itself, which the printer inlines asglobalThis.Bun) is rewritten as before.require.resolve/require.cache/_resolveFilenamebehavior is defined in terms of the written name. Leaving the record alone hands the resolver exactly what it handles; this is the output every entry point and everyrequire()d file has had since 1.2.6.VirtualMachine::_resolveandresolve_maybe_needs_trailing_slashlook the name up with the VM'srewrite_jest_for_tests(builtin_alias_cfg()), so underbun testarequire()of@jest/globals/vitestresolves tobun:teston both paths, the same wayimportof them already did. Outsidebun testthe flag is false and nothing changes.test/js/node/module/node-module-module.test.js, newrequire() of node builtins keeps the written specifier on every transpile path: one probe file loaded three ways (require(), staticimport,import()) reportsrequire.resolveoffs/node:fs/sys, whetherrequire.cache["fs"]is honored forfsand not fornode:fs, and what a_resolveFilenameoverride sees; expected values are what node prints for the same file. Run with the concurrent transpiler on and off. Fails on the released binary for the twoimportloads.test/js/bun/resolve/require.test.ts, newrequire() resolves the alias on both transpile paths:internal/cluster/round_robin_handle(the entry whose path is not a resolvable name) andffirequired from arequire()d and animport()ed file. Fails on the released binary for theimport()ed file; also the case that rejects skipping only node builtins, which a first version of this change did andtest/js/node/test/parallel/test-cluster-accept-fail.jscaught in CI (that test passes again).test/js/bun/test/bun_test.test.ts, newrequire() of @jest/globals and vitest resolves to bun:test on every transpile path: the test file, arequire()d helper and animport()ed helper eachrequire()both aliases andrequire.resolveone. Fails on the released binary for the first two (MODULE_NOT_FOUND); theimport()case pins the behavior that already worked.test/js/node/module/*,test/js/bun/resolve/{import-meta,import-meta-resolve,resolve,resolve-error,builtin-esm-lazy-exports},test/js/node/missing-module.test.js,test/cli/run/{transpiler-cache,require-and-import-trailing}.test.ts,test/regression/issue/{25707,09739}.test.ts,test/js/first_party/undici/undici-primordials.test.ts,test/js/node/test_runner/node-test.test.ts,test/js/bun/test/{fake-timers,test-auto-import-jest-globals,jest-extended,expect-extend,mock/mock-module},test/js/node/test/parallel/{test-require-node-prefix,test-stream-iter-disabled,test-cluster-accept-fail}.js, and thetest/internal/source-lintsdead-export / byte-search lints. The RSS leak tests intest/cli/run/require-cache.test.tsexceed their time and memory budgets on the local debug ASAN build; the files they reload in a loop contain no import records, so this pass never runs in them.rewrite_import_record.Background
require(), and everything when a plugin is registered are transpiled on the JS thread (transpile_source_code->Linker::link). Files reached throughimportstatements orimport()are transpiled on a thread pool byRuntimeTranspilerStore(BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1turns that off, which is how the tests run a case on both paths). Each path runs its own pre-pass over the file's import records before printing.import,import(),require()orrequire.resolve()in a file;kindsays which (is_common_js()isrequireorrequire.resolve) and the printer emitspath.textas the specifier, so rewriting it here changes the string the runtime later receives.src/resolve_builtins/HardcodedModule.rs): maps names to built-in module paths, e.g.fs->node:fs,sys->node:util,ffi->bun:ffi,internal/cluster/round_robin_handle->internal:cluster/RoundRobinHandle, and, only whenCfg::rewrite_jest_for_testsis set (bun test),@jest/globals/vitest->bun:test. Resolution (Alias::get) is keyed by name; the fetch step that serves the module is keyed by path.EXPECTED_VERSIONis bumped whenever the output for unchanged input changes.src/runtime/jsc_hooks.rsalso contains aresolve_hookwith the same alias lookup; it is installed inLoaderHooksbut nothing dispatches that field, so it is left alone here.Shared-cache probe on the released binary (1.4.0)
big.cjsismodule.exports = require.resolve("fs");padded past 4 KiB;BUN_RUNTIME_TRANSPILER_CACHE_PATHpoints at an empty directory.Superseded first version
The first commit skipped the rewrite only for entries with
node_builtinset and rewrote the remainingrequire()records on both paths, on the theory that the resolver maps every other entry's path back to the same module. That holds for every entry exceptinternal/cluster/round_robin_handle, whose path is not a name the resolver accepts, so it broketest-cluster-accept-fail.json the JS-thread path. The current version keeps the linker'sTag::Builtinrule instead and fixes the jest aliases in the resolver, which is also where arequire()of them has to be handled for files the store transpiles.