Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions crates/perry-codegen/src/expr/property_get.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,20 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
let prop_idx = ctx.strings.intern(property);
let prop_bytes_global = format!("@{}", ctx.strings.entry(prop_idx).bytes_global);
let prop_len_str = property.len().to_string();
// The value read of a native-module callable export (`const f =
// util.inherits`) mints a BOUND_METHOD closure that, when invoked
// indirectly, dispatches through the per-module `NM_DISPATCH_REGISTRY`
// populated by `js_nm_install_<module>()`. The *direct* call form
// (`util.inherits(a, b)`) is statically lowered to the runtime extern
// and never touches the registry, so a module reached ONLY via this
// value-read path would leave the registry empty and the indirect call
// would resolve to `undefined` (winston/readable-stream's
// `require('inherits')` → `util.inherits` value → `inherits(Sub, Base)`
// silently skipped, breaking the ES5 super-chain). Emit the install
// here so the value-read path's later dispatch finds the module fn.
if let Some(install_sym) = crate::nm_install::nm_install_symbol(module_name) {
ctx.block().call_void(install_sym, &[]);
}
return Ok(ctx.block().call(
DOUBLE,
"js_native_module_property_by_name",
Expand Down
13 changes: 12 additions & 1 deletion crates/perry-runtime/src/node_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,18 @@ fn this_value(closure: *const ClosureHeader) -> f64 {
// host object value cast to i64; reverse the cast.
if !closure.is_null() {
let bits = js_closure_get_capture_ptr(closure, 0) as u64;
return f64::from_bits(bits);
// 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);
}
Comment on lines +151 to +162

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.rs

Repository: 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 -30

Repository: 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.

}
crate::object::js_implicit_this_get()
}
Expand Down
36 changes: 36 additions & 0 deletions crates/perry-runtime/src/node_stream_dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,42 @@ pub(super) fn install_methods_on_existing_object(
}
}

/// Install the EventEmitter prototype methods (`on`/`once`/`emit`/
/// `removeListener`/`removeAllListeners`/…) on a *prototype* object as named
/// own properties, each a closure that reads its receiver from the call-site
/// `this` (IMPLICIT_THIS) rather than a captured instance.
///
/// readable-stream's `Readable.prototype.on` is `function (ev, fn) { var res =
/// Stream.prototype.on.call(this, ev, fn); … }` — the legacy `Stream.prototype`
/// must therefore expose `.on` (and siblings) as VALUES so the `.call(this)`
/// borrow dispatches the EventEmitter `on` against the real stream instance.
/// Before this, `Stream.prototype.on` was `undefined` and the `.call` threw
/// "Function.prototype.call was called on a value that is not a function".
///
/// The closures capture `TAG_UNDEFINED` in slot 0; `this_value` treats that as
/// "no fixed receiver — read IMPLICIT_THIS", which `Function.prototype.call`/
/// `.apply` sets to the borrowed `this`.
pub(crate) fn install_event_emitter_prototype_methods(proto: *mut ObjectHeader) {
register_stub_arities();
let methods = super::emitter_methods();
let mut on_method: Option<f64> = None;
for (name, func) in methods {
if name == "addListener" {
if let Some(val) = on_method {
js_object_set_field_by_name(proto, hidden_key(name.as_bytes()), val);
continue;
}
}
let closure = js_closure_alloc(func as *const u8, 1);
crate::closure::js_closure_set_capture_ptr(closure, 0, crate::value::TAG_UNDEFINED as i64);
let val = f64::from_bits(JSValue::pointer(closure as *const u8).bits());
if name == "on" {
on_method = Some(val);
}
js_object_set_field_by_name(proto, hidden_key(name.as_bytes()), val);
}
}

