From f41254624fab8c34d4613e2bb4e9f2bc5a516a1a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 22:21:00 +0000 Subject: [PATCH 1/4] node:vm: fix import attributes for duplicate specifiers in SourceTextModule createModuleRecord index-aligned the AST import statements with JSC's requestedModules(), but that list is deduplicated by specifier (first occurrence wins). Any repeated specifier made the statements outnumber the requests, shifting every later attributes lookup: debug builds hit the ASSERT 'More attributes nodes than requests', and release builds handed link() the wrong attributes for every import after the duplicate. Map each unique specifier to its import attributes node (first occurrence wins, mirroring ModuleAnalyzer's dedup) instead of aligning by position. Build the attributes object from the literal with {} source, falling back to the parsed fetch parameters only for export-from requests, which also stops the internal hostDefinedImportType field from leaking into the user-visible attributes. --- src/jsc/bindings/NodeVMSourceTextModule.cpp | 75 +++++++++------------ test/js/node/vm/vm.test.ts | 46 +++++++++++++ 2 files changed, 78 insertions(+), 43 deletions(-) diff --git a/src/jsc/bindings/NodeVMSourceTextModule.cpp b/src/jsc/bindings/NodeVMSourceTextModule.cpp index ae87e5b014e9..ae077536fabe 100644 --- a/src/jsc/bindings/NodeVMSourceTextModule.cpp +++ b/src/jsc/bindings/NodeVMSourceTextModule.cpp @@ -5,6 +5,7 @@ #include "ErrorCode.h" #include "JSDOMExceptionHandling.h" +#include "wtf/HashMap.h" #include "wtf/HashSet.h" #include "wtf/Scope.h" @@ -214,29 +215,24 @@ JSValue NodeVMSourceTextModule::createModuleRecord(JSGlobalObject* globalObject) const auto& builtinNames = WebCore::clientData(vm)->builtinNames(); const Identifier& specifierIdentifier = builtinNames.specifierPublicName(); const Identifier& attributesIdentifier = builtinNames.attributesPublicName(); - const Identifier& hostDefinedImportTypeIdentifier = builtinNames.hostDefinedImportTypePublicName(); - WTF::Vector attributesNodes; - attributesNodes.reserveInitialCapacity(requests.size()); + // JSC deduplicates requestedModules() by specifier (first occurrence wins, + // see ModuleAnalyzer::appendRequestedModule), so the import statements can + // outnumber the requests and positional alignment breaks on any repeated + // specifier. Map each unique specifier to its import attributes node, keeping + // the first occurrence to mirror that deduplication. + WTF::UncheckedKeyHashMap, ImportAttributesListNode*, IdentifierRepHash> attributesNodesBySpecifier; for (StatementNode* statement = node->statements()->firstStatement(); statement; statement = statement->next()) { - // Assumption: module declarations occur here in the same order they occur in `requestedModules`. - if (statement->isModuleDeclarationNode()) { - ModuleDeclarationNode* moduleDeclaration = static_cast(statement); - if (moduleDeclaration->isImportDeclarationNode()) { - ImportDeclarationNode* importDeclaration = static_cast(moduleDeclaration); - ASSERT_WITH_MESSAGE(attributesNodes.size() < requests.size(), "More attributes nodes than requests"); - ASSERT_WITH_MESSAGE(importDeclaration->moduleName()->moduleName().string().string() == requests.at(attributesNodes.size()).m_specifier.string(), "Module name mismatch"); - attributesNodes.append(importDeclaration->attributesList()); - } else if (moduleDeclaration->hasAttributesList()) { - // Necessary to make the indices of `attributesNodes` and `requests` match up - attributesNodes.append(nullptr); - } - } + if (!statement->isModuleDeclarationNode()) + continue; + ModuleDeclarationNode* moduleDeclaration = static_cast(statement); + if (!moduleDeclaration->isImportDeclarationNode()) + continue; + ImportDeclarationNode* importDeclaration = static_cast(moduleDeclaration); + attributesNodesBySpecifier.add(importDeclaration->moduleName()->moduleName().impl(), importDeclaration->attributesList()); } - ASSERT_WITH_MESSAGE(attributesNodes.size() >= requests.size(), "Attributes node count doesn't match request count (%zu < %zu)", attributesNodes.size(), requests.size()); - for (unsigned i = 0; i < requests.size(); ++i) { const auto& request = requests[i]; @@ -245,50 +241,43 @@ JSValue NodeVMSourceTextModule::createModuleRecord(JSGlobalObject* globalObject) JSObject* requestObject = constructEmptyObject(globalObject, globalObject->objectPrototype(), 2); requestObject->putDirect(vm, specifierIdentifier, specifierValue); - WTF::String attributesTypeString = "unknown"_str; - WTF::HashMap attributeMap; JSObject* attributesObject = constructEmptyObject(globalObject); - if (request.m_attributes) { - JSValue attributesType {}; + if (ImportAttributesListNode* attributesNode = attributesNodesBySpecifier.get(request.m_specifier.impl())) { + // Surface the literal `with { ... }` attributes from source, matching + // what Node exposes to the link callback. + for (auto [key, value] : attributesNode->attributes()) { + attributeMap.set(key->string(), value->string()); + attributesObject->putDirect(vm, *key, JSC::jsString(vm, value->string())); + } + } else if (request.m_attributes) { + // Export-from requests have no import statement node; derive the `type` + // from the parsed fetch parameters. A host-defined type (e.g. css) + // surfaces as the `type` value, never as a separate internal field. + WTF::String attributesTypeString; switch (request.m_attributes->type()) { using AttributeType = decltype(request.m_attributes->type()); using enum AttributeType; - case None: - attributesTypeString = "none"_str; - attributesType = JSC::jsString(vm, attributesTypeString); - break; case JavaScript: attributesTypeString = "javascript"_str; - attributesType = JSC::jsString(vm, attributesTypeString); break; case WebAssembly: attributesTypeString = "webassembly"_str; - attributesType = JSC::jsString(vm, attributesTypeString); break; case JSON: attributesTypeString = "json"_str; - attributesType = JSC::jsString(vm, attributesTypeString); + break; + case None: break; default: - attributesType = JSC::jsNumber(static_cast(request.m_attributes->type())); + attributesTypeString = request.m_attributes->hostDefinedImportType(); break; } - attributeMap.set("type"_s, WTF::move(attributesTypeString)); - attributesObject->putDirect(vm, JSC::Identifier::fromString(vm, "type"_s), attributesType); - - if (const String& hostDefinedImportType = request.m_attributes->hostDefinedImportType(); !hostDefinedImportType.isEmpty()) { - attributesObject->putDirect(vm, hostDefinedImportTypeIdentifier, JSC::jsString(vm, hostDefinedImportType)); - attributeMap.set("hostDefinedImportType"_s, hostDefinedImportType); - } - } - - if (ImportAttributesListNode* attributesNode = attributesNodes.at(i)) { - for (auto [key, value] : attributesNode->attributes()) { - attributeMap.set(key->string(), value->string()); - attributesObject->putDirect(vm, *key, JSC::jsString(vm, value->string())); + if (!attributesTypeString.isEmpty()) { + attributeMap.set("type"_s, attributesTypeString); + attributesObject->putDirect(vm, vm.propertyNames->type, JSC::jsString(vm, attributesTypeString)); } } diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 296971c440be..d97f16f4538c 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1177,3 +1177,49 @@ describe("node:vm SourceTextModule cyclic graph linking", () => { expect(exitCode).toBe(0); }); }); + +test("node:vm SourceTextModule maps import attributes per deduplicated request", async () => { + // JSC deduplicates requestedModules() by specifier, so a repeated import + // specifier makes the import statements outnumber the requests. The native + // createModuleRecord used to index-align statements with requests, which + // aborts debug builds (ASSERTION FAILED: More attributes nodes than requests) + // and hands link() the wrong attributes for every later import on release. + const fixture = ` + const vm = require("node:vm"); + const dep = new vm.SyntheticModule(["default", "a"], function () { + this.setExport("default", 1); + this.setExport("a", 2); + }); + + const seen = []; + const m = new vm.SourceTextModule( + "import 'a'; import 'a'; import j from 'j' with { type: 'json' }; import c from 'c' with { type: 'css' };"); + await m.link((s, ref, extra) => { seen.push([s, extra?.attributes]); return dep; }); + + const seen2 = []; + const m2 = new vm.SourceTextModule("import {a} from 'x'; import 'x'; export const v = a;"); + await m2.link((s, ref, extra) => { seen2.push([s, extra?.attributes]); return dep; }); + + console.log(JSON.stringify({ seen, seen2 })); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual({ + seen: [ + ["a", {}], + ["j", { type: "json" }], + ["c", { type: "css" }], + ], + seen2: [["x", {}]], + }); + expect(exitCode).toBe(0); +}); From 9a31816eea58faa7f6ba6c1298ed79629f5d9ee6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 23:06:27 +0000 Subject: [PATCH 2/4] node:vm: derive request type from parsed attributes, recover extra keys Address review on duplicate import attribute handling: - requestedModules() dedups by (specifier, phase), not specifier alone, so a specifier-keyed node map could hand a request the wrong statement's attributes (import defer sharing a specifier with an attributed import, or an export-from winning the dedup race before a same-specifier import). Take the authoritative type from the request's own parsed fetch parameters, and use the import statement node only to recover non-type attribute keys the parser discards. - Remove the now-dead hostDefinedImportType builtin name; its only consumer was removed when the internal field stopped leaking into user attributes. --- src/js/builtins/BunBuiltinNames.h | 1 - src/jsc/bindings/NodeVMSourceTextModule.cpp | 39 ++++++++++++--------- test/js/node/vm/vm.test.ts | 25 ++++++++++++- 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/src/js/builtins/BunBuiltinNames.h b/src/js/builtins/BunBuiltinNames.h index a56e6b55d2ad..3134a9eba2ea 100644 --- a/src/js/builtins/BunBuiltinNames.h +++ b/src/js/builtins/BunBuiltinNames.h @@ -105,7 +105,6 @@ using namespace JSC; macro(headers) \ macro(highWaterMark) \ macro(host) \ - macro(hostDefinedImportType) \ macro(hostname) \ macro(httpOnly) \ macro(ignoreBOM) \ diff --git a/src/jsc/bindings/NodeVMSourceTextModule.cpp b/src/jsc/bindings/NodeVMSourceTextModule.cpp index ae077536fabe..e90298b54c46 100644 --- a/src/jsc/bindings/NodeVMSourceTextModule.cpp +++ b/src/jsc/bindings/NodeVMSourceTextModule.cpp @@ -216,11 +216,14 @@ JSValue NodeVMSourceTextModule::createModuleRecord(JSGlobalObject* globalObject) const Identifier& specifierIdentifier = builtinNames.specifierPublicName(); const Identifier& attributesIdentifier = builtinNames.attributesPublicName(); - // JSC deduplicates requestedModules() by specifier (first occurrence wins, - // see ModuleAnalyzer::appendRequestedModule), so the import statements can - // outnumber the requests and positional alignment breaks on any repeated - // specifier. Map each unique specifier to its import attributes node, keeping - // the first occurrence to mirror that deduplication. + // `requestedModules()` is deduplicated by (specifier, phase) with the first + // occurrence winning (see ModuleAnalyzer::appendRequestedModule), so the + // import statements can outnumber the requests and positional alignment + // breaks on any repeated specifier. The request already carries the winning + // statement's parsed `type`, so derive `type` from it directly. The parser + // discards any non-`type` attributes, so recover those from the matching + // import statement, keyed by specifier (first occurrence wins, mirroring the + // deduplication). WTF::UncheckedKeyHashMap, ImportAttributesListNode*, IdentifierRepHash> attributesNodesBySpecifier; for (StatementNode* statement = node->statements()->firstStatement(); statement; statement = statement->next()) { @@ -244,17 +247,10 @@ JSValue NodeVMSourceTextModule::createModuleRecord(JSGlobalObject* globalObject) WTF::HashMap attributeMap; JSObject* attributesObject = constructEmptyObject(globalObject); - if (ImportAttributesListNode* attributesNode = attributesNodesBySpecifier.get(request.m_specifier.impl())) { - // Surface the literal `with { ... }` attributes from source, matching - // what Node exposes to the link callback. - for (auto [key, value] : attributesNode->attributes()) { - attributeMap.set(key->string(), value->string()); - attributesObject->putDirect(vm, *key, JSC::jsString(vm, value->string())); - } - } else if (request.m_attributes) { - // Export-from requests have no import statement node; derive the `type` - // from the parsed fetch parameters. A host-defined type (e.g. css) - // surfaces as the `type` value, never as a separate internal field. + // `type` is authoritative from the deduplicated request. A host-defined + // type (e.g. css) surfaces as the `type` value, never as a separate + // internal field. + if (request.m_attributes) { WTF::String attributesTypeString; switch (request.m_attributes->type()) { using AttributeType = decltype(request.m_attributes->type()); @@ -281,6 +277,17 @@ JSValue NodeVMSourceTextModule::createModuleRecord(JSGlobalObject* globalObject) } } + // Surface any non-`type` attributes the user wrote; the parser keeps only + // `type` on the request, so they are recovered from the import statement. + if (ImportAttributesListNode* attributesNode = attributesNodesBySpecifier.get(request.m_specifier.impl())) { + for (auto [key, value] : attributesNode->attributes()) { + if (*key == vm.propertyNames->type) + continue; + attributeMap.set(key->string(), value->string()); + attributesObject->putDirect(vm, *key, JSC::jsString(vm, value->string())); + } + } + requestObject->putDirect(vm, attributesIdentifier, attributesObject); addModuleRequest({ request.m_specifier.string(), WTF::move(attributeMap) }); requestsArray->putDirectIndex(globalObject, i, requestObject); diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index d97f16f4538c..a8bc1effcd78 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1200,7 +1200,24 @@ test("node:vm SourceTextModule maps import attributes per deduplicated request", const m2 = new vm.SourceTextModule("import {a} from 'x'; import 'x'; export const v = a;"); await m2.link((s, ref, extra) => { seen2.push([s, extra?.attributes]); return dep; }); - console.log(JSON.stringify({ seen, seen2 })); + // import defer and a plain import of the same specifier are two distinct + // requests (Evaluation + Defer phases); only the attributed one has a type. + const seenDefer = []; + const m3 = new vm.SourceTextModule("import d from 'x' with { type: 'json' }; import defer * as ns from 'x';"); + await m3.link((s, ref, extra) => { seenDefer.push([s, extra?.attributes]); return dep; }); + + // An export-from wins the dedup race over a later same-specifier import, so + // the request keeps the export's (absent) attributes, not the import's. + const seenExport = []; + const m4 = new vm.SourceTextModule("export * from 'x' with { type: 'css' }; import 'x' with { type: 'json' };"); + await m4.link((s, ref, extra) => { seenExport.push([s, extra?.attributes]); return dep; }); + + // Non-type attributes are ignored by the parser but still surfaced to link. + const seenExtra = []; + const m5 = new vm.SourceTextModule("import j from 'j' with { type: 'json', foo: 'bar' };"); + await m5.link((s, ref, extra) => { seenExtra.push([s, extra?.attributes]); return dep; }); + + console.log(JSON.stringify({ seen, seen2, seenDefer, seenExport, seenExtra })); `; await using proc = Bun.spawn({ @@ -1220,6 +1237,12 @@ test("node:vm SourceTextModule maps import attributes per deduplicated request", ["c", { type: "css" }], ], seen2: [["x", {}]], + seenDefer: [ + ["x", { type: "json" }], + ["x", {}], + ], + seenExport: [["x", { type: "css" }]], + seenExtra: [["j", { type: "json", foo: "bar" }]], }); expect(exitCode).toBe(0); }); From 9253e85ba8fdac6618ba143fd33d8ade5e57df9d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 23:26:12 +0000 Subject: [PATCH 3/4] test: correct stale comment on export-from dedup case --- test/js/node/vm/vm.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index a8bc1effcd78..79be3ee33a69 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1207,7 +1207,7 @@ test("node:vm SourceTextModule maps import attributes per deduplicated request", await m3.link((s, ref, extra) => { seenDefer.push([s, extra?.attributes]); return dep; }); // An export-from wins the dedup race over a later same-specifier import, so - // the request keeps the export's (absent) attributes, not the import's. + // the request reports the export's type, not the import's. const seenExport = []; const m4 = new vm.SourceTextModule("export * from 'x' with { type: 'css' }; import 'x' with { type: 'json' };"); await m4.link((s, ref, extra) => { seenExport.push([s, extra?.attributes]); return dep; }); From a810bddfc283e55b2cdb1c2b305d4f7679df8620 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 7 Jul 2026 00:45:55 +0000 Subject: [PATCH 4/4] ci: retrigger