fix(codegen): don't scalar-replace an instance whose chain reaches an unmodeled base (#6343) (#6357)

Escape analysis promoted a non-escaping `new C()` to one alloca per DECLARED
field and inlined the constructor stores. That model is only faithful when
codegen can see the whole construction — and a native base cannot be seen.

`EventEmitter` and the `node:stream` classes install their method surface as
OWN PROPERTIES on the instance at subclass-init time
(`js_object_set_field_by_name(obj, "emit", <native closure>)`), not on a
prototype. A runtime-installed own property has no declared slot, so it was
simply absent from the promoted set:

    class X extends EventEmitter { a = 1 }
    const x = new X();
    x.a        // 1          (declared field — promoted)
    x.emit     // undefined  (installed own property — no slot)

Silent wrong answer, no error. A method call on the receiver forces the heap
path and hides it, so the shape that bites is a bare property read next to a
field — exactly what `typeof x.emit` does.

`class_chain_has_unmodeled_base` walks the `extends` chain and disqualifies a
candidate when the chain reaches a base whose construction is opaque:

  * `native_extends` — events / node:stream / Web Streams / async_hooks / ws;
    found by walking the CHAIN, so `class Leaf extends Mid extends
    EventEmitter` is caught too, not just a literal `extends EventEmitter`;
  * `extends <expr>` (incl. a lexically shadowed heritage name) — an arbitrary
    runtime parent whose constructor can install anything;
  * a parent name that resolves to no visible class — a builtin (`Error`,
    `Map`, `Set`, …) or an import whose stub never landed.

A cyclic chain fails closed. This is the third member of the family that
already holds #313 (`this`-as-value), #573 (builtin `Error`) and #5872
(dispatch stability), and it is precise for the same reason they are: a chain
of ordinary user classes is fully modeled, so the plain non-escaping class
keeps its scalar replacement and the #945 IR guard's zero-alloc / zero-dispatch
fast path is untouched.

Fixture `test_gap_6343_scalar_native_base_surface.ts` covers EventEmitter,
`Readable`, `Writable`, an indirect chain, and an Error subclass, with
controls for a plain class and a pure user-class chain.

Co-authored-by: Ralph Küpper <ralph3@skelpo.com>

Author: proggeramlug

crates/perry-codegen/src/collectors/escape_news.rs

@@ -49,6 +49,22 @@ pub fn collect_non_escaping_news(
             else if class_chain_extends_builtin_error(class, classes) {
                 escaped.insert(*id);
             }
+            // Issue #6343: the class chain reaches a base whose construction
+            // codegen cannot see — a NATIVE base that stamps its method
+            // surface onto the instance as own properties (`extends
+            // EventEmitter`, `extends Readable`, …), a dynamic `extends
+            // <expr>` parent, or a parent name that resolves to no visible
+            // class. Scalar replacement promotes only the DECLARED fields of
+            // the chain, so a runtime-installed own property has no slot in
+            // the promoted set: `class X extends EventEmitter { a = 1 }` read
+            // `x.a` correctly but `typeof x.emit` as `undefined`. The heap
+            // path runs the real subclass-init and looks the property up on
+            // the object. Same family as the #573 check above and the #5872
+            // dispatch-stability pass below — a chain of ordinary user classes
+            // is fully modeled and stays scalar-replaced.
+            else if class_chain_has_unmodeled_base(class, classes) {
+                escaped.insert(*id);
+            }
         }
     }
 

crates/perry-codegen/src/collectors/mod.rs

@@ -83,4 +83,6 @@ pub(crate) use scalar_methods::simple_scalar_method_summary;
 pub(crate) use shadow_slots::{
     collect_declared_shadow_slots_in_stmts, collect_shadow_slot_clear_points,
 };
-pub(crate) use this_as_value::{class_chain_extends_builtin_error, class_uses_this_as_value};
+pub(crate) use this_as_value::{
+    class_chain_extends_builtin_error, class_chain_has_unmodeled_base, class_uses_this_as_value,
+};

crates/perry-codegen/src/collectors/this_as_value.rs

@@ -128,6 +128,79 @@ pub fn class_chain_extends_builtin_error(
     false
 }
 
