diff --git a/crates/perry-runtime/src/node_stream_dispatch.rs b/crates/perry-runtime/src/node_stream_dispatch.rs index bd1cedd2a..0545d7c29 100644 --- a/crates/perry-runtime/src/node_stream_dispatch.rs +++ b/crates/perry-runtime/src/node_stream_dispatch.rs @@ -80,6 +80,75 @@ pub(super) fn build_object(methods: &[(&str, StubFn)], shape_id: u32) -> *mut Ob obj } +/// #6316 — reserved own-key prefix for a native base method DISPLACED by a +/// subclass override. +/// +/// The native bases perry models by stamping their method surface onto the +/// instance (`EventEmitter`, every `node:stream` class) install those methods as +/// ORDINARY OWN PROPERTIES. Own properties legitimately shadow class methods, so +/// perry's own-property-override probe (issue #620, +/// `perry-codegen/src/lower_call/method_override.rs`) selected the native +/// closure in preference to the user's `class Bus extends EventEmitter { emit() +/// {…} }` override — inheritance ran BACKWARDS and the override never executed. +/// +/// The fix installs a base method under its plain name only when the receiver's +/// class chain does NOT declare it. When it does, the native closure is stashed +/// under `__perry_native_super__` instead: invisible to ordinary property +/// lookup (so the class method wins), but still reachable from +/// `js_super_method_call_dynamic` so `super.emit(…)` lands on the real base +/// implementation. Hidden from own-key enumeration by +/// `object::field_get_set::enumeration::is_internal_runtime_key_bytes`. +pub(crate) const NATIVE_BASE_SUPER_PREFIX: &[u8] = b"__perry_native_super__"; + +/// `__perry_native_super__` as an interned string key. +fn native_base_super_key(name: &str) -> *mut crate::string::StringHeader { + let mut buf = Vec::with_capacity(NATIVE_BASE_SUPER_PREFIX.len() + name.len()); + buf.extend_from_slice(NATIVE_BASE_SUPER_PREFIX); + buf.extend_from_slice(name.as_bytes()); + crate::string::js_string_from_bytes(buf.as_ptr(), buf.len() as u32) +} + +/// The native base method that a subclass override displaced, if any (#6316). +/// `js_super_method_call_dynamic` calls this after the class-vtable and +/// prototype-method lookups on the parent chain have both missed — which is +/// always, for a native base, since `EventEmitter`/`Readable`/… are not perry +/// classes and own no registry entry to resolve against. +/// +/// Returns `None` for a receiver that is not an object, or one carrying no +/// displaced method of that name, so an ordinary `super.m()` miss still yields +/// `undefined` (the #774 instance-field-shadow contract). +pub(crate) fn displaced_native_base_method(this_value: f64, name: &str) -> Option { + unsafe { + let jsval = JSValue::from_bits(this_value.to_bits()); + if !jsval.is_pointer() { + return None; + } + let raw = (this_value.to_bits() & crate::value::POINTER_MASK) as usize; + if raw == 0 || crate::value::addr_class::is_small_handle(raw) { + return None; + } + let header = crate::value::addr_class::try_read_gc_header(raw)?; + if header.obj_type != crate::gc::GC_TYPE_OBJECT { + return None; + } + let obj = raw as *const ObjectHeader; + let val = js_object_get_field_by_name_f64(obj, native_base_super_key(name)); + if JSValue::from_bits(val.to_bits()).is_pointer() { + Some(val) + } else { + None + } + } +} + +/// True when the receiver's class chain declares `name` as a real class method — +/// i.e. the user OVERRODE this native base method (#6316). The class registry is +/// populated at module init, long before any `new`, so the vtable is always +/// live by the time a constructor runs `super()`. +fn class_chain_overrides(class_id: u32, name: &str) -> bool { + class_id != 0 && crate::object::method_owner_class_id(class_id, name).is_some() +} + pub(super) fn install_methods_on_existing_object( obj: *mut ObjectHeader, this_value: f64, @@ -87,25 +156,60 @@ pub(super) fn install_methods_on_existing_object( skip_names: &[&str], ) { register_stub_arities(); - let this_bits = this_value.to_bits(); + // `js_closure_alloc` and the key interning below both allocate and can + // therefore GC-move the receiver, so root it and re-read the raw pointer at + // every use rather than trusting the `obj` snapshot across the loop. The + // NaN-boxed `this` goes in a handle for the same reason: it is copied into + // each closure's capture slot, and a stale value there would leave the + // method bound to a dead receiver. (Captures already stored are traced and + // rewritten by the GC; a bit-pattern parked in a Rust local is not.) + let scope = crate::gc::RuntimeHandleScope::new(); + let obj_handle = scope.root_raw_mut_ptr(obj); + let this_handle = scope.root_nanbox_f64(this_value); + let class_id = crate::object::js_object_get_class_id(obj); + let mut on_method: Option = None; for (name, func) in methods { if skip_names.iter().any(|skip| skip == name) { continue; } + // #6316: the subclass declares this method — the native base version + // must NOT become an own property, or it would shadow the override. + // Stash it where only `super.()` can find it. + let overridden = class_chain_overrides(class_id, name); + + // `addListener` is an ALIAS of `EventEmitter.prototype.on` in Node, so + // it reuses the base `on` closure even when the subclass overrides + // `on` — `emitter.addListener(…)` must reach the base, not the override. if *name == "addListener" { if let Some(val) = on_method { - js_object_set_field_by_name(obj, hidden_key(name.as_bytes()), val); + let key = native_or_plain_key(name, overridden); + js_object_set_field_by_name(obj_handle.get_raw_mut_ptr::(), key, val); continue; } } let closure = js_closure_alloc(*func as *const u8, 1); - crate::closure::js_closure_set_capture_ptr(closure, 0, this_bits as i64); + crate::closure::js_closure_set_capture_ptr( + closure, + 0, + this_handle.get_nanbox_f64().to_bits() 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(obj, hidden_key(name.as_bytes()), val); + let key = native_or_plain_key(name, overridden); + js_object_set_field_by_name(obj_handle.get_raw_mut_ptr::(), key, val); + } +} + +/// The key an installed base method lands on: its plain name normally, or the +/// reserved super-only key when the subclass overrides it (#6316). +fn native_or_plain_key(name: &str, overridden: bool) -> *mut crate::string::StringHeader { + if overridden { + native_base_super_key(name) + } else { + hidden_key(name.as_bytes()) } } diff --git a/crates/perry-runtime/src/object/class_constructors.rs b/crates/perry-runtime/src/object/class_constructors.rs index 26b431bb0..08654264c 100644 --- a/crates/perry-runtime/src/object/class_constructors.rs +++ b/crates/perry-runtime/src/object/class_constructors.rs @@ -584,7 +584,12 @@ pub unsafe extern "C" fn js_super_method_call_dynamic( }; let parent_cid = match crate::object::get_parent_class_id(child_class_id) { Some(p) if p != 0 => p, - _ => return undef, + // #6316: `class Bus extends EventEmitter` has NO registered parent — + // the native base is not a perry class, so nothing was ever wired into + // CLASS_REGISTRY. Before this, `super.emit(…)` fell off here and + // silently returned `undefined`. The base method the subclass override + // displaced is the correct target. + _ => return call_displaced_native_base_method(this_value, name, args_ptr, args_len, undef), }; // Static-context super call (`super.m()` inside a `static` method): the // receiver is the class constructor (a ClassRef), so resolve the PARENT's @@ -660,7 +665,40 @@ pub unsafe extern "C" fn js_super_method_call_dynamic( super::IMPLICIT_THIS.with(|c| c.set(prev_this)); return result; } - undef + // #6316: the parent chain is real (an intermediate user class) but bottoms + // out in a NATIVE base whose surface perry stamps onto the instance — + // `class Logged extends Bus`, `class Bus extends EventEmitter`. Neither the + // vtable nor the prototype registry knows `emit`; the displaced base method + // does. Runs LAST so a genuine user-class parent method always wins. + call_displaced_native_base_method(this_value, name, args_ptr, args_len, undef) +} + +/// Invoke the native base method a subclass override displaced (#6316), with +/// `this` bound to the receiver. Falls back to `undef` when the receiver carries +/// no such method — an ordinary `super.m()` miss stays `undefined`. +/// +/// # Safety +/// `args_ptr` must point to `args_len` valid `f64`s (or be null when +/// `args_len == 0`). +unsafe fn call_displaced_native_base_method( + this_value: f64, + name: &str, + args_ptr: *const f64, + args_len: usize, + undef: f64, +) -> f64 { + let Some(method_value) = crate::node_stream::displaced_native_base_method(this_value, name) + else { + return undef; + }; + // The stashed closure already captures the receiver in slot 0, but bind + // IMPLICIT_THIS too: the shared emitter/stream stubs read their receiver + // through `this_value(closure)`, which falls back to IMPLICIT_THIS when the + // capture is undefined (the prototype-installed form). + let prev_this = super::IMPLICIT_THIS.with(|c| c.replace(this_value.to_bits())); + let result = crate::closure::js_native_call_value(method_value, args_ptr, args_len); + super::IMPLICIT_THIS.with(|c| c.set(prev_this)); + result } /// Keepalive anchor (generated-code-only callee). diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index ad6b08cea..dcf385309 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -1003,9 +1003,19 @@ pub(crate) unsafe fn instance_private_key_hidden( /// Matches each key EXACTLY (an allowlist), not a broad `__perry_*` prefix — a /// prefix test would wrongly hide legitimate user properties whose name happens /// to begin with `__perry_` (e.g. `this.__perry_user = 1`). +/// +/// The one prefix family is `__perry_native_super__` (#6316): the native +/// base method a subclass override displaced. Its key set is parameterized by +/// method name, so an exact allowlist cannot enumerate it. The prefix is a +/// reserved, runtime-only spelling — narrow enough not to be a plausible user +/// property, unlike a blanket `__perry_*` test. Hiding it also moves enumeration +/// TOWARD Node: the displaced method previously sat on the instance under its +/// plain name (`emit`), which `Object.keys` wrongly reported as an own key. #[inline] pub(crate) fn is_internal_runtime_key_bytes(b: &[u8]) -> bool { - b == crate::object::map_set_subclass::BACKING_KEY || b == crate::weakref::WEAK_ENTRIES_KEY + b == crate::object::map_set_subclass::BACKING_KEY + || b == crate::weakref::WEAK_ENTRIES_KEY + || b.starts_with(crate::node_stream::NATIVE_BASE_SUPER_PREFIX) } /// `&str` form of [`is_internal_runtime_key_bytes`]. diff --git a/test-files/test_gap_6316_native_base_override.ts b/test-files/test_gap_6316_native_base_override.ts new file mode 100644 index 000000000..b68449603 --- /dev/null +++ b/test-files/test_gap_6316_native_base_override.ts @@ -0,0 +1,114 @@ +// #6316: a subclass method that OVERRIDES a native base-class method must run, +// and `super.()` from inside it must reach the native base. +// +// perry models EventEmitter and the node:stream classes by stamping the base's +// method surface onto the INSTANCE as ordinary own properties. Own properties +// legitimately shadow class methods, so perry's own-property-override probe +// (#620) picked the native closure over the user's override: inheritance ran +// backwards and the override never executed — silently, with no error. The +// `super.()` path into such a base was independently dead: a native base is +// not a perry class, so the subclass has no registered parent edge to resolve +// against and `super.emit(...)` returned undefined. + +import { EventEmitter } from "node:events"; +import { Readable, Writable } from "node:stream"; + +// ── EventEmitter: override + super, the issue's exact repro ── +class Bus extends EventEmitter { + emit(ev: string, ...args: any[]): boolean { + console.log("Bus.emit override ran:", ev); + return super.emit(ev, ...args); + } +} +const bus = new Bus(); +bus.on("ping", (x: number) => console.log("bus listener got:", x)); +console.log("bus emit returned:", bus.emit("ping", 42)); + +// ── several overrides at once, each super-calling the base ── +class Chatty extends EventEmitter { + events: string[] = []; + on(ev: string, fn: any): this { + console.log("Chatty.on override ran:", ev); + this.events.push(ev); + return super.on(ev, fn); + } + emit(ev: string, ...args: any[]): boolean { + console.log("Chatty.emit override ran:", ev); + return super.emit(ev, ...args); + } +} +const chatty = new Chatty(); +chatty.on("a", (v: string) => console.log("chatty listener got:", v)); +console.log("chatty emit returned:", chatty.emit("a", "hello")); +console.log("chatty registered:", chatty.events.join(",")); + +// ── a NON-overridden method on an overriding subclass still reaches the base ── +console.log("chatty listenerCount:", chatty.listenerCount("a")); +chatty.removeAllListeners("a"); +console.log("chatty after removeAll:", chatty.listenerCount("a")); + +// ── no-override control: plain subclass keeps working ── +class Plain extends EventEmitter {} +const plain = new Plain(); +plain.on("go", (v: number) => console.log("plain listener got:", v)); +console.log("plain emit returned:", plain.emit("go", 7)); + +// ── an explicit constructor + state: the `super()` install path ── +// (`class X extends Base {}` where Base is itself the EventEmitter subclass is +// NOT covered here — an INDIRECT native base never runs the subclass init at +// all, override or not. That is a separate pre-existing gap, not this issue.) +class Counter extends EventEmitter { + seen: number; + constructor(start: number) { + super(); + this.seen = start; + } + emit(ev: string, ...args: any[]): boolean { + this.seen++; + console.log("Counter.emit override ran:", ev, "seen:", this.seen); + return super.emit(ev, ...args); + } +} +const counter = new Counter(0); +counter.on("tick", () => console.log("counter listener fired")); +console.log("counter emit returned:", counter.emit("tick")); +console.log("counter emit returned:", counter.emit("tick")); + +// ── the displaced base method must not leak as an own key ── +const keys = Object.keys(bus); +console.log("bus own 'emit' key present:", keys.includes("emit")); + +// ── node:stream — same install-on-instance mechanism as EventEmitter ── +class CountingReadable extends Readable { + pushes = 0; + push(chunk: any, enc?: any): boolean { + this.pushes++; + console.log("CountingReadable.push override ran"); + return super.push(chunk, enc); + } + _read(): void { + this.push(null); + } +} +const rs = new CountingReadable(); +rs.on("end", () => { + console.log("readable end, pushes:", rs.pushes); + + // ── Writable: override `write`, forward to the base ── + class LoggingWritable extends Writable { + written: string[] = []; + write(chunk: any, enc?: any, cb?: any): boolean { + console.log("LoggingWritable.write override ran"); + this.written.push(String(chunk)); + return super.write(chunk, enc, cb); + } + _write(_chunk: any, _enc: any, cb: any): void { + cb(); + } + } + const ws = new LoggingWritable(); + const ok = ws.write("payload"); + console.log("writable write returned:", ok); + console.log("writable captured:", ws.written.join(",")); +}); +rs.resume();