From 7c4c76ee4a10a979f32afbc9d3ebcdf950f45b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 13 Jul 2026 09:59:08 +0200 Subject: [PATCH] fix(codegen): don't scalar-replace an instance whose chain reaches an unmodeled base (#6343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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", )`), 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 ` (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. --- .../src/collectors/escape_news.rs | 16 ++ crates/perry-codegen/src/collectors/mod.rs | 4 +- .../src/collectors/this_as_value.rs | 161 +++++++++++++++++ ...est_gap_6343_scalar_native_base_surface.ts | 171 ++++++++++++++++++ 4 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 test-files/test_gap_6343_scalar_native_base_surface.ts diff --git a/crates/perry-codegen/src/collectors/escape_news.rs b/crates/perry-codegen/src/collectors/escape_news.rs index e3b999bff..4cb128486 100644 --- a/crates/perry-codegen/src/collectors/escape_news.rs +++ b/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 + // ` 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); + } } } diff --git a/crates/perry-codegen/src/collectors/mod.rs b/crates/perry-codegen/src/collectors/mod.rs index dd84a79dd..fca3502b8 100644 --- a/crates/perry-codegen/src/collectors/mod.rs +++ b/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, +}; diff --git a/crates/perry-codegen/src/collectors/this_as_value.rs b/crates/perry-codegen/src/collectors/this_as_value.rs index 47dd24770..4648a18ff 100644 --- a/crates/perry-codegen/src/collectors/this_as_value.rs +++ b/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", )`) 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 `, 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, +) -> bool { + let mut current = class; + let mut seen: HashSet = 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 ` — 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) -> 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 ` — 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)); + } } diff --git a/test-files/test_gap_6343_scalar_native_base_surface.ts b/test-files/test_gap_6343_scalar_native_base_surface.ts new file mode 100644 index 000000000..91d4e1c7e --- /dev/null +++ b/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", )` — 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());