perf(codegen): return undefined from a ctor symbol that cannot replace this (closes #8648)

changelog.d/8648-ctor-symbol-returns-undefined.md

@@ -0,0 +1,70 @@
+Restored `ret undefined` for a constructor symbol that cannot replace `this`.
+
+This is the **second**, independent cause of #8648's regression — the one
+`field_init.rs` (#8653) did not touch. It is not about inheritance, despite the
+symptom set: `benchmarks/issue-8289/cycles.ts` has no `extends` anywhere and was
+1.68x.
+
+#8630 gave every standalone `<Class>_constructor` symbol a completion block that
+ends in `js_ctor_return_override(this, <return slot>, …)`, so a derived `super()`
+whose base hands back a replacement object can publish it to the caller. The
+motivation is right, but it also changed the ORDINARY constructor's return value
+from `undefined` to `this` — and `lower_call/new.rs` says in its own comment what
+that costs: *"the fast arm ran no constructor, which is `undefined` — the same
+thing an ordinary ctor body returns — so `emit_ctor_return_override` below yields
+the instance on both arms"*. That sentence stopped being true. The caller's
+`is_undef` test went from never-taken to always-taken, and the call it guards is
+not cheap: `constructor_return_overrides_this` probes the typed-array registry,
+the buffer registry, callability, the Proxy registry, `arguments`, `clean_arr_ptr`
+(which walks GC forwarding chains) and finally the GC header — per construction,
+to answer "yes, an object" and hand back the value the caller already had. Under
+RS4GC the call is also a statepoint, so the live-pointer set spills and relocates
+around it.
+
+Why the affected set looked like an inheritance story: `ctor_prologue_stores`
+skips the constructor call entirely when the whole body is a run of
+`this.<f> = <param>` stores, so those classes never reach the symbol. That is
+exactly `micro_ctor` and `tree_wide` (1.00x). One literal initializer
+(`this.peer = null` in `cycles.ts`) or a `super()` disqualifies the plan, the
+call is emitted, and the class pays.
+
+The fix publishes `this` only when a replacement can exist. `ctor_chain_can_replace_this`
+(now shared, in `new_helpers.rs`) walks the heritage chain and answers `true` for a
+value-bearing `return` in any constructor on it, a native base, a dynamic
+`extends`, an id-only parent edge, or a class missing from `ctx.classes` —
+conservative on every edge it cannot see. Everything else returns `undefined`
+exactly as before #8630; every caller (`lower_call/new.rs`, the synthesized
+default-derived path in `codegen/method.rs`, and the runtime construct paths,
+which discard the value outright) already maps `undefined` onto its own receiver.
+
+`field_init.rs`'s own copy of this predicate is replaced by the shared one. The
+copy looked for the constructor in `class.methods` under the name `"constructor"`;
+HIR keeps it in `class.constructor` and never puts it in `methods`, so the
+value-returning arm could not fire. The shared version also uses
+`ctor_body_has_value_return`, which walks `try`/`switch`/`for-of` bodies that
+`collectors::mutation`'s version did not — that now-superseded copy is deleted.
+
+Measured (instructions retired, `/usr/bin/time -l`, vs the pre-#8630 numbers in
+the issue):
+
+| bench | pre-#8630 | main `c2da034` | this change | |
+|---|---|---|---|---|
+| two-class `new B(x, y)` loop | 998,471,071 | 1,648,244,291 | **1,007,905,144** | 1.65x -> **1.01x** |
+| `cycles.ts` | 1,301,925,013 | 2,182,711,384 | **1,330,975,040** | 1.68x -> **1.02x** |
+| plain-class control | 381,756,402 | 372,826,080 | 373,056,337 | 0.98x (unchanged) |
+
+Program output is byte-identical on all three.
+
+Differential against Node 26.5.1 (`--experimental-strip-types`), 21 constructor
+semantics cases: every one is byte-identical to what `main` prints, and 18 of 21
+match Node. The three that do not (`super()` from inside an arrow, an uncaught
+`ReferenceError` after a base ctor throws inside `try`, and `class X extends
+Error`) fail identically on `main` — pre-existing, untouched by this change. The
+cases that pin the semantics being preserved all match Node: a base constructor
+returning a replacement object seen by a subclass field initializer, a derived
+constructor returning an object, a derived constructor returning a primitive
+(TypeError), a conditional value return, and a value return inside `try`.
+
+`lower_call/ctor_return_publish_tests.rs` pins all four directions at the IR
+level — this is invisible to every behavioural test, since both spellings produce
+the same program output.

crates/perry-codegen/src/codegen/method.rs

@@ -1202,6 +1202,15 @@ pub(super) fn compile_method(
         })?;
     }
 
