Skip to content

fix(hir,codegen): route class-extends userland stream-shim ctor through its real constructor (winston)#5357

Merged
proggeramlug merged 1 commit into
mainfrom
fix/winston-stream-subclass-state
Jun 17, 2026
Merged

fix(hir,codegen): route class-extends userland stream-shim ctor through its real constructor (winston)#5357
proggeramlug merged 1 commit into
mainfrom
fix/winston-stream-subclass-state

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Problem

Native-compiling winston (corpus compilePackages:[\"*\"]) threw
TypeError: Cannot convert undefined or null to object at
winston/lib/winston/logger.js:695const { pipes } = this._readableState
where this._readableState was undefined.

winston's Logger is class Logger extends Transform, and Transform comes
from the readable-stream npm package
(const { Stream, Transform } = require('readable-stream')), NOT node:stream.
readable-stream's hierarchy is Transform → Duplex → Readable wired with ES5
require('inherits')(...); the Readable constructor sets
this._readableState = new ReadableState(...).

Root cause

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,
purely on the textual name. So winston's class Logger extends Transform was
routed to the native js_node_stream_transform_subclass_init shim, which
installs the native stream method surface but never sets _readableState
/_writableState/_transformState. readable-stream's own code then reads
this._readableState.pipes and crashes.

A genuine node:stream import registers the name as the stream native
module (register_native_module(binding, \"stream\", Some(name))); a
readable-stream binding never does (it isn't a node builtin), so the two are
distinguishable.

Fix

  • HIR: 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
    (js_register_class_parent_dynamic + js_fetch_or_value_super), which runs
    the package's real constructor chain on the subclass instance.
  • Codegen (this_super_call.rs): the stream-family names are only treated
    as built-in parents for super() routing when HIR captured no
    extends_expr; when one was captured (the userland case) defer to the
    dynamic dispatch so the real Transform body runs.

Validation

  • cargo fmt --check clean; check_file_size.sh, gc_store_site_inventory.py,
    addr_class_inventory.py all pass; perry-hir tests green.
  • Genuine node:stream subclass path unchanged
    (test_read_undefined_proto_getter_promise_ctor.ts matches node).
  • New regression fixture test_gap_class_extends_userland_stream_shim.ts
    (ES5 inherits-built Transform → Duplex → Readable, ES6 class extends Transform) matches node byte-for-byte.
  • Full corpus 57/59 — no regressions (ws and other stream-using packages
    still green; the other failure, semver, is pre-existing and unrelated).

winston before / after & next wall

  • Before: TypeError: Cannot convert undefined or null to object at
    logger.js:695 (this._readableState undefined).

  • After: that throw is gone. winston now reaches a deeper,
    distinct cross-module wall:
    TypeError: Cannot set properties of null or undefined (setting 'needReadable')
    at readable-stream/lib/_stream_transform.js:97.

    Investigation of the new wall (probed at runtime): two interacting
    cross-module issues remain, both outside the scope of this stream-routing fix:

    1. Destructured re-export binds the wrong value — readable-stream's
      index does module.exports = require('./_stream_readable.js'); exports.Transform = require('./_stream_transform.js').
      winston's const { Transform } = require('readable-stream') resolves
      Logger's dynamic parent to the Readable function value, not Transform
      (confirmed: Logger's register_parent_dynamic parent bits == Readable's).
    2. util.inherits reached indirectly (require('inherits')(...) /
      var inherits = require('util').inherits; inherits(...)) does not always
      lower to the native js_util_inherits, so the synthetic-class parent chain
      Transform → Duplex → Readable isn't fully registered and the
      if (!(this instanceof Readable)) guard re-news a throwaway object.

    Minimal multi-module repros of both were built and confirm the behavior. A
    speculative runtime mint of the synthetic parent id was prototyped and
    reverted (it did not clear winston and added unvalidated behavioral
    surface).

Notes

Per CONTRIBUTING / CLAUDE.md, this PR does not bump Cargo.toml version, the
Current Version line, CHANGELOG.md, or Cargo.lock — the maintainer folds
those in at merge.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed class inheritance handling for userland stream-like implementations. Classes extending custom stream classes (instead of native Node.js streams) now compile correctly.
    • Added distinction between native Node.js stream constructors and userland stream-shim bindings to resolve edge cases in prototype inheritance.

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

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Adds a is_genuine_node_stream_parent helper in HIR class lowering that consults ctx.lookup_native_module to confirm a name like Readable/Writable/Duplex/Transform is actually from the node:stream native module before mapping it to the node_stream native parent. The codegen SuperCall path mirrors this by making the stream-family built-in gate conditional on extends_expr being absent. A new TypeScript test file exercises Logger extends a userland Transform shim.

Changes

Userland stream-shim parent discrimination

Layer / File(s) Summary
HIR: is_genuine_node_stream_parent helper and call sites
crates/perry-hir/src/lower_decl/class_decl.rs
Introduces the private helper is_genuine_node_stream_parent, which returns true only when ctx.lookup_native_module(name) resolves to the "stream" native module. Both lower_class_decl and lower_class_from_ast now gate the ("node_stream", <name>) mapping on this check; non-genuine bindings are no longer mapped to native parents.
Codegen: extends_expr-conditional stream built-in gate
crates/perry-codegen/src/expr/this_super_call.rs
In Expr::SuperCall, is_builtin_parent_name for stream-family names (Readable, Writable, Transform, ReadableStream, etc.) is now false when current_class.extends_expr is present, routing those through the dynamic dispatch path. Error/TypeError and is_other_builtin_constructor_name remain unconditional.
Test: Logger extending userland Transform
test-files/test_gap_class_extends_userland_stream_shim.ts
Adds a self-contained regression test with ES5-style inherits, minimal Readable/Writable/Duplex/Transform constructor chain, a Logger extends Transform class, and runtime state assertions validating _readableState, _writableState, _transformState, configured, and pipeCount().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PerryTS/perry#5095: Directly related — modifies the same class_decl.rs and expr/this_super_call.rs files to fix stream-parent/subclassing handling, forming the baseline this PR builds on.

Poem

🐇 Hoppity-hop through the stream shim maze,
Where Readable names wore a native disguise!
Now lookup_native_module peeks through the haze,
And userland Transform no longer misguides.
extends_expr speaks truth — the gate opens wide,
Winston can log with the right parent inside! 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: routing userland stream-shim constructors through their real implementation rather than the node:stream native path.
Description check ✅ Passed The description comprehensively covers the problem, root cause, fix strategy, and validation with clear examples and code references.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/winston-stream-subclass-state

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

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.

1 participant