From ab5ad4fcff828870576fbf7da3b34a8d406fb35c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 17 Jun 2026 23:21:51 +0200 Subject: [PATCH] fix(hir,codegen): route class-extends userland stream-shim ctor through its real constructor `class Logger extends Transform` where `Transform` is bound from a userland stream-shim package (`const { Transform } = require('readable-stream')`, as in winston's logger.js) was routed to the native node:stream subclass-init shim purely on the textual parent name. The shim installs the native stream surface but never sets `this._readableState`/`_writableState`/`_transformState`, so winston's `const { pipes } = this._readableState` threw 'Cannot convert undefined or null to object'. Root: the native-parent match in lower_class_decl / lower_class_from_ast (perry-hir/src/lower_decl/class_decl.rs) treated any parent literally named Readable/Writable/Duplex/Transform as the node:stream builtin, regardless of where the binding came from. Genuine node:stream imports register the name as the 'stream' native module (register_native_module(binding,"stream",..)); a readable-stream binding does not (it is not a node builtin). Fix: - New is_genuine_node_stream_parent() gate: the four classic stream names map to the native node_stream parent only when lookup_native_module(name) resolves to the 'stream' module. Otherwise the class falls through to the dynamic extends_expr parent path, which runs the package's real constructor chain (Transform -> Duplex -> Readable, wired via util.inherits) on the subclass instance, so the _readableState/_writableState/_transformState objects are set. - Codegen (this_super_call.rs): the stream-family names are only 'builtin' parents for super() routing when HIR captured no extends_expr; when one was captured (the userland case) defer to the js_fetch_or_value_super dynamic dispatch. Verified the genuine node:stream subclass path is unchanged (test_read_undefined_proto_getter_promise_ctor.ts) and the new fixture test_gap_class_extends_userland_stream_shim.ts matches node byte-for-byte. Full corpus 57/59 (no regressions; ws and other stream users still green). winston advances past the _readableState-undefined wall to a deeper cross-module function-identity / util.inherits-resolution wall (the destructured Transform re-export binds to the wrong function value and the require('inherits')(...) chain is not fully linked), tracked separately. --- .../perry-codegen/src/expr/this_super_call.rs | 64 +++++++++------ crates/perry-hir/src/lower_decl/class_decl.rs | 61 +++++++++++--- ..._gap_class_extends_userland_stream_shim.ts | 79 +++++++++++++++++++ 3 files changed, 172 insertions(+), 32 deletions(-) create mode 100644 test-files/test_gap_class_extends_userland_stream_shim.ts diff --git a/crates/perry-codegen/src/expr/this_super_call.rs b/crates/perry-codegen/src/expr/this_super_call.rs index 130fe4a9d..ba8f0bd19 100644 --- a/crates/perry-codegen/src/expr/this_super_call.rs +++ b/crates/perry-codegen/src/expr/this_super_call.rs @@ -245,29 +245,47 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // semantics (Error sets this.message + this.name; streams allocate // a registry handle). Anything else with an extends_expr is a // real runtime-value parent and routes through this dispatch. - let is_builtin_parent_name = - matches!( - parent_name.as_str(), - "Error" - | "TypeError" - | "RangeError" - | "ReferenceError" - | "SyntaxError" - | "URIError" - | "EvalError" - | "AggregateError" - | "Readable" - | "Writable" - | "Duplex" - | "Transform" - | "ReadableStream" - | "WritableStream" - | "TransformStream" - | "Request" - | "Response" - | "Event" - | "CustomEvent" - ) || is_other_builtin_constructor_name(parent_name.as_str()); + // The classic node:stream / Web-Streams names are only the + // genuine built-in parents when HIR did NOT capture an + // `extends_expr`. When it did, the parent is a userland + // stream-shim value (e.g. readable-stream's `Transform`, + // winston's `class Logger extends Transform`) whose real + // constructor — which sets `this._readableState`, + // `this._writableState`, `this._transformState` — must run. + // HIR's `is_genuine_node_stream_parent` gate only leaves + // `extends_expr` set for the non-builtin case (the genuine + // node:stream import keeps `native_extends` + no + // `extends_expr`), so deferring to the dynamic dispatch here + // whenever an `extends_expr` exists is safe. + let has_extends_expr = current_class.extends_expr.is_some(); + let is_stream_family_name = matches!( + parent_name.as_str(), + "Readable" + | "Writable" + | "Duplex" + | "Transform" + | "ReadableStream" + | "WritableStream" + | "TransformStream" + ); + let is_builtin_parent_name = (matches!( + parent_name.as_str(), + "Error" + | "TypeError" + | "RangeError" + | "ReferenceError" + | "SyntaxError" + | "URIError" + | "EvalError" + | "AggregateError" + | "Request" + | "Response" + | "Event" + | "CustomEvent" + ) || (is_stream_family_name + && !has_extends_expr) + || is_other_builtin_constructor_name(parent_name.as_str())) + && !(is_stream_family_name && has_extends_expr); if !is_builtin_parent_name { if let Some(extends_expr) = current_class.extends_expr.as_deref() { // Lower the super-call args first so they get fresh slots diff --git a/crates/perry-hir/src/lower_decl/class_decl.rs b/crates/perry-hir/src/lower_decl/class_decl.rs index 3b86d1829..41825adc8 100644 --- a/crates/perry-hir/src/lower_decl/class_decl.rs +++ b/crates/perry-hir/src/lower_decl/class_decl.rs @@ -11,6 +11,39 @@ use crate::lower::{ use crate::lower_patterns::*; use crate::lower_types::*; +/// The four classic node:stream base-class names (`Readable`/`Writable`/ +/// `Duplex`/`Transform`). When a class extends a parent with one of these +/// textual names, perry routes `super()` to the native `js_node_stream_*` +/// shim (which installs the native stream surface but never sets the +/// `_readableState`/`_writableState`/`_transformState` objects). That is only +/// correct when the name actually resolves to the `node:stream` builtin — +/// i.e. it was imported from `stream`/`node:stream`, in which case the import +/// machinery registered it as the `stream` native module +/// (`register_native_module(binding, "stream", Some(name))`, see +/// `var_decl_sources::register_destructured_stream_ctors` and the import +/// arms in `lower/module_decl.rs`). +/// +/// The same textual name can instead be a userland binding from a +/// stream-shim npm package — `const { Transform } = +/// require('readable-stream')` in winston's `logger.js`, where +/// `class Logger extends Transform` then reads `this._readableState.pipes` +/// directly. Such a binding never registers as the `stream` native module +/// (readable-stream is not a node builtin), so the package's real constructor +/// must run instead. Returning `false` here lets the unknown-Ident arm +/// capture the parent as `extends_expr` and route `super()` through the +/// dynamic-parent path (`js_register_class_parent_dynamic` + the +/// `js_fetch_or_value_super` dispatch), which runs the real `Transform` +/// function body on the subclass instance. +fn is_genuine_node_stream_parent(ctx: &LoweringContext, name: &str) -> bool { + if !matches!(name, "Readable" | "Writable" | "Duplex" | "Transform") { + return false; + } + // Only the genuine `node:stream` builtin import registers these names as + // the `stream` native module. Anything else (a userland require/import of + // a stream-shim package, or an unbound reference) is not the builtin. + matches!(ctx.lookup_native_module(name), Some(("stream", _))) +} + use super::*; fn generic_computed_member_key<'a>( @@ -361,10 +394,16 @@ pub fn lower_class_decl( // native stream methods directly onto `this`, were already // built; this is the missing HIR wiring. Keep in lockstep // with the parallel arm in `lower_class_from_ast` below. - "Readable" => Some(("node_stream".to_string(), "Readable".to_string())), - "Writable" => Some(("node_stream".to_string(), "Writable".to_string())), - "Duplex" => Some(("node_stream".to_string(), "Duplex".to_string())), - "Transform" => Some(("node_stream".to_string(), "Transform".to_string())), + // + // Gate the classic node:stream names on `is_genuine_node_stream_parent` + // so a userland stream-shim binding (readable-stream's + // `Transform`, winston) falls through to the dynamic + // `extends_expr` parent path and runs its real constructor. + "Readable" | "Writable" | "Duplex" | "Transform" + if is_genuine_node_stream_parent(ctx, &parent_name) => + { + Some(("node_stream".to_string(), parent_name.clone())) + } _ => None, }; if native_parent.is_some() { @@ -1329,11 +1368,15 @@ pub fn lower_class_from_ast( "TransformStream".to_string(), )), // #1545: classic node:stream base classes — keep in lockstep - // with the parallel arm in `lower_class_decl` above. - "Readable" => Some(("node_stream".to_string(), "Readable".to_string())), - "Writable" => Some(("node_stream".to_string(), "Writable".to_string())), - "Duplex" => Some(("node_stream".to_string(), "Duplex".to_string())), - "Transform" => Some(("node_stream".to_string(), "Transform".to_string())), + // with the parallel arm in `lower_class_decl` above. Gated on + // `is_genuine_node_stream_parent` so a userland stream-shim + // binding (readable-stream's `Transform`) falls through to the + // dynamic `extends_expr` parent path. + "Readable" | "Writable" | "Duplex" | "Transform" + if is_genuine_node_stream_parent(ctx, &parent_name) => + { + Some(("node_stream".to_string(), parent_name.clone())) + } _ => None, }; if native_parent.is_some() { diff --git a/test-files/test_gap_class_extends_userland_stream_shim.ts b/test-files/test_gap_class_extends_userland_stream_shim.ts new file mode 100644 index 000000000..b84ff87fa --- /dev/null +++ b/test-files/test_gap_class_extends_userland_stream_shim.ts @@ -0,0 +1,79 @@ +// winston native-compile wall (branch fix/winston-stream-subclass-state): +// `class Logger extends Transform` where `Transform` is NOT node:stream's +// builtin but a userland stream-shim's ES5 function constructor +// (`const { Transform } = require('readable-stream')`). readable-stream's +// hierarchy is `Transform -> Duplex -> Readable` wired via ES5 +// `inherits()` (Object.create on the prototype), and the Readable ctor sets +// `this._readableState = new ReadableState(...)`. winston's logger.js then +// reads `const { pipes } = this._readableState`. +// +// Before the fix, perry routed `super()` for ANY parent textually named +// `Transform`/`Readable`/`Writable`/`Duplex` to the native node:stream shim +// (which never sets `_readableState`), so `this._readableState` was +// `undefined` and the destructure threw +// "Cannot convert undefined or null to object". The fix gates the native +// routing on `is_genuine_node_stream_parent` (the name must resolve to the +// `stream` native module) so a userland binding falls through to the +// dynamic `extends_expr` parent path, which runs the real constructor chain. + +function inherits(ctor: any, superCtor: any) { + ctor.super_ = superCtor; + ctor.prototype = Object.create(superCtor.prototype, { + constructor: { + value: ctor, + enumerable: false, + writable: true, + configurable: true, + }, + }); +} + +// Base of the chain: sets the state objects the userland code reads. +function Readable(this: any, options: any) { + this._readableState = { pipes: [], objectMode: !!(options && options.objectMode) }; + this.readable = true; +} + +function Writable(this: any, options: any) { + this._writableState = { finished: false }; + this.writable = true; +} + +function Duplex(this: any, options: any) { + Readable.call(this, options); + Writable.call(this, options); + this.allowHalfOpen = true; +} +inherits(Duplex, Readable); + +function Transform(this: any, options: any) { + Duplex.call(this, options); + this._transformState = { transforming: false }; + // Mirror readable-stream: read the state set by the Readable ctor. + this._readableState.needReadable = true; +} +inherits(Transform, Duplex); + +// User ES6 class extending the userland ES5 `Transform`. +class Logger extends Transform { + configured: boolean; + constructor(options: any) { + super({ objectMode: true }); + this.configured = true; + } + // Mirror winston logger.js line 695: read this._readableState directly. + pipeCount(): number { + const { pipes } = (this as any)._readableState; + return pipes.length; + } +} + +const l: any = new Logger({}); +console.log("readableState defined:", l._readableState !== undefined); +console.log("readableState.objectMode:", l._readableState.objectMode); +console.log("readableState.needReadable:", l._readableState.needReadable); +console.log("writableState defined:", l._writableState !== undefined); +console.log("transformState defined:", l._transformState !== undefined); +console.log("allowHalfOpen:", l.allowHalfOpen); +console.log("configured:", l.configured); +console.log("pipeCount:", l.pipeCount());