pub(super) fn register_stub_arities() {
let register = |func: *const u8, arity: u32| {
crate::closure::js_register_closure_arity(func, arity);
Expand Down
22 changes: 14 additions & 8 deletions crates/perry-runtime/src/object/class_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -759,14 +759,20 @@ unsafe fn inherited_proto_accessor_value(
if acc.get == 0 {
return Some(JSValue::undefined());
}
let closure = (acc.get & crate::value::POINTER_MASK) as *const crate::closure::ClosureHeader;
if closure.is_null() {
return Some(JSValue::undefined());
}
let previous_this = js_implicit_this_set(receiver);
let value = crate::closure::js_closure_call0(closure);
js_implicit_this_set(previous_this);
Some(JSValue::from_bits(value.to_bits()))
// Route through `invoke_accessor_getter` rather than a bare
// `js_implicit_this_set` + `js_closure_call0`. 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 from IMPLICIT_THIS — so merely setting IMPLICIT_THIS left the getter
// observing the prototype it lives on instead of the instance (winston's
// `get transports()` saw the prototype, whose `this._readableState` is
// undefined → "Cannot convert undefined or null to object").
// `invoke_accessor_getter` clones the closure with `this` rebound to the
// real receiver (and applies strict/sloppy coercion), matching the
// own-accessor read path.
Some(super::field_get_set::invoke_accessor_getter(
acc.get, receiver,
))
}

unsafe fn resolve_proto_chain_field_inner(
Expand Down
22 changes: 22 additions & 0 deletions crates/perry-runtime/src/object/native_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6445,6 +6445,28 @@ pub(crate) unsafe fn get_native_module_constant(
return cjs_default_export_value("process");
}

// Node's `require('stream')` IS the legacy `Stream` constructor (a function
// that also carries `.Readable`/`.Writable`/… statics), so its `.prototype`
// is the EventEmitter-derived `Stream.prototype`. Perry models the module as
// a namespace OBJECT, so `require('stream').prototype` was `undefined`.
// readable-stream's `Readable.prototype.on = function (ev, fn) { var res =
// Stream.prototype.on.call(this, ev, fn); … }` (where `Stream =
// require('stream')`) then threw "Function.prototype.call was called on a
// value that is not a function". Resolve `require('stream').prototype` to the
// same legacy `Stream.prototype` the `.Stream` export carries (minted +
// cached by `bound_native_callable_export_value("stream", "Stream")`), which
// now exposes the EventEmitter prototype methods.
if module_name == "stream" && property == "prototype" {
let stream_ctor = bound_native_callable_export_value("stream", "Stream");
let ctor_ptr = (stream_ctor.to_bits() & crate::value::POINTER_MASK) as usize;
if ctor_ptr != 0 {
let proto = crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype");
if !JSValue::from_bits(proto.to_bits()).is_undefined() {
return Some(proto);
}
}
}

if property == "default" && !is_cjs_default_object && module_name != "process" {
if let Some(value) = cjs_default_export_value(module_name) {
return Some(value);
Expand Down
9 changes: 9 additions & 0 deletions crates/perry-runtime/src/object/native_module_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ pub(crate) fn attach_stream_legacy_prototype(constructor_value: f64) {
b"constructor\0".len() as u32,
);
js_object_set_field(proto, 0, JSValue::from_bits(constructor_value.to_bits()));
// readable-stream's `Readable.prototype.on` borrows `Stream.prototype.on`
// via `.call(this)`; expose the EventEmitter methods on the legacy
// `Stream.prototype` as receiver-from-`this` values so the borrow works.
crate::node_stream::install_event_emitter_prototype_methods(proto);
let proto_value = crate::value::js_nanbox_pointer(proto as i64);
STREAM_EVENT_EMITTER_PROTOTYPES.with(|protos| {
let mut protos = protos.borrow_mut();
Expand Down Expand Up @@ -79,6 +83,11 @@ pub(crate) fn attach_stream_constructor_prototype(constructor_value: f64, name:
b"constructor\0".len() as u32,
);
js_object_set_field(proto, 0, JSValue::from_bits(constructor_value.to_bits()));
// `Readable`/`Writable`/`Duplex`/`Transform`/`PassThrough` also chain their
// own prototype methods onto `<Ctor>.prototype.<m>.call(this, …)` (e.g.
// `Duplex.prototype.on` ↔ readable-stream borrows). Expose the EventEmitter
// methods on these prototypes too.
crate::node_stream::install_event_emitter_prototype_methods(proto);
let proto_value = crate::value::js_nanbox_pointer(proto as i64);
STREAM_EVENT_EMITTER_PROTOTYPES.with(|protos| {
let mut protos = protos.borrow_mut();
Expand Down
106 changes: 106 additions & 0 deletions tests/test_defineproperty_prototype_getter_this.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
#!/usr/bin/env bash
# Regression: a getter installed via `Object.defineProperty(Class.prototype,
# name, { get })` must run with `this` bound to the INSTANCE, not the prototype
# object it lives on.
#
# Such a getter is an ordinary method closure whose body reads `this` from its
# captured receiver slot (not IMPLICIT_THIS). The inherited-accessor walk used
# to merely set IMPLICIT_THIS and call the closure, so the getter observed the
# prototype — `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".
#
# Fix: the inherited prototype-accessor path routes through
# `invoke_accessor_getter`, which clones the getter closure with `this` rebound
# to the real receiver (matching the own-accessor read path).
set -euo pipefail

SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
PERRY="${PERRY_BIN:-${PERRY:-$REPO_ROOT/target/release/perry}}"

if [[ ! -x "$PERRY" ]]; then
PERRY="$REPO_ROOT/target/debug/perry"
fi
if [[ ! -x "$PERRY" ]]; then
echo "SKIP: perry binary not found (build with cargo build -p perry)"
exit 0
fi

TMPDIR="$(mktemp -d)"
trap 'rm -rf "$TMPDIR"' EXIT

SRC="$TMPDIR/defineproperty_proto_getter.ts"
BIN="$TMPDIR/defineproperty_proto_getter"

cat >"$SRC" <<'TS'
let failures = 0;

// Plain class: getter reads an own data field through `this`.
class A {
_x = 42;
}
Object.defineProperty(A.prototype, "px", {
get() { return (this as any)._x; },
});
const a: any = new A();
if (a.px !== 42) {
console.log("FAIL: defineProperty prototype getter read prototype, not instance (px=" + a.px + ")");
failures = failures + 1;
}

// Getter that DESTRUCTURES a nested own object — the exact winston shape.
class L {
_readableState: any;
constructor() { this._readableState = { pipes: null }; }
}
Object.defineProperty(L.prototype, "transports", {
configurable: false,
enumerable: true,
get() {
const { pipes } = (this as any)._readableState;
return !Array.isArray(pipes) ? [pipes].filter(Boolean) : pipes;
},
});
const l: any = new L();
let threw = false;
let result: any = "unset";
try {
result = l.transports;
} catch (e) {
threw = true;
}
if (threw) {
console.log("FAIL: destructuring getter threw (this bound to prototype, _readableState undefined)");
failures = failures + 1;
} else if (JSON.stringify(result) !== "[]") {
console.log("FAIL: destructuring getter wrong result: " + JSON.stringify(result));
failures = failures + 1;
}

if (failures !== 0) {
throw new Error("defineProperty prototype getter this-binding regression failed");
}
console.log("defineProperty proto getter this ok");
TS

"$PERRY" compile --no-cache --no-auto-optimize "$SRC" -o "$BIN" >"$TMPDIR/compile.log" 2>&1 || {
echo "FAIL: compile failed"
sed 's/^/ /' "$TMPDIR/compile.log" | tail -80
exit 1
}

"$BIN" >"$TMPDIR/run.log" 2>&1 || {
echo "FAIL: program failed"
sed 's/^/ /' "$TMPDIR/run.log" | tail -80
exit 1
}

if ! grep -q "defineProperty proto getter this ok" "$TMPDIR/run.log"; then
echo "FAIL: expected success marker"
sed 's/^/ /' "$TMPDIR/run.log" | tail -80
exit 1
fi

echo "PASS: defineProperty prototype getter this-binding"
118 changes: 118 additions & 0 deletions tests/test_native_module_value_read_install_codegen.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
#!/usr/bin/env bash
# Regression: reading a native-module callable export AS A VALUE
# (`const f = util.inherits`) and invoking it indirectly must dispatch to the
# real runtime impl. The indirect call resolves through the per-module
# NM_DISPATCH_REGISTRY, which is populated by `js_nm_install_<module>()`. The
# *direct* call form (`util.inherits(a, b)`) is statically lowered straight to
# the runtime extern and never touches that registry, so a module reached ONLY
# through the value-read path used to leave the registry empty — the indirect
# call silently resolved to `undefined`.
#
# Concretely: winston's `class Logger extends Transform` (readable-stream)
# relies on `require('inherits')(Transform, Duplex)` — an indirect
# `util.inherits` value-call — to wire the ES5 super-chain so the nested
# `Readable.call(this)` `if (!(this instanceof Readable))` guard takes the
# in-place branch and sets `this._readableState`. With the install skipped,
# the guard saw `false`, returned a discarded `new Readable()`, and
# `this._readableState.needReadable = true` threw on `null`.
#
# This test pins both halves:
# 1. RUNTIME: the value-read `inherits(Sub, Base)` registers the ES5 parent
# edge so `new Sub() instanceof Base` is true and base ctor writes persist.
# 2. CODEGEN: the `PropertyGet { NativeModuleRef("util"), "inherits" }`
# value-read emits `call void @js_nm_install_util()`.
set -euo pipefail

SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
PERRY="${PERRY_BIN:-${PERRY:-$REPO_ROOT/target/release/perry}}"

if [[ ! -x "$PERRY" ]]; then
PERRY="$REPO_ROOT/target/debug/perry"
fi
if [[ ! -x "$PERRY" ]]; then
echo "SKIP: perry binary not found (build with cargo build -p perry)"
exit 0
fi

TMPDIR="$(mktemp -d)"
trap 'rm -rf "$TMPDIR"' EXIT

SRC="$TMPDIR/nm_value_read_install.ts"
BIN="$TMPDIR/nm_value_read_install"
OBJ="$TMPDIR/nm_value_read_install.o"

cat >"$SRC" <<'TS'
import * as util from "util";

// Read the native-module callable export AS A VALUE, then invoke indirectly.
const inh: any = (util as any).inherits;

function Base(this: any) {
if (!(this instanceof Base)) return new (Base as any)();
this._state = { x: 1 };
}
function Sub(this: any) {
(Base as any).call(this);
this._state.y = 2; // mirrors readable-stream's post-super `this._x` writes
}
inh(Sub, Base);

const s: any = new (Sub as any)();

let failures = 0;
if (!(s instanceof Base)) {
console.log("FAIL: value-read util.inherits did not register the ES5 parent edge");
failures = failures + 1;
}
if (!s._state || s._state.x !== 1 || s._state.y !== 2) {
console.log("FAIL: base ctor writes did not persist through the super-chain");
failures = failures + 1;
}

if (failures !== 0) {
throw new Error("native-module value-read inherits regression failed");
}
console.log("nm value-read inherits ok");
TS

"$PERRY" compile --no-cache --no-auto-optimize "$SRC" -o "$BIN" >"$TMPDIR/compile.log" 2>&1 || {
echo "FAIL: compile failed"
sed 's/^/ /' "$TMPDIR/compile.log" | tail -80
exit 1
}

"$BIN" >"$TMPDIR/run.log" 2>&1 || {
echo "FAIL: program failed"
sed 's/^/ /' "$TMPDIR/run.log" | tail -80
exit 1
}

if ! grep -q "nm value-read inherits ok" "$TMPDIR/run.log"; then
echo "FAIL: expected success marker"
sed 's/^/ /' "$TMPDIR/run.log" | tail -80
exit 1
fi

(
cd "$TMPDIR"
"$PERRY" compile --no-cache --no-auto-optimize --trace llvm --no-link \
"$SRC" -o "$OBJ" >"$TMPDIR/trace-compile.log" 2>&1
) || {
echo "FAIL: trace compile failed"
sed 's/^/ /' "$TMPDIR/trace-compile.log" | tail -80
exit 1
}

TRACE_DIR="$TMPDIR/.perry-trace/llvm"
if [[ ! -d "$TRACE_DIR" ]]; then
echo "FAIL: LLVM trace directory not found"
exit 1
fi

if ! grep -R "call void @js_nm_install_util()" "$TRACE_DIR" >/dev/null; then
echo "FAIL: expected js_nm_install_util() call on the native-module value-read path"
exit 1
fi

echo "PASS: native-module value-read install codegen"
Loading
Loading