fix: native-compile winston (ES5 .call(this) super-chain → 59/59 corpus)#5368
Merged
Conversation
added 3 commits
June 18, 2026 01:24
…perty path
Reading a native-module callable export AS A VALUE (`const f = util.inherits`)
and invoking it indirectly dispatches through the per-module NM_DISPATCH_REGISTRY,
populated by `js_nm_install_<module>()`. The PropertyGet on a NativeModuleRef
(`util.inherits` value) lowered to `js_native_module_property_by_name` without
ever emitting the install. The *direct* call form (`util.inherits(a, b)`) is
statically lowered straight to the runtime extern and never touches the registry,
so a module reached ONLY through the value-read path left the registry empty and
the indirect call resolved to `undefined`.
This broke winston's `class Logger extends Transform` (readable-stream):
`require('inherits')(Transform, Duplex)` is an indirect `util.inherits` value-call.
With it skipped, the ES5 parent edges were never registered, so the nested
`Readable.call(this)` guard `if (!(this instanceof Readable))` saw false, returned
a discarded `new Readable()`, and `this._readableState.needReadable = true` threw
on null.
Fix: emit `js_nm_install_<module>()` before the `js_native_module_property_by_name`
call in the NativeModuleRef PropertyGet branch (property_get.rs).
Adds tests/test_native_module_value_read_install_codegen.sh (runtime + IR assertion).
…or .call borrows
readable-stream's `Readable.prototype.on = function (ev, fn) { var res =
Stream.prototype.on.call(this, ev, fn); … }` (with `Stream = require('stream')`)
needs `Stream.prototype.on` to be EventEmitter.prototype.on. In Node
`require('stream')` IS the legacy Stream constructor; Perry models it as a
namespace object, so both `require('stream').prototype` and
`require('stream').Stream.prototype` lacked the EventEmitter methods —
`Stream.prototype.on` was undefined and the `.call` threw
"Function.prototype.call was called on a value that is not a function".
Two parts:
- install_event_emitter_prototype_methods() installs on/once/emit/
removeListener/removeAllListeners/addListener/etc. on the legacy
Stream.prototype AND the Readable/Writable/Duplex/Transform/PassThrough
prototypes, as receiver-from-`this` values (slot-0 capture = TAG_UNDEFINED
sentinel → this_value reads IMPLICIT_THIS, set by Function.prototype.call/
apply). Existing instance-bound closures always capture a real object so are
unaffected.
- get_native_module_constant resolves `require('stream').prototype` to the same
legacy Stream.prototype the `.Stream` export carries.
Adds tests/test_stream_prototype_eventemitter_borrow.sh.
…) getters
A getter installed via Object.defineProperty(Class.prototype, name, { get }) is
an ordinary method closure whose body reads this from its captured receiver
slot, not IMPLICIT_THIS. The inherited prototype-accessor walk
(inherited_proto_accessor_value) merely set IMPLICIT_THIS and called the closure
with js_closure_call0, so the getter observed the PROTOTYPE it lives on instead
of the instance — this.<ownField> came back undefined.
winston's Object.defineProperty(Logger.prototype, 'transports', { get() {
const { pipes } = this._readableState; … } }), read as this.transports inside
the Logger constructor, then threw 'Cannot convert undefined or null to object'
(this._readableState was undefined on the prototype).
Fix: route inherited_proto_accessor_value through invoke_accessor_getter, which
clones the getter closure with this rebound to the real receiver (and applies
strict/sloppy coercion), matching the own-accessor read path.
Adds tests/test_defineproperty_prototype_getter_this.sh.
📝 WalkthroughWalkthroughThree runtime correctness fixes: (1) codegen now emits a per-module install call before native-module property reads; (2) stream prototype objects receive EventEmitter method closures that resolve ChangesNative module install, stream EventEmitter prototype, and accessor this-binding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/node_stream.rs`:
- Around line 151-162: The comparison in the if statement uses the fully
qualified constant `crate::value::TAG_UNDEFINED` instead of the local
`TAG_UNDEFINED` constant that is defined and already used consistently
throughout this file in multiple other locations. Replace
`crate::value::TAG_UNDEFINED` with the local `TAG_UNDEFINED` constant to
maintain consistency with the rest of the codebase in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da51e427-217c-445f-bdb5-2344b169a119
📒 Files selected for processing (9)
crates/perry-codegen/src/expr/property_get.rscrates/perry-runtime/src/node_stream.rscrates/perry-runtime/src/node_stream_dispatch.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module_stream.rstests/test_defineproperty_prototype_getter_this.shtests/test_native_module_value_read_install_codegen.shtests/test_stream_prototype_eventemitter_borrow.sh
Comment on lines
+151
to
+162
| // A `TAG_UNDEFINED` slot-0 marks a *prototype-method value* (e.g. | ||
| // `Stream.prototype.on`, installed by `attach_event_emitter_prototype_methods`) | ||
| // rather than an instance-bound method: it has no fixed receiver and must | ||
| // read the call-site `this` (set by `Function.prototype.call`/`apply` into | ||
| // IMPLICIT_THIS), so `Stream.prototype.on.call(streamInstance, ev, fn)` | ||
| // — readable-stream's `Readable.prototype.on` wrapper — registers the | ||
| // listener on the instance, not on the prototype. `build_object` always | ||
| // captures a real object pointer, so existing instance-bound closures | ||
| // never hit this branch. | ||
| if bits != crate::value::TAG_UNDEFINED { | ||
| return f64::from_bits(bits); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that crate::value::TAG_UNDEFINED matches the local constant
# Find the definition in crate::value (likely value.rs or similar)
echo "=== Searching for TAG_UNDEFINED in value module ==="
rg -n 'pub\s+const\s+TAG_UNDEFINED' --type rust crates/perry-runtime/src/
echo ""
echo "=== Local definition in node_stream.rs ==="
rg -n '^const TAG_UNDEFINED' crates/perry-runtime/src/node_stream.rs
echo ""
echo "=== Usage of crate::value::TAG_UNDEFINED in node_stream.rs ==="
rg -n 'crate::value::TAG_UNDEFINED' crates/perry-runtime/src/node_stream.rsRepository: PerryTS/perry
Length of output: 323
🏁 Script executed:
#!/bin/bash
# Search for all TAG_UNDEFINED definitions across the entire codebase
echo "=== All TAG_UNDEFINED definitions ==="
rg -n 'TAG_UNDEFINED' --type rust crates/perry-runtime/src/ | head -20
echo ""
echo "=== Check if value.rs exists and list its contents ==="
find crates/perry-runtime/src -name "value.rs" -o -name "value" -type d
echo ""
echo "=== Check value module structure ==="
rg -n 'pub\s+(const|static).*TAG' --type rust crates/perry-runtime/src/value.rs 2>/dev/null || echo "value.rs not found or no matches"
echo ""
echo "=== Check what crate::value module exports ==="
rg -n '(pub\s+use|pub\s+const|pub\s+mod)' --type rust crates/perry-runtime/src/value.rs 2>/dev/null | head -30Repository: PerryTS/perry
Length of output: 2099
🏁 Script executed:
#!/bin/bash
# Check the value module structure
echo "=== Contents of value module directory ==="
ls -la crates/perry-runtime/src/value/
echo ""
echo "=== Check value module's mod.rs for exports ==="
rg -n '(pub\s+(use|const)|TAG)' --type rust crates/perry-runtime/src/value/mod.rs | head -40
echo ""
echo "=== Check for TAG_UNDEFINED export specifically ==="
rg -n 'pub.*TAG' --type rust crates/perry-runtime/src/value/Repository: PerryTS/perry
Length of output: 3198
Use the local TAG_UNDEFINED constant on line 160 for consistency.
Line 160 uses crate::value::TAG_UNDEFINED while the rest of this file uses the local TAG_UNDEFINED constant defined at line 60. Both constants have the identical value (0x7FFC_0000_0000_0001), but the inconsistency should be resolved. Since this file already defines and uses its own TAG_UNDEFINED throughout (e.g., lines 216, 418, 456), line 160 should use that same local constant for consistency.
🤖 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-runtime/src/node_stream.rs` around lines 151 - 162, The
comparison in the if statement uses the fully qualified constant
`crate::value::TAG_UNDEFINED` instead of the local `TAG_UNDEFINED` constant that
is defined and already used consistently throughout this file in multiple other
locations. Replace `crate::value::TAG_UNDEFINED` with the local `TAG_UNDEFINED`
constant to maintain consistency with the rest of the codebase in this file.
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
Gets winston to native-compile + run (
winston object, exit 0), the last of 59 corpus packages still failing. Corpus is now 59/59 (100%), 0 regressions.winston is multi-wall (
class Logger extends Transformfromreadable-stream, an ES5-constructor stack). The OUTERclass extends Transformrouting was already fixed (#5357); this PR clears three further walls in the INNER ES5/readable-stream machinery.Root causes & walls cleared (one commit each)
Wall 1 —
util.inheritsvalue-read never dispatched (the ES5 super-chain root)require('inherits')(Transform, Duplex)is an indirectutil.inheritsvalue-call (theinheritspackage exportsutil.inherits). Reading a native-module callable export as a value mints a BOUND_METHOD closure that dispatches through the per-moduleNM_DISPATCH_REGISTRY, populated byjs_nm_install_<module>(). But thePropertyGeton aNativeModuleRef(util.inheritsvalue) lowered tojs_native_module_property_by_namewithout emitting the install. The direct call form (util.inherits(a,b)) is statically lowered straight to the runtime extern and never needs the registry — so a module reached ONLY via the value-read path left the registry empty and the indirect call resolved toundefined.Result: the ES5 parent edges (
Mid→Base) were never registered, so the nestedReadable.call(this)guardif (!(this instanceof Readable))saw false, returned a discardednew Readable(), andthis._readableState.needReadable = truethrew onnull(_stream_transform.js:106).Fix: emit
js_nm_install_<module>()beforejs_native_module_property_by_namein theNativeModuleRefPropertyGet branch (property_get.rs).Wall 2 —
Stream.prototype.on.call(this, …)borrow threwreadable-stream's
Readable.prototype.on = function (ev, fn) { var res = Stream.prototype.on.call(this, ev, fn); … }(withStream = require('stream')). In Noderequire('stream')IS the legacyStreamconstructor, soStream.prototype.onisEventEmitter.prototype.on. Perry models the module as a namespace object, so bothrequire('stream').prototypeandrequire('stream').Stream.prototypelacked the EventEmitter methods —Stream.prototype.onwasundefinedand the.callthrew "Function.prototype.call was called on a value that is not a function" (_stream_readable.js:745).Fix:
install_event_emitter_prototype_methods()putson/once/emit/removeListener/removeAllListeners/… on the legacyStream.prototypeAND theReadable/Writable/Duplex/Transform/PassThroughprototypes as receiver-from-thisvalues (slot-0 capture =TAG_UNDEFINEDsentinel →this_valuereads IMPLICIT_THIS, set byFunction.prototype.call/apply).get_native_module_constantresolvesrequire('stream').prototypeto that same legacy prototype.Wall 3 —
defineProperty(Class.prototype, …)getter boundthisto the prototypewinston's
Object.defineProperty(Logger.prototype, 'transports', { get() { const { pipes } = this._readableState; … } }), read asthis.transportsinside the constructor, threw "Cannot convert undefined or null to object" (logger.js:695). The getter is an ordinary method closure whose body readsthisfrom its captured receiver slot, not IMPLICIT_THIS; the inherited prototype-accessor walk merely set IMPLICIT_THIS and called the closure, so the getter observed the prototype (whose_readableStateis undefined) instead of the instance.Fix: route
inherited_proto_accessor_valuethroughinvoke_accessor_getter, which clones the getter closure withthisrebound to the real receiver (matching the own-accessor read path).Files changed
crates/perry-codegen/src/expr/property_get.rs(Wall 1)crates/perry-runtime/src/object/native_module_stream.rs,crates/perry-runtime/src/node_stream.rs,crates/perry-runtime/src/node_stream_dispatch.rs,crates/perry-runtime/src/object/native_module.rs(Wall 2)crates/perry-runtime/src/object/class_registry.rs(Wall 3)Tests
tests/test_native_module_value_read_install_codegen.sh(Wall 1, runtime + IR assertion)tests/test_stream_prototype_eventemitter_borrow.sh(Wall 2)tests/test_defineproperty_prototype_getter_this.sh(Wall 3)Validation
--no-cacheruns clean (winston object, exit 0) — GREEN.PERRY_NO_AUTO_OPTIMIZE=1,--no-cache, returncode+binary): 59/59 (100%), 0 failing. No regressions inws/rxjs/commander/pino/bluebird/execa(the other stream/ES5-inheritance packages).perry-runtimeunit tests: 1061/1061 pass single-threaded (the 3 parallel failures are pre-existing shared-state flakes).cargo fmt --all --checkclean;check_file_size.sh,gc_store_site_inventory.py,addr_class_inventory.pyall pass.Version / CHANGELOG
Left to the maintainer to fold at merge (per CLAUDE.md / task constraints).
Summary by CodeRabbit
Bug Fixes
thisbinding in stream method calls and prototype getters.require('stream').prototypeto return the correct prototype object.New Features
Tests
thisbinding.