Repository navigation
Do not look a bare builtin name up as a package: after a ?query cut, for a preload, for a macro: import #44957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2c3f877
465b8f2
050bdc9
2e21cd1
25c7e6b
7fbe67d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,6 +202,17 @@ pub fn exposed_internal_tag(spec: &[u8]) -> Option<(Vec<u8>, crate::ResolvedSour | |
| Some((name, tag)) | ||
| } | ||
|
|
||
| /// Whether `name` is a builtin module's: an alias-table key, or an `internal/` module under `--expose-internals`. | ||
| pub fn is_builtin(name: &[u8]) -> bool { | ||
| HardcodedModule::Alias::has(name, bun_ast::Target::Bun, Default::default()) | ||
| || exposed_internal_tag(name).is_some() | ||
| } | ||
|
|
||
| /// Whether a preload skips the resolver, which takes a builtin's name for a package. Any `node:` name skips it too. | ||
| pub fn preload_skips_resolver(name: &[u8]) -> bool { | ||
| name.starts_with(b"node:") || is_builtin(name) | ||
| } | ||
|
|
||
| /// C++ entry point: whether `data[..len]` names a builtin module. | ||
| #[unsafe(no_mangle)] | ||
| unsafe extern "C" fn ModuleLoader__isBuiltin(data: *const u8, len: usize) -> bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): Maintainers now carry two copies of the "is this name a builtin" predicate that can drift apart. The new is_builtin at src/jsc/ModuleLoader.rs:206 and the existing ModuleLoader__isBuiltin at src/jsc/ModuleLoader.rs:221 compute the same answer, one through the BUN_ALIAS_MAP hash lookup and one through the linear bun_aliases_get scan flagged "PERF" at src/jsc/ModuleLoader.rs:171. Fix: have ModuleLoader__isBuiltin return is_builtin(str) so require.resolve.paths and Module._resolveLookupPaths share the single predicate; bun_aliases_get stays for its one remaining caller at src/runtime/jsc_hooks.rs:4677, which needs the Alias value. Why this was flaggedThe diff adds is_builtin at src/jsc/ModuleLoader.rs:206-209 as HardcodedModule::Alias::has(name, Target::Bun, Default::default()) || exposed_internal_tag(name).is_some(). The pre-existing C++ entry point ModuleLoader__isBuiltin at src/jsc/ModuleLoader.rs:218-222 evaluates bun_aliases_get(str).is_some() || exposed_internal_tag(str).is_some(). bun_aliases_get at src/jsc/ModuleLoader.rs:174-190 linearly scans BUN_ALIASES after the same stream_iter_alias_gated gate that Alias::get applies at src/resolve_builtins/HardcodedModule.rs:928, and BUN_ALIAS_MAP at src/resolve_builtins/HardcodedModule.rs:900 is built from the same BUN_ALIASES tables (line 830), so the two predicates are semantically identical today. No runtime behaviour differs; this is a duplicate-implementation concern under the repository's one-implementation rule, and the comment at src/jsc/ModuleLoader.rs:171-172 already asks for the hash-map form that is_builtin now provides. Nothing on the base branch prevents it, because the base had only the scan-based predicate. Verification: nit. The PR adds a second "is this name a builtin" predicate beside an existing one that gives the identical answer. New |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { spawnSync } from "bun"; | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { mkdirSync, realpathSync } from "fs"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
| import { bunEnv, bunExe, isDebug, tempDir } from "harness"; | ||
| import { tmpdir } from "os"; | ||
| import { join } from "path"; | ||
| const preloadModule = ` | ||
|
|
@@ -191,6 +191,152 @@ plugin({ | |
| expect({ stdout, stderr, exitCode }).toEqual({ stdout: "main ran\n", stderr: "", exitCode: 0 }); | ||
| }); | ||
|
|
||
| // A builtin's bare name is the builtin, as it is to `import` and to `node -r path`. The resolver would take it | ||
| // for a package. "abort-controller" is a package that Bun replaces with a built-in module. | ||
| describe("of a builtin by its bare name", () => { | ||
| const builtins = ["path", "fs/promises", "bun:sqlite", "abort-controller", "events"]; | ||
| const packages = (...names) => | ||
| Object.fromEntries( | ||
| names.flatMap(name => [ | ||
| [`node_modules/${name}/package.json`, JSON.stringify({ name, version: "1.0.0", main: "index.js" })], | ||
| [`node_modules/${name}/index.js`, `console.log("node_modules/${name} ran");`], | ||
| ]), | ||
| ); | ||
|
|
||
| async function run(cwd, args) { | ||
| const requests = []; | ||
| await using registry = Bun.serve({ | ||
| port: 0, | ||
| fetch(req) { | ||
| requests.push(new URL(req.url).pathname); | ||
| return new Response("{}", { status: 404, headers: { "content-type": "application/json" } }); | ||
| }, | ||
| }); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), ...args, "main.js"], | ||
| env: { | ||
| ...bunEnv, | ||
| NODE_NO_WARNINGS: undefined, | ||
| BUN_CONFIG_REGISTRY: registry.url.href, | ||
| NPM_CONFIG_REGISTRY: registry.url.href, | ||
| }, | ||
| cwd, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| return { stdout, stderr, exitCode, requests }; | ||
| } | ||
|
|
||
| test.each(["--preload", "--import", "--require", "bunfig.toml"])( | ||
| "%s does not run a file that the resolver has for that name", | ||
| async how => { | ||
| // A package that is no builtin, and a file, are still the resolver's. | ||
| const preloads = [...builtins, "lodashy", "./setup.js"]; | ||
| using dir = tempDir("preload-builtin-name-resolver", { | ||
| ...packages("path", "fs", "abort-controller", "lodashy"), | ||
| "node_modules/fs/promises.js": `console.log("node_modules/fs/promises.js ran");`, | ||
| "tsconfig.json": JSON.stringify({ compilerOptions: { baseUrl: "./src" } }), | ||
| "src/events.js": `console.log("src/events.js ran");`, | ||
| "setup.js": `console.log("setup.js ran");`, | ||
| "main.js": `console.log("main ran");`, | ||
| ...(how === "bunfig.toml" ? { "bunfig.toml": `preload = ${JSON.stringify(preloads)}` } : {}), | ||
| }); | ||
|
|
||
| const args = how === "bunfig.toml" ? [] : preloads.flatMap(name => [how, name]); | ||
| expect(await run(String(dir), args)).toEqual({ | ||
| stdout: "node_modules/lodashy ran\nsetup.js ran\nmain ran\n", | ||
| stderr: "", | ||
| exitCode: 0, | ||
| requests: [], | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| test("--preload does not ask the registry for a package of that name", async () => { | ||
| using dir = tempDir("preload-builtin-name-autoinstall", { "main.js": `console.log("main ran");` }); | ||
| const args = builtins.flatMap(name => ["--preload", name]); | ||
| expect(await run(String(dir), args)).toEqual({ stdout: "main ran\n", stderr: "", exitCode: 0, requests: [] }); | ||
| }); | ||
|
|
||
| // The name that goes to the resolver is the one without `file://`, so that name is the one to test. | ||
| test("--preload file://path does not run the package of that name", async () => { | ||
| using dir = tempDir("preload-builtin-name-file-url", { | ||
| ...packages("path"), | ||
| "main.js": `console.log("main ran");`, | ||
| }); | ||
| const { stdout, requests } = await run(String(dir), ["--preload", "file://path"]); | ||
| expect({ stdout, requests }).toEqual({ stdout: "main ran\n", requests: [] }); | ||
|
Comment on lines
+268
to
+269
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): The Why this was flaggedThe test at test/cli/run/preload-test.test.js:263-270 spawns Verification: nit. Trigger: any future regression on the |
||
| }); | ||
|
|
||
| const worker = preload => ` | ||
| const worker = new Worker("./worker.js", { preload: ${JSON.stringify(preload)} }); | ||
| worker.onmessage = e => { | ||
| console.log(e.data); | ||
| worker.terminate(); | ||
| }; | ||
| worker.onerror = e => { | ||
| console.log("error: " + e.message); | ||
| }; | ||
| `; | ||
|
|
||
| // The `preload` option of a Worker is "equivalent to passing the `--preload` CLI argument". Its resolver also | ||
| // takes a bare name for a `./name` file: `util` is the builtin, and `./util` is the file. | ||
| test("Worker preload does not run a file that the resolver has for that name", async () => { | ||
| using dir = tempDir("preload-builtin-name-worker", { | ||
| ...packages("path", "abort-controller", "lodashy"), | ||
| "util.ts": `console.log("util.ts ran");`, | ||
| "worker.js": `postMessage("main ran");`, | ||
| "main.js": worker(["path", "bun:sqlite", "abort-controller", "util", "./util", "lodashy"]), | ||
| }); | ||
| expect(await run(String(dir), [])).toEqual({ | ||
| stdout: "util.ts ran\nnode_modules/lodashy ran\nmain ran\n", | ||
| stderr: "", | ||
| exitCode: 0, | ||
| requests: [], | ||
| }); | ||
| }); | ||
|
|
||
| // A preload that is skipped passes the tests above too. Only the builtin `_tls_wrap` warns when it loads. | ||
| // It loads `node:tls`, which takes seconds in a debug build. | ||
| test.skipIf(isDebug).each(["--preload", "Worker preload"])("%s loads the builtin", async how => { | ||
| using dir = tempDir("preload-builtin-name-loads", { | ||
| ...packages("_tls_wrap"), | ||
| "worker.js": `postMessage("main ran");`, | ||
| "main.js": how === "--preload" ? `console.log("main ran");` : worker(["_tls_wrap"]), | ||
| }); | ||
| const { stdout, stderr, exitCode } = await run(String(dir), how === "--preload" ? [how, "_tls_wrap"] : []); | ||
| // "(node:<pid>) <warning>", then a hint that names the executable. | ||
| const warnings = stderr | ||
| .split("\n") | ||
| .map(line => line.replace(/^\(node:\d+\) /, "")) | ||
| .filter(line => line && !line.startsWith("(Use `")); | ||
| expect({ stdout, warnings, exitCode }).toEqual({ | ||
| stdout: "main ran\n", | ||
| warnings: ["[DEP0192] DeprecationWarning: The _tls_wrap module is deprecated. Use `node:tls` instead."], | ||
| exitCode: 0, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| // Under `--expose-internals` an `internal/` module is a builtin too, and `internal` is a package's name. | ||
| test("--preload of an internal/ module under --expose-internals", async () => { | ||
| using dir = tempDir("preload-builtin-name-expose-internals", { | ||
| "node_modules/internal/package.json": JSON.stringify({ name: "internal", version: "1.0.0" }), | ||
| "node_modules/internal/validators.js": `console.log("node_modules/internal/validators.js ran");`, | ||
| "main.js": `console.log("main ran");`, | ||
| }); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "--expose-internals", "--preload", "internal/validators", "main.js"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect({ stdout, stderr, exitCode }).toEqual({ stdout: "main ran\n", stderr: "", exitCode: 0 }); | ||
| }); | ||
|
|
||
| test("throws an error when preloaded module not found", async () => { | ||
| const bunfig = `preload = ["./bad-file.js"]`; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) Users who preload a replaced npm package (
--preload ws,-r abort-controller/polyfill, bunfigpreload = ["undici"]) now get Bun's replacement module instead of the package they installed, silently. src/jsc/ModuleLoader.rs:213 makes preload_skips_resolver true for every alias-table key, including the 19 third-party replacement names, so src/runtime/jsc_hooks.rs:755 never calls resolve_and_auto_install and node_modules/ws is never read. Node-r wsruns the installed package; the base branch did too. Fix: limit the resolver skip to Node/Bun builtins (node_builtin keys,bun:keys, exposed internals) and keep replaced package names on the resolver path, at both src/runtime/jsc_hooks.rs:755 and src/jsc/web_worker.rs:331.Why this was flagged
Trigger:
bun --preload ws ./app.ts,bun -r abort-controller/polyfill ..., or bunfigpreload = ["undici"]with the package installed in node_modules; this is the Node-r <pkg>migration pattern. On the base branch src/runtime/jsc_hooks.rs:753 only skipped the resolver fornode:names, so the installed package resolved and ran. After the change HardcodedModule::Alias::has at src/jsc/ModuleLoader.rs:207 is true forws,undici,node-fetch,abort-controller/polyfill,utf-8-validate,next/dist/compiled/*(src/resolve_builtins/HardcodedModule.rs:747-802), so preload_skips_resolver at src/jsc/ModuleLoader.rs:213 returns true and src/runtime/jsc_hooks.rs:756 passes the raw name to JSModuleLoader::import, which loads Bun's replacement. A patched or pinnedws/undicithe user deliberately installed is bypassed with no message; Node and base Bun ran it. Remedy: skip the resolver for node_builtin /bun:/ exposed-internal names only, leaving replaced package names on the resolver path.Verification: src/jsc/ModuleLoader.rs:206-208
is_builtinisHardcodedModule::Alias::has(name, Target::Bun, ..), andBUN_ALIAS_MAPincludes the "Thirdparty packages we override" entries at src/resolve_builtins/HardcodedModule.rs:747-802. src/jsc/ModuleLoader.rs:212-213preload_skips_resolverreturns true for those, so src/runtime/jsc_hooks.rs:755-756 hand the raw name to JSModuleLoader and node_modules/ws is never read.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not changed. The Notes of the PR body list this row as ruling 2 for a maintainer.
The reason for the current choice: a preload of
wsnow loads the module thatimport "ws"loads in the same program. The documentation says that Bun loads its own module for these specifiers "even when the package is installed innode_modules", so the program does not use the installedws, patched or not. With nonode_modules, main asks the registry forwsfor the preload, runs it, and then the program imports the module of Bun.The other choice keeps a replaced name with the resolver, for a preload and for a
?query. It needs a way to tell a replaced package from a builtin, and the alias table has no such field today. It is a small follow-up if a maintainer prefers it.