fix(runtime): a subclass override of a native base method must win over the native surface (#6316)

crates/perry-runtime/src/node_stream_dispatch.rs

@@ -80,32 +80,136 @@ 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__<name>` 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__<name>` 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<f64> {
+    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,
     methods: &[(&str, StubFn)],
     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<f64> = 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.<name>()` 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::<ObjectHeader>(), 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::<ObjectHeader>(), 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())
     }
 }
 

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).

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__<method>` (#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`].

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.<method>()` 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.<m>()` 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();