Skip to content

fix(stream): #3663 — wire on/write/pipe + data pump on all stream construction paths - #4076

Merged
proggeramlug merged 5 commits into
mainfrom
fix/3663-stream-newdynamic
Jun 2, 2026
Merged

fix(stream): #3663 — wire on/write/pipe + data pump on all stream construction paths#4076
proggeramlug merged 5 commits into
mainfrom
fix/3663-stream-newdynamic

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Summary

Fixes #3663 — classic node:stream objects (Readable/Writable/Duplex/Transform/PassThrough) missing on/write/pipe on some construction paths.

The methods only worked on the direct ESM named-import path (import { Readable } from "stream"; new Readable(...)). Every other construction form failed:

Construction Before After
import { Readable } from "stream" works works
const { Readable } = require("stream") TypeError: on is not a function works + pumps
import * as s; new s.Readable() TypeError works + pumps
const s = require("stream"); const { Readable } = s TypeError works + pumps

The reported 65 parity failures concentrate here because Node's own stream test files use const { Readable } = require("stream").

Root cause

Two independent gaps, both keyed off how the binding is introduced:

  1. Construction. A named ESM import lowers new Readable() to Expr::New { class_name: "Readable" } → the existing lower_builtin_new stream handler builds a fully-methoded object. CJS-destructured / namespace-member bindings instead lower to Expr::NewDynamic, which fell through to an empty-object placeholder — no EventEmitter/Writable methods.

  2. Data pump. The classic-stream method routing (.pipe/.on/.write → real NativeMethodCall pump) fires during lowering only when the receiver local is tagged as a ("stream", Ctor) native instance. That tagging keys off lookup_native_module, which ESM named imports populate but CJS require-destructure does not — so even once methods existed they were no-op stubs.

Fix (4 files)

  • runtime/object/class_registry.rsjs_new_function_construct already resolves a callee value to (module, method) and dispatches tty/fs/tls stream-likes; add a stream arm so any aliasing shape that reaches it builds the real object via js_node_stream_*_new.
  • codegen/expr/new_dynamic.rs — route NewDynamic { callee: PropertyGet { NativeModuleRef("stream"), Ctor } } (the import * as stream member form) straight to lower_builtin_new.
  • hir/destructuring/var_decl.rs — register destructured / namespaced stream constructors as native-module member aliases (mirroring import { Readable }), so the existing instance-tagging + method-routing machinery treats them identically. is_require_builtin_module's allowlist is only fs/path/crypto, so a dedicated require_literal_specifier recognizes require("stream").
  • test-parity/node-suite/stream/construct-paths/namespace-member-new.ts — parity fixture for the new stream.Readable() construct-and-pump path.

Verification

  • All four construction forms now produce byte-identical output to node --experimental-strip-types for a full pipe/push/'end'/'finish' pump, plus Readable.from, r.constructor.name, and Transform/Duplex/PassThrough method presence.
  • Stream parity sweep over the affected fixtures (pipe/write/push/flow/events/duplex/transform): 172/175 match Node. The 3 mismatches are all ESM-path fixtures untouched by this change (pre-existing fidelity gaps: repipe double-pump, default-_transform throw, sync-throw stack trace).
  • Non-stream sweep (events/fs/path/crypto/buffer/util): no regressions; the only mismatches are pre-existing buffer/byte-length gaps unrelated to this change.
  • The destructure hook is gated to module == "stream" + the five constructor names, so non-stream destructures are unaffected.
  • Default auto-optimize build verified (not just PERRY_NO_AUTO_OPTIMIZE). cargo fmt --all --check clean; perry-hir tests pass.

Known follow-ups (out of scope, pre-existing)

Data-pump fidelity gaps that also affect the ESM path: repipe-after-unpipe double-pump, new Transform() with no _transform should throw ERR_METHOD_NOT_IMPLEMENTED, and sync-throw stack-trace depth.

Ralph Küpper added 5 commits June 2, 2026 09:14
new Readable()/Writable()/Duplex()/Transform()/PassThrough() only produced a
fully-methoded object on the ESM named-import path (lowered to Expr::New ->
lower_builtin_new). The CJS-destructure form (const { Readable } =
require('stream')) and namespace-member form (new stream.Readable()) lower to
NewDynamic, which fell through to an empty-object placeholder, so .on()/.write()/
.pipe() threw 'is not a function' — the bulk of issue #3663's 65 parity failures.

Two routing fixes:
- codegen new_dynamic.rs: NewDynamic { callee: PropertyGet { NativeModuleRef(
  "stream"), Ctor } } routes to lower_builtin_new (import * as stream).
- runtime js_new_function_construct: when the callee resolves to the stream.<Ctor>
  bound-method closure, dispatch to js_node_stream_*_new. Covers every aliasing
  shape (require destructure, require-into-namespace, const alias).
…stances

The classic-stream method routing (.pipe/.on/.write -> NativeMethodCall real
pump) fires during lowering only when the receiver local is tagged as a
('stream', Ctor) native instance. That tagging keys off lookup_native_module,
which the ESM named-import path populates but CJS require-destructure does not —
so const { Readable } = require('stream') left Readable a plain local, the
instance was never tagged, and methods dispatched to no-op stub closures.

Register destructured stream constructors (and the new stream.Ctor() member
form) as native-module aliases so the existing instance-registration +
method-routing machinery treats them identically to import { Readable }.
…e tagging

is_require_builtin_module's allowlist is only fs/path/crypto, so it didn't
recognize require('stream'); add require_literal_specifier which returns the
specifier verbatim and let the caller match 'stream'. This makes the direct
const { Readable } = require('stream') form (what Node's own test files use)
tag the stream instance so .pipe/.on/.write route to the real pump.
…e construction

Exercises the NewDynamic -> stream-constructor path (import * as stream;
new stream.Readable(...)) end-to-end: methods present AND data pumps + events
fire, byte-identical to node --experimental-strip-types. (CJS require('stream')
forms can't be node-compared under the ESM-only strip-types harness, but are
verified manually and share the same runtime/lowering path.)
@proggeramlug
proggeramlug merged commit 5628ce6 into main Jun 2, 2026
11 checks passed
@proggeramlug
proggeramlug deleted the fix/3663-stream-newdynamic branch June 2, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:stream: classic stream objects missing on/write/pipe on some construction paths

1 participant