Skip to content

node:vm: fix import attributes for duplicate specifiers in SourceTextModule - #33563

Closed
robobun wants to merge 4 commits into
mainfrom
farm/2a0d0ad6/vm-dup-import-attributes
Closed

robobun wants to merge 4 commits into
mainfrom
farm/2a0d0ad6/vm-dup-import-attributes

Conversation

@robobun

@robobun robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

What

vm.SourceTextModule hands the wrong import attributes to the link() callback when the source repeats an import specifier, and aborts debug/canary builds outright.

import * as vm from "node:vm";
const dep = new vm.SyntheticModule(["default"], function () { this.setExport("default", 1); });
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}:${JSON.stringify(extra?.attributes)}`); return dep; });
console.log(seen);
// node --experimental-vm-modules :  a:{}   j:{"type":"json"}   c:{"type":"css"}
// bun (release), before          :  a:{}   j:{"type":"json"}   c:{"type":"json","hostDefinedImportType":"css"}
// bun (debug), before            :  ASSERTION FAILED: More attributes nodes than requests, NodeVMSourceTextModule.cpp -> SIGABRT

The idiomatic "import for bindings + import for side effects" pair is enough to crash a debug build:

new vm.SourceTextModule("import {a} from 'x'; import 'x'; export const v=a;")
// -> ASSERTION FAILED: More attributes nodes than requests -> SIGABRT

Cause

NodeVMSourceTextModule::createModuleRecord walked the AST's import statements in source order and index-aligned them with JSC's requestedModules(). But that list is deduplicated by specifier (ModuleAnalyzer::appendRequestedModule keeps only the first occurrence per specifier), so any repeated specifier makes the statements outnumber the requests and shifts every later index. On debug builds the two ASSERTs fire; on release the code reads attributesNodes.at(i) for the wrong i, so every JSON/CSS/host-defined type decision downstream of a duplicate is made on the wrong statement's attributes. Separately, a host-defined type (e.g. css) was copied into the user-visible attributes object as an internal hostDefinedImportType field.

Fix

Map each unique specifier to its import attributes node (first occurrence wins, mirroring JSC's dedup) instead of aligning by position. Build the user-visible attributes object from the literal with { ... } source, falling back to the parsed fetch parameters only for export-from requests; a host-defined type now surfaces as the type value and the internal hostDefinedImportType field no longer leaks.

Verification

bun bd test test/js/node/vm/vm.test.ts
# 207 pass, 0 fail

New regression test in test/js/node/vm/vm.test.ts covers the duplicate side-effect import, the dedup across a binding + side-effect import pair, and the css/json attribute mapping. It aborts the unfixed debug build (fail-before) and passes with the fix.

…Module

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.
@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b1e2e45e-5237-4574-8259-f7ec86e94c63

📥 Commits

Reviewing files that changed from the base of the PR and between 9d0e93d and 9253e85.

📒 Files selected for processing (3)
  • src/js/builtins/BunBuiltinNames.h
  • src/jsc/bindings/NodeVMSourceTextModule.cpp
  • test/js/node/vm/vm.test.ts
💤 Files with no reviewable changes (1)
  • src/js/builtins/BunBuiltinNames.h

Walkthrough

This PR reworks how NodeVMSourceTextModule recovers import attributes for module requests, replacing positional index-based lookup with a specifier-keyed HashMap, changing type derivation logic, removes the unused hostDefinedImportType builtin identifier, and adds a test for deduplicated import attribute mapping.

Changes

Import attribute mapping fix

Layer / File(s) Summary
Specifier-keyed attribute lookup and type derivation
src/jsc/bindings/NodeVMSourceTextModule.cpp, src/js/builtins/BunBuiltinNames.h
Replaces positional recovery of import attributes with a specifier-keyed HashMap lookup, derives type from request.m_attributes->type() via a switch, sets type only when non-empty, skips duplicate type keys among restored attributes, and removes the now-unused hostDefinedImportType builtin identifier.
Test coverage for attribute mapping
test/js/node/vm/vm.test.ts
Adds a fixture exercising repeated imports, with { type: ... }, import defer, export * from ... with { type: ... }, and extra non-type attributes, asserting the JSON output of collected extra?.attributes and process exit code.

Possibly related PRs

  • oven-sh/bun#29540: Also modifies BunBuiltinNames' BUN_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME macro list, removing identifiers in the same generated accessor mechanism.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main fix to import attribute handling in vm.SourceTextModule.
Description check ✅ Passed The description covers the bug, cause, fix, and verification, though its headings differ from the requested template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 6, 2026
Comment thread src/jsc/bindings/NodeVMSourceTextModule.cpp
Comment thread src/jsc/bindings/NodeVMSourceTextModule.cpp
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.
Comment thread src/jsc/bindings/NodeVMSourceTextModule.cpp
Comment thread test/js/node/vm/vm.test.ts
@robobun

robobun commented Jul 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:04 AM PT - Jul 7th, 2026

❌ @robobun, your commit a810bdd has some failures in Build #69511 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 33563

That installs a local version of the PR into your bun-33563 executable, so you can run:

bun-33563 --bun

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

The diff is green locally and the CI failures are unrelated infra:

  • build 69447: flaky unrelated tests (filesystem_router, es-module-lexer, spawn timeout, napi napi_wrap GC-timing on Windows baseline). Re-rolled once.
  • build 69511: both darwin 26 aarch64 - test-bun jobs hit buildkite-agent artifact download timed out after 120s before running any tests; remaining noise is retry-tagged flaky warnings on Windows (update_interactive_install, node-net-server, bun-install-registry).

No failure touches node:vm, NodeVMSourceTextModule.cpp, BunBuiltinNames.h, or vm.test.ts. All build targets succeeded. Ready for review/merge; happy to rebase if CI needs another run.

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: this branch conflicts with main and #41654 carries the same fix, rebased on current main.

@robobun robobun closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant