fix(stream): node:stream classic subclassing (class X extends Writable/Readable/Duplex/Transform)#5095
Merged
Merged
Conversation
…` (node:stream classic subclassing) User classes extending the classic node:stream base classes threw `TypeError: Class extends value is not a constructor` at `new`. The complete codegen path (`lower_node_stream_super_init`) and runtime helpers (`js_node_stream_*_subclass_init`, which install the native stream methods directly onto `this` and pick up the subclass `_write`/ `_read` override) were already built, but the HIR `class_decl` lowering never recognised these bare names as native parents — so they fell to the unknown-Ident `extends_expr` path, which emits a dynamic parent-registration that rejects the (callable but non-constructor) node:stream export. Fix is the missing HIR wiring: add Readable/Writable/Duplex/Transform to both `native_parent` match arms in `class_decl.rs` (module tag `node_stream`, inert in the only `lookup_class_native_extends` consumer, which gates on the Web Streams tags). Also complete both `node_stream_kind` super-call arms in `this_super_call.rs` so all four bases reach `lower_node_stream_super_init` regardless of which parent-resolution branch fires. Flips console/console-class/stream-errors.ts (custom Writable subclass + ignoreErrors + ERR_CONSOLE_WRITABLE_STREAM) to byte-for-byte parity, and the 6 dedicated stream/extends-* subclass tests (previously throwing) now pass. No new dispatch entry / FFI, so no API_MANIFEST row. Refs #1545.
📝 WalkthroughWalkthroughThis PR adds support for Node.js stream base classes ( ChangesNode.js Stream Base Class Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-hir/src/lower_decl/class_decl.rs (1)
190-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve class bindings before native-name fallback for
Readable/Writable/Duplex/Transform.At Line 190 and Line 1210, the new bare-name native mapping runs before
ctx.lookup_class(&parent_name). That can mis-lower valid user code likeclass Readable {}thenclass X extends Readable {}by forcingnode_streaminstead of the lexical parent.Suggested fix
- // First check if it's a native module class - let native_parent = match parent_name.as_str() { + // Prefer lexical/class resolution first; fall back to native-name mapping. + let parent_cid = ctx.lookup_class(&parent_name); + let native_parent = if parent_cid.is_none() { + match parent_name.as_str() { "EventEmitter" => Some(("events".to_string(), "EventEmitter".to_string())), ... "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())), _ => None, - }; - if native_parent.is_some() { + } + } else { + None + }; + if let Some(cid) = parent_cid { + (Some(cid), Some(parent_name), None, None) + } else if native_parent.is_some() { (None, Some(parent_name), native_parent, None) } else { - let parent_cid = ctx.lookup_class(&parent_name); - if parent_cid.is_none() { + if true { ... } else { ... } }Also applies to: 1210-1239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower_decl/class_decl.rs` around lines 190 - 233, The native-parent bare-name mapping for "Readable"/"Writable"/"Duplex"/"Transform" currently runs before checking for a user-defined class and thus can mis-lower e.g. a lexical class named Readable; change the logic so you first call ctx.lookup_class(&parent_name) and, if it returns Some (a resolved user class), use that result and skip the native-name fallback, otherwise then apply the existing native mapping that sets native_parent; apply this same fix in both places where the mapping appears (the class-decl lowering branch that builds native_parent and the parallel arm in lower_class_from_ast) so the lexical class lookup wins and native fallback is only used when lookup returns None.
🧹 Nitpick comments (1)
crates/perry-codegen/src/expr/this_super_call.rs (1)
291-297: ⚡ Quick winDeduplicate
node_stream_kindmapping to prevent future branch drift.The two separate
match parent_name.as_str()tables encode the same mapping; this exact duplication already diverged once. Centralizing it avoids repeating this regression class.Suggested refactor
+fn node_stream_kind(parent_name: &str) -> Option<&'static str> { + match parent_name { + "Readable" => Some("readable"), + "Writable" => Some("writable"), + "Duplex" => Some("duplex"), + "Transform" => Some("transform"), + _ => None, + } +} ... - let node_stream_kind = match parent_name.as_str() { - "Readable" => Some("readable"), - "Writable" => Some("writable"), - "Duplex" => Some("duplex"), - "Transform" => Some("transform"), - _ => None, - }; - if let Some(kind) = node_stream_kind { + if let Some(kind) = node_stream_kind(parent_name.as_str()) { ... } ... - let node_stream_kind = match parent_name.as_str() { - "Readable" => Some("readable"), - "Writable" => Some("writable"), - "Duplex" => Some("duplex"), - "Transform" => Some("transform"), - _ => None, - }; - if let Some(kind) = node_stream_kind { + if let Some(kind) = node_stream_kind(parent_name.as_str()) { ... }Also applies to: 338-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen/src/expr/this_super_call.rs` around lines 291 - 297, The duplicated match on parent_name.as_str() that produces node_stream_kind should be centralized: extract the mapping into a single helper (e.g., a function like map_parent_to_stream_kind(parent_name: &str) -> Option<&'static str> or a static lookup) and replace both match blocks with calls to that helper (use the same helper where node_stream_kind is computed and in the other duplicated site), ensuring the helper returns Some("readable"/"writable"/"duplex"/"transform") or None as before so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry-hir/src/lower_decl/class_decl.rs`:
- Around line 190-233: The native-parent bare-name mapping for
"Readable"/"Writable"/"Duplex"/"Transform" currently runs before checking for a
user-defined class and thus can mis-lower e.g. a lexical class named Readable;
change the logic so you first call ctx.lookup_class(&parent_name) and, if it
returns Some (a resolved user class), use that result and skip the native-name
fallback, otherwise then apply the existing native mapping that sets
native_parent; apply this same fix in both places where the mapping appears (the
class-decl lowering branch that builds native_parent and the parallel arm in
lower_class_from_ast) so the lexical class lookup wins and native fallback is
only used when lookup returns None.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/this_super_call.rs`:
- Around line 291-297: The duplicated match on parent_name.as_str() that
produces node_stream_kind should be centralized: extract the mapping into a
single helper (e.g., a function like map_parent_to_stream_kind(parent_name:
&str) -> Option<&'static str> or a static lookup) and replace both match blocks
with calls to that helper (use the same helper where node_stream_kind is
computed and in the other duplicated site), ensuring the helper returns
Some("readable"/"writable"/"duplex"/"transform") or None as before so behavior
is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e973a80-3b6e-42ec-a1ac-dab2120fb00a
📒 Files selected for processing (2)
crates/perry-codegen/src/expr/this_super_call.rscrates/perry-hir/src/lower_decl/class_decl.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Originally chartered to close the node-suite
console/formatting gap (11 diffs / 119). Investigation found the console formatter itself has zero bugs — of the 11 diffs:time/tests — byte-identical format, only the measuredX.XXXmsduration varies → timing-variant (verified: diff is empty after normalizing the ms value).output/error-cause.ts— the only differences are V8 stack-trace frames (real file paths + node-internal frames; Perry rendersat <anonymous>). The surrounding inspect structure (Error: outer { code: 'E_OUTER', [cause]: TypeError: inner }) matches byte-for-byte → environment-variant.console-class/stream-errors.ts— a genuine bug, but not a formatter bug:class Boom extends WritablethrewTypeError: Class extends value is not a constructor, so the test couldn't even run.This PR fixes that last one — the real, deterministic gap.
Root cause
User classes extending the classic
node:streambase classes (Writable/Readable/Duplex/Transform) threw atnew. The full machinery already existed:lower_node_stream_super_init(this_super_call.rs)js_node_stream_{writable,readable,duplex,transform}_subclass_init, which install the native stream methods directly ontothisand pick up the subclass_write/_readoverride.But the HIR
class_decllowering never tagged these bare names as native parents (only the Web StreamsWritableStream/ReadableStream/TransformStreamwere). So they fell to the unknown-Identextends_exprpath, which emitsjs_register_class_parent_dynamic— and that rejects the callable-but-non-constructor node:stream export.Fix
Pure wiring of the already-built path:
crates/perry-hir/src/lower_decl/class_decl.rs— addReadable/Writable/Duplex/Transformto bothnative_parentmatch arms (module tagnode_stream, which is inert in the solelookup_class_native_extendsconsumer — it gates on the Web Streams tags).crates/perry-codegen/src/expr/this_super_call.rs— complete bothnode_stream_kindsuper-call arms (one was missingReadable, the otherWritable) so all four bases reachlower_node_stream_super_initregardless of which parent-resolution branch fires.No new dispatch entry / FFI ⇒ no
API_MANIFESTrow.Before / after —
console/console-class/stream-errors.tsValidation
stream-errorsnow passes. Remaining 10 diffs are all documented environment-variant (9 timing-value, 1 stack-trace) — none are formatter bugs.stream/extends-*+passthrough-*+transform-constructorsubclass tests (which previously threw) now pass.node-suite/streamdiffs are pre-existing (none subclass a stream base; my change is codegen-isolated to subclassing paths). The 9console/time+error-causediffs are unchanged.cargo test -p perry-hir -p perry-codegengreen.perry-runtimehas 2 pre-existingurl::node_compat::path_to_file_url_posix_*failures (CWD-sensitive, in the untouchedurlcrate).Refs #1545.
Summary by CodeRabbit
Readable,Writable,Duplex,Transform) when used in class inheritance, ensuring proper initialization and routing through stream-specific handlers.