Skip to content

node:module: build builtinModules from the same table as isBuiltin, and freeze it #33430

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.

Sign up for GitHub

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

Closed
robobun wants to merge 1 commit into from
+117 −157
Closed

node:module: build builtinModules from the same table as isBuiltin, and freeze it #33430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 79 additions & 67 deletions src/jsc/bindings/isBuiltinModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,92 +1,104 @@
#include "root.h"
#include "isBuiltinModule.h"

static constexpr ASCIILiteral builtinModuleNamesSortedLength[] = {
"fs"_s,
"os"_s,
"v8"_s,
"vm"_s,
"ws"_s,
"bun"_s,
"dns"_s,
"net"_s,
"sys"_s,
"tls"_s,
"tty"_s,
"url"_s,
"http"_s,
"path"_s,
"repl"_s,
"util"_s,
"wasi"_s,
"zlib"_s,
"dgram"_s,
"http2"_s,
"https"_s,
// Bun's own modules and its thirdparty overrides are intentionally included, so
// that passing `module.builtinModules` as the 'external' option to a bundler
// properly excludes things like 'ws', which only work with Bun's native
// implementation and not the JS one on npm.
static constexpr ASCIILiteral builtinModuleNamesTable[] = {
"_http_agent"_s,
"_http_client"_s,
"_http_common"_s,
"_http_incoming"_s,
"_http_outgoing"_s,
"_http_server"_s,
"_stream_duplex"_s,
"_stream_passthrough"_s,
"_stream_readable"_s,
"_stream_transform"_s,
"_stream_wrap"_s,
"_stream_writable"_s,
"_tls_common"_s,
"_tls_wrap"_s,
"assert"_s,
"assert/strict"_s,
"async_hooks"_s,
"buffer"_s,
"crypto"_s,
"domain"_s,
"events"_s,
"module"_s,
"stream"_s,
"timers"_s,
"undici"_s,
"bun:ffi"_s,
"bun:jsc"_s,
"bun:main"_s,
"bun:sqlite"_s,
"bun:test"_s,
"bun:wrap"_s,
"bun"_s,
"child_process"_s,
"cluster"_s,
"console"_s,
"process"_s,
"bun:wrap"_s,
"punycode"_s,
"bun:test"_s,
"bun:main"_s,
"readline"_s,
"_tls_wrap"_s,
"constants"_s,
"crypto"_s,
"dgram"_s,
"diagnostics_channel"_s,
"dns"_s,
"dns/promises"_s,
"domain"_s,
"events"_s,
"fs"_s,
"fs/promises"_s,
"http"_s,
"http2"_s,
"https"_s,
"inspector"_s,
"node:test"_s,
"bun:sqlite"_s,
"inspector/promises"_s,
"module"_s,
"net"_s,
"os"_s,
"path"_s,
"path/posix"_s,
"path/win32"_s,
"perf_hooks"_s,
"stream/web"_s,
"util/types"_s,
"_http_agent"_s,
"_tls_common"_s,
"async_hooks"_s,
"fs/promises"_s,
"process"_s,
"punycode"_s,
"querystring"_s,
"_http_client"_s,
"_http_common"_s,
"_http_server"_s,
"_stream_wrap"_s,
"dns/promises"_s,
"trace_events"_s,
"assert/strict"_s,
"child_process"_s,
"_http_incoming"_s,
"_http_outgoing"_s,
"_stream_duplex"_s,
"string_decoder"_s,
"worker_threads"_s,
"readline"_s,
"readline/promises"_s,
"repl"_s,
"stream"_s,
"stream/consumers"_s,
"stream/promises"_s,
"stream/web"_s,
"string_decoder"_s,
"sys"_s,
"timers"_s,
"timers/promises"_s,
"_stream_readable"_s,
"_stream_writable"_s,
"stream/consumers"_s,
"_stream_transform"_s,
"readline/promises"_s,
"inspector/promises"_s,
"_stream_passthrough"_s,
"diagnostics_channel"_s,
"tls"_s,
"trace_events"_s,
"tty"_s,
"undici"_s,
"url"_s,
"util"_s,
"util/types"_s,
"v8"_s,
"vm"_s,
"wasi"_s,
"worker_threads"_s,
"ws"_s,
"zlib"_s,
// Builtins that only resolve with the "node:" prefix. Node lists these with
// the prefix too, since the bare name is not a builtin.
"node:test"_s,
};

namespace Bun {

std::span<const ASCIILiteral> builtinModuleNames()
{
return builtinModuleNamesTable;
}

bool isBuiltinModule(const String& namePossiblyWithNodePrefix)
{
// First check the original name as-is
for (auto& builtinModule : builtinModuleNamesSortedLength) {
for (auto& builtinModule : builtinModuleNamesTable) {
if (namePossiblyWithNodePrefix == builtinModule)
return true;
}
Expand All @@ -97,7 +109,7 @@ bool isBuiltinModule(const String& namePossiblyWithNodePrefix)
name = name.substringSharingImpl(5);

// Check again with the prefix removed
for (auto& builtinModule : builtinModuleNamesSortedLength) {
for (auto& builtinModule : builtinModuleNamesTable) {
if (name == builtinModule)
return true;
}
Expand Down
6 changes: 6 additions & 0 deletions src/jsc/bindings/isBuiltinModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
#pragma once

#include <span>

namespace Bun {
/// The publicly listed builtin specifiers. `module.builtinModules` is built from this
/// list and `isBuiltinModule` consults it, so the two cannot disagree. Specifiers the
/// loader resolves but does not expose, like `bun:app`, are deliberately absent.
std::span<const ASCIILiteral> builtinModuleNames();
bool isBuiltinModule(const String& namePossiblyWithNodePrefix);
String isUnprefixedNodeBuiltin(const String& name);
} // namespace Bun
107 changes: 18 additions & 89 deletions src/jsc/modules/NodeModuleModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <JavaScriptCore/CallData.h>
#include <JavaScriptCore/JSPromise.h>
#include <JavaScriptCore/IteratorOperations.h>
#include <JavaScriptCore/ObjectConstructor.h>
#include "JavaScriptCore/Completion.h"
#include "JavaScriptCore/JSNativeStdFunction.h"
#include "JSCommonJSExtensions.h"
Expand Down Expand Up @@ -44,90 +45,6 @@ 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
// 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
// only work with Bun's native 'ws' implementation and not the JS one on NPM.
static constexpr ASCIILiteral builtinModuleNames[] = {
"_http_agent"_s,
"_http_client"_s,
"_http_common"_s,
"_http_incoming"_s,
"_http_outgoing"_s,
"_http_server"_s,
"_stream_duplex"_s,
"_stream_passthrough"_s,
"_stream_readable"_s,
"_stream_transform"_s,
"_stream_wrap"_s,
"_stream_writable"_s,
"_tls_common"_s,
"_tls_wrap"_s,
"assert"_s,
"assert/strict"_s,
"async_hooks"_s,
"buffer"_s,
"bun:ffi"_s,
"bun:jsc"_s,
"bun:sqlite"_s,
"bun:test"_s,
"bun:wrap"_s,
"bun"_s,
"child_process"_s,
"cluster"_s,
"console"_s,
"constants"_s,
"crypto"_s,
"dgram"_s,
"diagnostics_channel"_s,
"dns"_s,
"dns/promises"_s,
"domain"_s,
"events"_s,
"fs"_s,
"fs/promises"_s,
"http"_s,
"http2"_s,
"https"_s,
"inspector"_s,
"inspector/promises"_s,
"module"_s,
"net"_s,
"os"_s,
"path"_s,
"path/posix"_s,
"path/win32"_s,
"perf_hooks"_s,
"process"_s,
"punycode"_s,
"querystring"_s,
"readline"_s,
"readline/promises"_s,
"repl"_s,
"stream"_s,
"stream/consumers"_s,
"stream/promises"_s,
"stream/web"_s,
"string_decoder"_s,
"sys"_s,
"timers"_s,
"timers/promises"_s,
"tls"_s,
"trace_events"_s,
"tty"_s,
"undici"_s,
"url"_s,
"util"_s,
"util/types"_s,
"v8"_s,
"vm"_s,
"wasi"_s,
"worker_threads"_s,
"ws"_s,
"zlib"_s,
};

template<std::size_t N, class T> consteval std::size_t countof(T (&)[N])
{
return N;
Expand Down Expand Up @@ -655,15 +572,27 @@ static JSValue getSourceMapFunction(VM& vm, JSObject* moduleObject)

static JSValue getBuiltinModulesObject(VM& vm, JSObject* moduleObject)
{
MarkedArgumentBuffer args;
args.ensureCapacity(countof(builtinModuleNames));
auto names = Bun::builtinModuleNames();

for (unsigned i = 0; i < countof(builtinModuleNames); ++i) {
args.append(JSC::jsOwnedString(vm, String(builtinModuleNames[i])));
MarkedArgumentBuffer args;
args.ensureCapacity(names.size());
for (auto& name : names) {
args.append(JSC::jsOwnedString(vm, String(name)));
}

auto* globalObject = defaultGlobalObject(moduleObject->globalObject());
return JSC::constructArray(globalObject, static_cast<JSC::ArrayAllocationProfile*>(nullptr), JSC::ArgList(args));
auto scope = DECLARE_THROW_SCOPE(vm);
JSArray* array = JSC::constructArray(globalObject, static_cast<JSC::ArrayAllocationProfile*>(nullptr), JSC::ArgList(args));
// Neither building nor freezing a dense array of strings runs user code. The
// assertions are what let the exception-check validator see both of those
// ThrowScopes as checked before the next one opens at the same depth.
scope.assertNoException();

// Node freezes this array. Every reader shares one instance, so leaving it
// mutable lets one package's mutation leak into all the others.
JSC::objectConstructorFreeze(globalObject, array);
scope.assertNoException();
return array;
}

static JSValue getConstantsObject(VM& vm, JSObject* moduleObject)
Expand Down
15 changes: 14 additions & 1 deletion test/js/node/module/node-module-module.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,20 @@ import path from "path";
describe.concurrent("node-module-module", () => {
test("builtinModules exists", () => {
expect(Array.isArray(builtinModules)).toBe(true);
expect(builtinModules).toHaveLength(76);
expect(builtinModules).toHaveLength(78);
});

test("builtinModules agrees with isBuiltin()", () => {
expect(builtinModules.filter(name => !isBuiltin(name))).toEqual([]);
// "node:test" only resolves with the prefix, so it is listed with the prefix.
expect(builtinModules).toContain("node:test");
expect(builtinModules).not.toContain("test");
});

test("builtinModules is frozen", () => {
expect(Object.isFrozen(builtinModules)).toBe(true);
expect(() => builtinModules.push("not-a-builtin")).toThrow(TypeError);
expect(builtinModules).not.toContain("not-a-builtin");
});

test("isBuiltin() works", () => {
Expand Down
Loading