+    // #8648: pre-#8630 this symbol ended in `ret undefined`, and every caller
+    // maps `undefined` onto its own receiver. Returning `this` instead flipped
+    // the callers' `js_ctor_return_override` check from never-taken to
+    // always-taken — a cross-crate call that runs the typed-array, buffer,
+    // callable, Proxy, arguments and array probes before answering "yes, an
+    // object" and handing back the value the caller already held. Publish only
+    // when a replacement `this` can actually exist.
+    let publishes_this = standalone_ctor_return.is_some()
+        && crate::lower_call::ctor_chain_can_replace_this(ctx.classes, &class.name);
     if let Some((target, after_idx)) = standalone_ctor_return.as_ref() {
         let _ = ctx
             .inline_ctor_return
@@ -1215,23 +1224,24 @@ pub(super) fn compile_method(
 
     if !ctx.block().is_terminated() {
         let undef = crate::nanbox::double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED));
-        let return_value = if let Some((target, _)) = standalone_ctor_return.as_ref() {
-            let raw = ctx.block().load(DOUBLE, &target.result_slot);
-            let this_value = ctx
-                .this_stack
-                .last()
-                .cloned()
-                .map(|slot| ctx.block().load(DOUBLE, &slot))
-                .unwrap_or_else(|| undef.clone());
-            crate::lower_call::emit_ctor_return_override(
-                &mut ctx,
-                &this_value,
-                &raw,
-                target.is_derived,
-            )
-        } else {
-            undef.clone()
-        };
+        let return_value =
+            if let Some((target, _)) = standalone_ctor_return.as_ref().filter(|_| publishes_this) {
+                let raw = ctx.block().load(DOUBLE, &target.result_slot);
+                let this_value = ctx
+                    .this_stack
+                    .last()
+                    .cloned()
+                    .map(|slot| ctx.block().load(DOUBLE, &slot))
+                    .unwrap_or_else(|| undef.clone());
+                crate::lower_call::emit_ctor_return_override(
+                    &mut ctx,
+                    &this_value,
+                    &raw,
+                    target.is_derived,
+                )
+            } else {
+                undef.clone()
+            };
         if ctx.shared_super_scope_active {
             ctx.block().call_void("js_derived_super_scope_pop", &[]);
         }

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

@@ -82,9 +82,7 @@ pub(crate) use integer_locals::{
     collect_flat_row_aliases, is_int32_producing_expr, static_index_window,
 };
 pub(crate) use local_refs::{expr_contains_local_get, mark_all_candidate_refs_in_expr};
-pub(crate) use mutation::{
-    body_contains_call, body_contains_closure, body_returns_value, has_any_mutation,
-};
+pub(crate) use mutation::{body_contains_call, body_contains_closure, has_any_mutation};
 pub(crate) use number_by_construction::collect_number_by_construction_locals;
 pub(crate) use param_ranges::{collect_param_int_ranges, ParamIntRanges};
 pub(crate) use pointer_locals::collect_pointer_typed_locals;

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

@@ -57,44 +57,6 @@ pub fn body_contains_closure(stmts: &[perry_hir::Stmt]) -> bool {
     any_top_level_expr(stmts, &mut expr_contains_closure)
 }
 
-/// #8648: can this constructor body hand back a replacement `this`?
-///
-/// ECMAScript lets a constructor `return` an object, which becomes the
-/// construction's result (`js_ctor_return_override`). That replacement may be a
-/// Proxy, and DefineField must reach a Proxy's `defineProperty` trap rather
-/// than its `set`. With no value-returning `return` anywhere on the chain, the
-/// instance a field initializer writes to is provably the freshly allocated
-/// ordinary object, so `CreateDataProperty` and a plain own-slot store agree.
-pub fn body_returns_value(stmts: &[perry_hir::Stmt]) -> bool {
-    stmts_have_value_return(stmts)
-}
-
-/// `Stmt::Return(Some(_))` at any statement depth.
-fn stmts_have_value_return(stmts: &[perry_hir::Stmt]) -> bool {
-    use perry_hir::Stmt;
-    for s in stmts {
-        let hit = match s {
-            Stmt::Return(Some(_)) => true,
-            Stmt::If {
-                then_branch,
-                else_branch,
-                ..
-            } => {
-                stmts_have_value_return(then_branch)
-                    || else_branch
-                        .as_ref()
-                        .is_some_and(|b| stmts_have_value_return(b))
-            }
-            Stmt::While { body, .. } | Stmt::For { body, .. } => stmts_have_value_return(body),
-            _ => false,
-        };
-        if hit {
-            return true;
-        }
-    }
-    false
-}
-
 fn expr_contains_closure(expr: &perry_hir::Expr) -> bool {
     if matches!(expr, perry_hir::Expr::Closure { .. }) {
         return true;