diff --git a/.github/workflows/source-lints.yml b/.github/workflows/source-lints.yml index 98775deb80d2..a9e14c51d22a 100644 --- a/.github/workflows/source-lints.yml +++ b/.github/workflows/source-lints.yml @@ -17,6 +17,7 @@ on: - "src/codegen/class-definitions.ts" - "src/js/builtins.d.ts" - "src/jsc/bindings/**" + - "src/jsc/modules/NodeModuleModule.cpp" - "packages/bun-types/redis.d.ts" - "scripts/build/**" - "scripts/glob-sources.ts" @@ -38,6 +39,7 @@ on: - "src/codegen/class-definitions.ts" - "src/js/builtins.d.ts" - "src/jsc/bindings/**" + - "src/jsc/modules/NodeModuleModule.cpp" - "packages/bun-types/redis.d.ts" - "scripts/build/**" - "scripts/glob-sources.ts" diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index 6b94b9af3f97..5ae91f914d63 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -42,7 +42,8 @@ JSC_DECLARE_HOST_FUNCTION(jsFunctionWrap); JSC_DECLARE_CUSTOM_GETTER(getterRequireFunction); JSC_DECLARE_CUSTOM_SETTER(setterRequireFunction); -// This is a list of builtin module names that do not have the node prefix. It +// module.builtinModules. Node builtins are listed without the "node:" prefix, +// except those that only resolve with it ("node:sqlite", "node:test"). It // also includes Bun's builtin modules, as well as Bun's thirdparty overrides. // The reason for overstuffing this list is so that uses that use these as the // 'external' option to a bundler will properly exclude things like 'ws' which @@ -92,6 +93,7 @@ static constexpr ASCIILiteral builtinModuleNames[] = { "module"_s, "net"_s, "node:sqlite"_s, + "node:test"_s, "os"_s, "path"_s, "path/posix"_s, diff --git a/test/internal/source-lints/builtin-module-tables.test.ts b/test/internal/source-lints/builtin-module-tables.test.ts new file mode 100644 index 000000000000..b62053f18fa5 --- /dev/null +++ b/test/internal/source-lints/builtin-module-tables.test.ts @@ -0,0 +1,73 @@ +import { expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import path from "node:path"; + +// The names of the builtin modules live in three hand-written tables: +// +// - builtinModuleNames in src/jsc/modules/NodeModuleModule.cpp is module.builtinModules. +// - builtinModuleNamesSortedLength in src/jsc/bindings/isBuiltinModule.cpp is what module.isBuiltin() +// accepts (and what Bun.plugin refuses to override). +// - node_entry_only_prefix!() in src/resolve_builtins/HardcodedModule.rs marks the builtins that the module +// loader resolves with the "node:" prefix only. Node lists those in builtinModules with the prefix. +// +// Nothing else compares them. A new module tends to land in the last two and not in the first: "node:test" +// was missing from builtinModules for over a year while isBuiltin("node:test") was true. This lint requires +// every name isBuiltin() accepts to be listed in builtinModules, unless notListed says why it is not. + +// isBuiltin() names that builtinModules leaves out on purpose, with the reason. The lint fails while an entry +// here is listed after all, or is no longer accepted by isBuiltin(). +const internalPlumbing = + "internal plumbing, not a public module (#31831). isBuiltin() accepts it so that Bun.plugin refuses to override it."; +const notListed: Record = { + "bun:main": internalPlumbing, + "bun:wrap": internalPlumbing, + "node:quic": "experimental. Stock Node 26 builds do not list it either: QUIC is compiled out of them.", +}; + +const builtinModulesFile = "src/jsc/modules/NodeModuleModule.cpp"; +const isBuiltinFile = "src/jsc/bindings/isBuiltinModule.cpp"; +const resolverFile = "src/resolve_builtins/HardcodedModule.rs"; +const root = path.resolve(import.meta.dir, "..", "..", ".."); + +function asciiLiteralTable(file: string, name: string): Set { + const source = readFileSync(path.join(root, file), "utf8"); + const table = new RegExp(String.raw`\b${name}\[\] = \{([^}]*)\};`).exec(source); + if (table === null) throw new Error(`${file} no longer defines the table ${name}[]`); + const names = new Set(Array.from(table[1].matchAll(/"([^"]+)"_s/g), m => m[1])); + if (names.size === 0) throw new Error(`${file}: the table ${name}[] has no "..."_s entries`); + return names; +} + +const builtinModules = asciiLiteralTable(builtinModulesFile, "builtinModuleNames"); +const isBuiltin = asciiLiteralTable(isBuiltinFile, "builtinModuleNamesSortedLength"); +const onlyPrefix = Array.from( + readFileSync(path.join(root, resolverFile), "utf8").matchAll(/\bnode_entry_only_prefix!\("([^"]+)"\)/g), + m => m[1], +); +if (onlyPrefix.length === 0) throw new Error(`${resolverFile} has no node_entry_only_prefix!() entries`); + +test(`${builtinModulesFile} lists every name ${isBuiltinFile} accepts, unless notListed says why not`, () => { + const missing = [...isBuiltin].filter(name => !builtinModules.has(name) && !Object.hasOwn(notListed, name)).sort(); + expect(missing).toEqual([]); +}); + +test(`${isBuiltinFile} accepts every name ${builtinModulesFile} lists`, () => { + const unknown = [...builtinModules].filter(name => !isBuiltin.has(name)).sort(); + expect(unknown).toEqual([]); +}); + +test(`${isBuiltinFile} accepts every prefix-only builtin of ${resolverFile} under its prefixed name`, () => { + const unknown = onlyPrefix.filter(name => !isBuiltin.has(name)).sort(); + expect(unknown).toEqual([]); +}); + +test("notListed names only entries that isBuiltin() still accepts and builtinModules still leaves out", () => { + const stale = Object.keys(notListed) + .flatMap(name => { + if (!isBuiltin.has(name)) return [`${name} is no longer in ${isBuiltinFile}; delete its entry`]; + if (builtinModules.has(name)) return [`${name} is listed in ${builtinModulesFile} now; delete its entry`]; + return []; + }) + .sort(); + expect(stale).toEqual([]); +}); diff --git a/test/js/node/module/node-module-module.test.js b/test/js/node/module/node-module-module.test.js index f812826ccfdf..ad0a3a4a6974 100644 --- a/test/js/node/module/node-module-module.test.js +++ b/test/js/node/module/node-module-module.test.js @@ -5,11 +5,20 @@ import Module, { _nodeModulePaths, builtinModules, createRequire, isBuiltin, wra import path from "path"; describe.concurrent("node-module-module", () => { - test("builtinModules exists", () => { + // test/internal/source-lints/builtin-module-tables.test.ts checks the table behind builtinModules against the + // table behind isBuiltin(), so this only checks what the array looks like at runtime. + test("builtinModules holds requireable builtins and lists prefix-only modules with their prefix, as Node does", () => { expect(Array.isArray(builtinModules)).toBe(true); - // "bun:wrap" is no longer listed: it is internal transpiler plumbing, - // not a requireable public module. - expect(builtinModules).toHaveLength(76); + expect(builtinModules).toContain("node:test"); + expect(builtinModules).not.toContain("test"); + // "bun:wrap" is internal transpiler plumbing, not a requireable public module. + expect(builtinModules).not.toContain("bun:wrap"); + expect(builtinModules.filter(name => !isBuiltin(name))).toEqual([]); + // A prefixed entry is listed that way because it is requireable only with its prefix. + for (const name of builtinModules.filter(name => name.startsWith("node:"))) { + expect(isBuiltin(name.slice("node:".length))).toBe(false); + expect(process.getBuiltinModule(name)).toBe(require(name)); + } }); test("isBuiltin() works", () => {