+/// Issue #6343: walk the class's `extends` chain and report whether it reaches
+/// a base whose *construction* codegen cannot see.
+///
+/// Scalar replacement models an instance as exactly the set of DECLARED fields
+/// on its chain — one alloca per field name — and inlines the constructor
+/// bodies that fill them. That is only faithful when the whole chain is
+/// visible. A base that isn't contributes instance state the promoted set does
+/// not model, and every way that happens is silent:
+///
+///   * a **native** base — `class X extends EventEmitter`, `extends Readable`,
+///     … — installs its method surface as OWN PROPERTIES on the instance at
+///     subclass-init time (`js_object_set_field_by_name(obj, "emit", <native
+///     closure>)`) rather than on a prototype. `emit` has no declared slot, so
+///     the promoted set has no slot for it and `x.emit` reads back
+///     `undefined` (#6343: `class X extends EventEmitter { a = 1 }` printed
+///     `typeof x.emit === "undefined"` while `x.a` was correct).
+///   * a **dynamic** base — `extends <expr>`, including a lexically shadowed
+///     heritage name — runs an arbitrary constructor that can install
+///     anything.
+///   * a parent NAME that resolves to no visible class: a builtin (`Error`,
+///     `Map`, `Set`, `Event`, …) or a base whose declaration never reached
+///     this module. Its construction happens in the runtime, not in code
+///     codegen can inline.
+///
+/// The only sound answer for all three is to keep the instance on the heap,
+/// where the real init runs and property lookup goes through the object.
+///
+/// This is deliberately a chain property, not a name test: it must hop user
+/// classes (`class Leaf extends Mid`, `class Mid extends EventEmitter`) and it
+/// must NOT fire for a chain that bottoms out in an ordinary user class — that
+/// instance is fully modeled and keeping it scalar-replaced is a real win
+/// (guarded by `scripts/run_issue_945_scalar_method_ir_guard.sh`).
+///
+/// Generalizes [`class_chain_extends_builtin_error`] (#573), which stays as-is
+/// because it is name-keyed and therefore also fires for a *locally shadowed*
+/// `Error` — this walk resolves such a shadow to the user class and would let
+/// it through.
+pub fn class_chain_has_unmodeled_base(
+    class: &perry_hir::Class,
+    classes: &std::collections::HashMap<String, &perry_hir::Class>,
+) -> bool {
+    let mut current = class;
+    let mut seen: HashSet<String> = HashSet::new();
+    loop {
+        // A cycle (or a chain deep enough to look like one) means the walk
+        // can't prove anything. Fail closed: keep the instance on the heap.
+        if !seen.insert(current.name.clone()) || seen.len() > 64 {
+            return true;
+        }
+        // `native_extends` is the (module, class) tag for a base whose
+        // subclass-init shim stamps a surface onto `this` at construction —
+        // events, node:stream, the Web Streams bases, async_hooks, ws.
+        if current.native_extends.is_some() {
+            return true;
+        }
+        // `class X extends <expr>` — the parent is a runtime value; its
+        // constructor is opaque to this analysis.
+        if current.extends_expr.is_some() || current.heritage_lexically_shadowed {
+            return true;
+        }
+        let Some(parent_name) = current.extends_name.as_deref() else {
+            // Chain bottoms out in a root user class: fully modeled.
+            return false;
+        };
+        match classes.get(parent_name) {
+            Some(parent) => current = parent,
+            // A parent name with no visible class behind it — a builtin, or an
+            // import whose stub never landed. Unmodeled either way.
+            None => return true,
+        }
+    }
+}
+
 pub fn stmts_use_this_as_value(stmts: &[perry_hir::Stmt], fields: &HashSet<String>) -> bool {
     use perry_hir::Stmt;
     for s in stmts {
@@ -498,4 +571,92 @@ mod tests {
 
         assert!(!class_chain_extends_builtin_error(&child, &classes));
     }
+
+    // ── #6343: unmodeled-base chain walk ──
+
+    /// A root user class with no heritage is fully modeled — scalar
+    /// replacement must stay available (this is the #945 fast path).
+    #[test]
+    fn unmodeled_base_allows_plain_class() {
+        let plain = class("Plain", None);
+        let mut classes = HashMap::new();
+        classes.insert(plain.name.clone(), &plain);
+
+        assert!(!class_chain_has_unmodeled_base(&plain, &classes));
+    }
+
+    /// A chain of ordinary user classes is fully modeled too.
+    #[test]
+    fn unmodeled_base_allows_user_class_chain() {
+        let base = class("Base", None);
+        let mid = class("Mid", Some("Base"));
+        let leaf = class("Leaf", Some("Mid"));
+        let mut classes = HashMap::new();
+        classes.insert(base.name.clone(), &base);
+        classes.insert(mid.name.clone(), &mid);
+        classes.insert(leaf.name.clone(), &leaf);
+
+        assert!(!class_chain_has_unmodeled_base(&leaf, &classes));
+    }
+
+    /// `class X extends EventEmitter` — the native base installs its surface as
+    /// own properties on the instance, so the instance must stay on the heap.
+    #[test]
+    fn unmodeled_base_rejects_direct_native_parent() {
+        let mut child = class("X", Some("EventEmitter"));
+        child.native_extends = Some(("events".to_string(), "EventEmitter".to_string()));
+        let mut classes = HashMap::new();
+        classes.insert(child.name.clone(), &child);
+
+        assert!(class_chain_has_unmodeled_base(&child, &classes));
+    }
+
+    /// The native base is found by walking the CHAIN, not by matching the
+    /// leaf's own `extends` name: `class Leaf extends Mid`, `class Mid extends
+    /// EventEmitter`.
+    #[test]
+    fn unmodeled_base_rejects_indirect_native_parent() {
+        let mut mid = class("Mid", Some("EventEmitter"));
+        mid.native_extends = Some(("events".to_string(), "EventEmitter".to_string()));
+        let leaf = class("Leaf", Some("Mid"));
+        let mut classes = HashMap::new();
+        classes.insert(mid.name.clone(), &mid);
+        classes.insert(leaf.name.clone(), &leaf);
+
+        assert!(class_chain_has_unmodeled_base(&leaf, &classes));
+    }
+
+    /// A parent name with no class behind it (a builtin such as `Error` /
+    /// `Map`, or an import whose stub never landed) is unmodeled.
+    #[test]
+    fn unmodeled_base_rejects_unresolvable_parent_name() {
+        let child = class("MyError", Some("Error"));
+        let mut classes = HashMap::new();
+        classes.insert(child.name.clone(), &child);
+
+        assert!(class_chain_has_unmodeled_base(&child, &classes));
+    }
+
+    /// `class X extends <expr>` — an arbitrary runtime parent value.
+    #[test]
+    fn unmodeled_base_rejects_dynamic_parent_expr() {
+        let mut child = class("X", None);
+        child.extends_expr = Some(Box::new(Expr::LocalGet(0)));
+        let mut classes = HashMap::new();
+        classes.insert(child.name.clone(), &child);
+
+        assert!(class_chain_has_unmodeled_base(&child, &classes));
+    }
+
+    /// A cyclic chain proves nothing, so it must fail closed (escape).
+    #[test]
+    fn unmodeled_base_fails_closed_on_cyclic_parent_chain() {
+        let child = class("A", Some("B"));
+        let parent = class("B", Some("A"));
+        let mut classes = HashMap::new();
+        classes.insert(child.name.clone(), &child);
+        classes.insert(parent.name.clone(), &parent);
+
+        assert!(class_chain_has_unmodeled_base(&child, &classes));
+    }
 }

test-files/test_gap_6343_scalar_native_base_surface.ts

@@ -0,0 +1,171 @@
+// #6343: escape analysis must not scalar-replace an instance whose class chain
+// reaches a base that codegen cannot fully model.
+//
+// perry's native bases (`EventEmitter`, the `node:stream` classes, …) install
+// their method surface as OWN PROPERTIES on the instance at subclass-init time
+// — `js_object_set_field_by_name(obj, "emit", <native closure>)` — not on a
+// prototype. Scalar replacement promotes a non-escaping instance to one alloca
+// per DECLARED field, so a runtime-installed own property has no slot in the
+// promoted set and reads back `undefined`.
+//
+// The trigger is that the instance never ESCAPES: a method call (`x.on(…)`)
+// forces the heap path and hides the bug, so every probe below deliberately
+// only *reads* properties off its receiver. A declared field alongside the
+// read is what makes the instance look worth promoting.
+//
+// The controls are the other half: a class with no unmodeled base must STILL
+// scalar-replace — that is a real perf win (see
+// scripts/run_issue_945_scalar_method_ir_guard.sh), so the disqualifier has to
+// be precise rather than a blanket disable.
+
+import { EventEmitter } from "node:events";
+import { Readable, Writable } from "node:stream";
+
+// ── the issue's exact repro: declared field + bare reads, no method call ──
+class X extends EventEmitter {
+  a = 1;
+}
+function probeX(): string {
+  const x = new X();
+  x.a = 2;
+  return `${x.a} ${typeof x.emit} ${typeof x.on}`;
+}
+console.log("X:", probeX());
+
+// same shape at module scope (the local is not referenced from any function,
+// so it stays a plain init-time local — still a promotion candidate)
+const moduleX = new X();
+console.log("moduleX:", moduleX.a, typeof moduleX.emit, typeof moduleX.on);
+
+// ── no declared field at all: the promoted set is simply empty ──
+class NoField extends EventEmitter {}
+function probeNoField(): string {
+  const n = new NoField();
+  return `${typeof n.emit} ${typeof n.on} ${typeof n.once}`;
+}
+console.log("NoField:", probeNoField());
+
+// ── several fields, a post-construction write, and more of the surface ──
+class Multi extends EventEmitter {
+  count = 0;
+  label = "start";
+  flag = true;
+}
+function probeMulti(): string {
+  const m = new Multi();
+  m.count = 5;
+  m.label = "changed";
+  return `${m.count} ${m.label} ${m.flag} ${typeof m.once} ${typeof m.removeAllListeners} ${typeof m.listenerCount}`;
+}
+console.log("Multi:", probeMulti());
+
+// ── node:stream bases install the same way ──
+class R extends Readable {
+  pushes = 0;
+}
+function probeR(): string {
+  const r = new R();
+  r.pushes = 3;
+  return `${r.pushes} ${typeof r.on} ${typeof r.push} ${typeof r.pipe}`;
+}
+console.log("R:", probeR());
+
+class W extends Writable {
+  written = 0;
+}
+function probeW(): string {
+  const w = new W();
+  w.written = 4;
+  return `${w.written} ${typeof w.on} ${typeof w.write} ${typeof w.end}`;
+}
+console.log("W:", probeW());
+
+// ── the surface must be LIVE, not merely present ──
+class Bus extends EventEmitter {
+  seen = 0;
+}
+const bus = new Bus();
+bus.on("ping", (v: number) => {
+  bus.seen += v;
+  console.log("bus listener got:", v);
+});
+console.log("bus emit:", bus.emit("ping", 42), "seen:", bus.seen);
+
+// ── explicit constructor + super(): already escaped via `super`, keep working ──
+class Ctor extends EventEmitter {
+  seen = 0;
+  constructor(start: number) {
+    super();
+    this.seen = start;
+  }
+}
+function probeCtor(): string {
+  const c = new Ctor(3);
+  c.seen = 9;
+  return `${c.seen} ${typeof c.emit}`;
+}
+console.log("Ctor:", probeCtor());
+
+// ── an Error subclass (#573's sibling disqualifier) still works ──
+class MyError extends Error {
+  code = 42;
+}
+function probeErr(): string {
+  const e = new MyError("boom");
+  return `${e.message} ${e.code}`;
+}
+console.log("MyError:", probeErr());
+
+// ── an INDIRECT native base: declared fields on both hops must survive ──
+// (the emitter SURFACE on an indirect chain is a separate open gap — #6326 —
+// so this probes fields only, which is what this issue is about.)
+class Mid extends EventEmitter {
+  mid = 7;
+}
+class Leaf extends Mid {
+  leaf = 8;
+}
+function probeLeaf(): string {
+  const l = new Leaf();
+  l.leaf = 9;
+  return `${l.mid} ${l.leaf}`;
+}
+console.log("Leaf:", probeLeaf());
+
+// ── CONTROL: a plain class with no unmodeled base MUST still scalar-replace ──
+class Point {
+  x: number;
+  y: number;
+  constructor(x: number, y: number) {
+    this.x = x;
+    this.y = y;
+  }
+  getX(): number {
+    return this.x;
+  }
+}
+function hot(n: number): number {
+  let sum = 0;
+  for (let i = 0; i < n; i++) {
+    const p = new Point(i, i * 2);
+    sum += p.getX() + p.y;
+  }
+  return sum;
+}
+console.log("plain class (scalar-replaced):", hot(5));
+
+// ── CONTROL: a user-class chain is fully modeled — still scalar-replaced ──
+class Base {
+  base = 10;
+}
+class Derived extends Base {
+  own = 20;
+  total(): number {
+    return this.base + this.own;
+  }
+}
+function probeDerived(): string {
+  const d = new Derived();
+  return `${d.base} ${d.own} ${d.total()}`;
+}
+console.log("user chain:", probeDerived());