fix(runtime): a borrowed builtin is only an Array builtin when it IS Array.prototype[m] (#5902)

changelog.d/7471-borrowed-array-builtin-identity.md

@@ -0,0 +1 @@
+fix(runtime): a borrowed builtin only runs the array engine when it IS `Array.prototype[method]` (#5902). `classify_own_slot` (`array/generic.rs`) decides whether an own slot holding a builtin closure means "the array-like engine must run on this receiver", but it asked only "is this a non-constructable builtin closure?" — a test every builtin prototype method passes, with the owning prototype never checked. So `obj.concat = String.prototype.concat` was read as a borrowed *Array* builtin and ran the array algorithm, yielding `[obj, "two", undefined]` instead of `"onetwoundefined"` (test262 `built-ins/String/prototype/concat/S15.5.4.6_A4_T1`) — while the equivalent `String.prototype.concat.call(obj, …)` was already correct, which is the tell that this was dispatch and not coercion. The new `is_array_prototype_method_value` compares the slot against the real `Array.prototype[method]` by closure FUNCTION POINTER rather than by closure identity: reading `Array.prototype.concat` can hand back a freshly reified closure, so identity would give false negatives, while every reification of one builtin shares a single code address — and a func ptr is a code address, not a heap pointer, so nothing here becomes GC-visible and no root scanner is required. The `BOUND_METHOD_FUNC_PTR` arm is untouched and the new check gates only the raw-builtin-thunk arm, so the behavior the classification exists to protect is preserved: a genuine `obj.pop = Array.prototype.pop` borrow still runs the engine on the real receiver instead of looping on its captured `Array.prototype`. test262 `built-ins/String/prototype/concat` 20/21 → 21/21 (parity 100%); `Array/prototype/{concat,push,pop,splice,sort,shift,unshift,reverse}` zero regressions — 10 remaining failures are named in #5898's baseline (including `unshift/S15.4.4.13_A4_T1`, which itself borrows `Array.prototype.unshift` and is unchanged), and `Array/prototype/concat/S15.4.4.4_A1_T2`, absent from that snapshot, was proven pre-existing by rebuilding without the change and reproducing it identically. A differential probe matches node byte-for-byte on all 8 lines, covering both directions (the String borrow, genuine Array borrows of concat/push/pop/splice, a same-named user method, plain arrays). New per-PR-visible coverage: a `perry-runtime --lib` unit test pinning both directions of the discriminator plus the wrong-method-name and non-callable cases, which cannot pass vacuously — an always-true or always-false predicate fails one of its asserts.

crates/perry-runtime/src/array/generic.rs

@@ -1263,7 +1263,7 @@ pub fn try_array_proto_chain_method(
     let raw = (object.to_bits() & 0x0000_FFFF_FFFF_FFFF) as *const crate::object::ObjectHeader;
     let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32);
     let own = crate::object::js_object_get_field_by_name_f64(raw, key);
-    if matches!(classify_own_slot(own), OwnSlot::UserMethod) {
+    if matches!(classify_own_slot(own, method), OwnSlot::UserMethod) {
         return None;
     }
     if !proto_chain_contains_real_array(raw as usize) {
@@ -1462,7 +1462,7 @@ enum OwnSlot {
     BorrowedBuiltin,
 }
 
-fn classify_own_slot(v: f64) -> OwnSlot {
+fn classify_own_slot(v: f64, method: &str) -> OwnSlot {
     let jv = JSValue::from_bits(v.to_bits());
     if !jv.is_pointer() {
         return OwnSlot::Absent;
@@ -1473,19 +1473,32 @@ fn classify_own_slot(v: f64) -> OwnSlot {
     }
     let fp = crate::closure::get_valid_func_ptr(c);
     if fp.is_null() {
-        OwnSlot::Absent
-    } else if fp == crate::closure::BOUND_METHOD_FUNC_PTR
-        // A raw built-in prototype-method closure (`{ splice:
-        // Array.prototype.splice }` stores the thunk itself, not a bound
-        // reification) must also run the generic engine on THIS receiver —
-        // dispatching it as a user method loses the receiver entirely
-        // (test262 splice/S15.4.4.12_A6.1_T3).
-        || crate::object::builtin_closure_is_non_constructable_value(v)
+        return OwnSlot::Absent;
+    }
+    if fp == crate::closure::BOUND_METHOD_FUNC_PTR {
+        return OwnSlot::BorrowedBuiltin;
+    }
+    // A raw built-in prototype-method closure (`{ splice:
+    // Array.prototype.splice }` stores the thunk itself, not a bound
+    // reification) must also run the generic engine on THIS receiver —
+    // dispatching it as a user method loses the receiver entirely
+    // (test262 splice/S15.4.4.12_A6.1_T3).
+    //
+    // #5902: but "a non-constructable builtin closure" is NOT the same claim as
+    // "a borrowed *Array* builtin". Every builtin prototype method answers that
+    // test, so `obj.concat = String.prototype.concat` was misclassified and ran
+    // the ARRAY algorithm — `[obj, "two", undefined]` instead of
+    // `"onetwoundefined"` (test262 concat/S15.5.4.6_A4_T1), even though the
+    // equivalent `String.prototype.concat.call(obj, …)` was already correct.
+    // Require the slot to hold the actual `Array.prototype[method]`; anything
+    // else is a foreign builtin that must reach its own reflective thunk via
+    // the normal dispatch, which binds `this` to the receiver.
+    if crate::object::builtin_closure_is_non_constructable_value(v)
+        && crate::object::is_array_prototype_method_value(v, method)
     {
-        OwnSlot::BorrowedBuiltin
-    } else {
-        OwnSlot::UserMethod
+        return OwnSlot::BorrowedBuiltin;
     }
+    OwnSlot::UserMethod
 }
 
 /// True when `object` owns a user method (an own callable field, not a borrowed
@@ -1498,7 +1511,7 @@ pub(crate) fn object_owns_user_method(object: f64, method: &str) -> bool {
     let raw = (object.to_bits() & 0x0000_FFFF_FFFF_FFFF) as *const crate::object::ObjectHeader;
     let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32);
     let own = crate::object::js_object_get_field_by_name_f64(raw, key);
-    matches!(classify_own_slot(own), OwnSlot::UserMethod)
+    matches!(classify_own_slot(own, method), OwnSlot::UserMethod)
 }
 
 /// Dispatch a generic `Array.prototype` mutator over an array-like receiver.
@@ -1540,7 +1553,7 @@ pub fn try_object_arraylike_mutator(
     // method (`{ push(x) {…} }`) is left to the normal dispatch.
     let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32);
     let own = crate::object::js_object_get_field_by_name_f64(raw, key);
-    if matches!(classify_own_slot(own), OwnSlot::UserMethod) {
+    if matches!(classify_own_slot(own, method), OwnSlot::UserMethod) {
         return None;
     }
     run_object_mutator(object, method, args_ptr, args_len)

crates/perry-runtime/src/array/tests.rs

@@ -1642,3 +1642,110 @@ fn push_built_array_gets_and_keeps_dense_raw_f64_flag() {
         );
     }
 }
+
+/// #5902: the array-like engine may only claim an own slot when the borrowed
+/// builtin it holds is genuinely `Array.prototype[method]`.
+///
+/// `classify_own_slot` used to ask only "is this a non-constructable builtin
+/// closure?", which EVERY builtin prototype method answers yes to — so
+/// `obj.concat = String.prototype.concat` was read as a borrowed *Array*
+/// builtin and ran the array algorithm, returning `[obj, "two", undefined]`
+/// where the spec gives `"onetwoundefined"` (test262
+/// `built-ins/String/prototype/concat/S15.5.4.6_A4_T1`). The discriminator is
+/// the closure's function pointer; this test pins both directions of it, so a
+/// regression that re-broadens (or over-narrows) the predicate fails here
+/// rather than only in the parity sweep.
+#[test]
+fn array_prototype_method_discriminator_separates_foreign_builtins() {
+    // Reads realm intrinsics and holds raw pointers across allocating calls,
+    // and libtest gives each test its own thread where `GLOBAL_THIS_PTR` can be
+    // re-created — so resolve the whole snapshot inside one iteration and retry
+    // until a GC-quiet pass yields a self-consistent view. Mirrors
+    // `array_literal_shares_the_realm_array_prototype`'s loop above.
+    let mut checked = false;
+    for _ in 0..256 {
+        let global = crate::object::js_get_global_this();
+        let global_ptr =
+            crate::value::js_nanbox_get_pointer(global) as *const crate::object::ObjectHeader;
+        if global_ptr.is_null() {
+            std::thread::yield_now();
+            continue;
+        }
+
+        let proto_of = |ctor_name: &[u8]| -> Option<f64> {
+            let ctor =
+                crate::object::js_object_get_field_by_name(global_ptr, string_key(ctor_name));
+            if !ctor.is_pointer() {
+                return None;
+            }
+            let ctor_ptr =
+                crate::value::js_nanbox_get_pointer(f64::from_bits(ctor.bits())) as usize;
+            if ctor_ptr == 0 {
+                return None;
+            }
+            Some(crate::closure::closure_get_dynamic_prop(
+                ctor_ptr,
+                "prototype",
+            ))
+        };
+
+        let (Some(array_proto), Some(string_proto)) = (proto_of(b"Array"), proto_of(b"String"))
+        else {
+            std::thread::yield_now();
+            continue;
+        };
+        let array_proto_ptr =
+            crate::value::js_nanbox_get_pointer(array_proto) as *const crate::object::ObjectHeader;
+        let string_proto_ptr =
+            crate::value::js_nanbox_get_pointer(string_proto) as *const crate::object::ObjectHeader;
+        if array_proto_ptr.is_null() || string_proto_ptr.is_null() {
+            std::thread::yield_now();
+            continue;
+        }
+
+        let array_concat =
+            crate::object::js_object_get_field_by_name_f64(array_proto_ptr, string_key(b"concat"));
+        let string_concat =
+            crate::object::js_object_get_field_by_name_f64(string_proto_ptr, string_key(b"concat"));
+        if !crate::value::JSValue::from_bits(array_concat.to_bits()).is_pointer()
+            || !crate::value::JSValue::from_bits(string_concat.to_bits()).is_pointer()
+        {
+            std::thread::yield_now();
+            continue;
+        }
+
+        // The Array borrow must still be claimed — this is the behavior the
+        // original classification existed to protect (`obj.concat =
+        // Array.prototype.concat` has to run the array engine on `obj`).
+        assert!(
+            crate::object::is_array_prototype_method_value(array_concat, "concat"),
+            "Array.prototype.concat must be recognized as an Array builtin"
+        );
+        // The foreign borrow must NOT be claimed.
+        assert!(
+            !crate::object::is_array_prototype_method_value(string_concat, "concat"),
+            "String.prototype.concat must not be mistaken for an Array builtin"
+        );
+        // Right closure, wrong method name is also a mismatch — the predicate
+        // keys on the (method, closure) pair, not on "is some Array builtin".
+        assert!(
+            !crate::object::is_array_prototype_method_value(array_concat, "push"),
+            "Array.prototype.concat must not answer for `push`"
+        );
+        // Non-callable / non-pointer slots are never a borrowed builtin.
+        assert!(!crate::object::is_array_prototype_method_value(
+            1.0, "concat"
+        ));
+        assert!(!crate::object::is_array_prototype_method_value(
+            f64::from_bits(crate::value::TAG_UNDEFINED),
+            "concat"
+        ));
+
+        checked = true;
+        break;
+    }
+    assert!(
+        checked,
+        "never obtained a GC-quiet view of Array.prototype / String.prototype"
+    );
+}

crates/perry-runtime/src/object/global_this.rs

@@ -97,16 +97,16 @@ pub(crate) use ctor_thunks::{
     builtin_prototype_value, cryptokey_algorithm_getter_thunk, cryptokey_extractable_getter_thunk,
     cryptokey_type_getter_thunk, cryptokey_usages_getter_thunk, error_constructor_call_thunk,
     eval_error_constructor_call_thunk, global_this_crypto_getter_thunk,
-    global_this_url_pattern_call_thunk, is_function_prototype_object_value,
-    map_constructor_call_thunk, normalize_eval_this_body, promise_constructor_call_thunk,
-    range_error_constructor_call_thunk, reference_error_constructor_call_thunk,
-    regexp_constructor_call_thunk, set_constructor_call_thunk, subtle_crypto_method_value,
-    syntax_error_constructor_call_thunk, type_error_constructor_call_thunk,
-    typed_array_constructor_call_thunk, uri_error_constructor_call_thunk,
-    weak_map_constructor_call_thunk, weak_ref_constructor_call_thunk,
-    weak_set_constructor_call_thunk, webcrypto_get_random_values_thunk,
-    webcrypto_illegal_constructor_thunk, webcrypto_method_value, webcrypto_random_uuid_thunk,
-    webcrypto_subtle_getter_thunk,
+    global_this_url_pattern_call_thunk, is_array_prototype_method_value,
+    is_function_prototype_object_value, map_constructor_call_thunk, normalize_eval_this_body,
+    promise_constructor_call_thunk, range_error_constructor_call_thunk,
+    reference_error_constructor_call_thunk, regexp_constructor_call_thunk,
+    set_constructor_call_thunk, subtle_crypto_method_value, syntax_error_constructor_call_thunk,
+    type_error_constructor_call_thunk, typed_array_constructor_call_thunk,
+    uri_error_constructor_call_thunk, weak_map_constructor_call_thunk,
+    weak_ref_constructor_call_thunk, weak_set_constructor_call_thunk,
+    webcrypto_get_random_values_thunk, webcrypto_illegal_constructor_thunk, webcrypto_method_value,
+    webcrypto_random_uuid_thunk, webcrypto_subtle_getter_thunk,
 };
 #[cfg(feature = "temporal")]
 pub(crate) use fetch_globals::temporal_subclass_super;

crates/perry-runtime/src/object/global_this/ctor_thunks.rs

@@ -221,6 +221,58 @@ pub(crate) fn builtin_prototype_value(name: &str) -> f64 {
     crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype")
 }
 
+/// #5902: is `value` literally `Array.prototype[method]`?
+///
+/// The array-like engine treats *any* borrowed builtin closure stored in an
+/// own slot as a borrowed **Array** builtin (`classify_own_slot`), which is
+/// only true when the borrow actually came off `Array.prototype`.
+/// `obj.concat = String.prototype.concat` stores a non-constructable builtin
+/// closure too, so it was misread and ran the array algorithm — returning
+/// `[obj, "two", undefined]` where the spec (and the working
+/// `String.prototype.concat.call(obj, …)` form) gives `"onetwoundefined"`.
+///
+/// Compared by closure FUNCTION POINTER, not by closure identity: reading
+/// `Array.prototype.concat` can hand back a freshly reified closure, but every
+/// reification of the same builtin shares one code address. A func ptr is a
+/// code address rather than a heap pointer, so nothing here is GC-visible and
+/// no root scanner is required.
+pub(crate) fn is_array_prototype_method_value(value: f64, method: &str) -> bool {
+    let slot = crate::value::JSValue::from_bits(value.to_bits());
+    if !slot.is_pointer() {
+        return false;
+    }
+    let slot_ptr = slot.as_pointer::<crate::closure::ClosureHeader>();
+    if slot_ptr.is_null() {
+        return false;
+    }
+    let slot_fp = crate::closure::get_valid_func_ptr(slot_ptr);
+    if slot_fp.is_null() {
+        return false;
+    }
+
+    let proto = builtin_prototype_value("Array");
+    let proto_bits = proto.to_bits();
+    if (proto_bits >> 48) != 0x7FFD {
+        return false;
+    }
+    let proto_ptr = (proto_bits & crate::value::POINTER_MASK) as *const super::super::ObjectHeader;
+    if proto_ptr.is_null() {
+        return false;
+    }
+    let key = crate::string::js_string_from_bytes(method.as_ptr(), method.len() as u32);
+    let canonical = super::super::js_object_get_field_by_name_f64(proto_ptr, key);
+    let canonical_jv = crate::value::JSValue::from_bits(canonical.to_bits());
+    if !canonical_jv.is_pointer() {
+        return false;
+    }
+    let canonical_ptr = canonical_jv.as_pointer::<crate::closure::ClosureHeader>();
+    if canonical_ptr.is_null() {
+        return false;
+    }
+    let canonical_fp = crate::closure::get_valid_func_ptr(canonical_ptr);
+    !canonical_fp.is_null() && canonical_fp == slot_fp
+}
+
 pub(crate) extern "C" fn webcrypto_illegal_constructor_thunk(
     _closure: *const crate::closure::ClosureHeader,
 ) -> f64 {

crates/perry-runtime/src/object/mod.rs

@@ -55,7 +55,8 @@ mod global_this;
 pub mod handle_expando;
 pub(crate) mod prop_plan;
 pub(crate) use global_this::{
-    default_prepare_stack_trace_func_ptr, scan_error_constructor_root_mut, ERROR_CONSTRUCTOR_PTR,
+    default_prepare_stack_trace_func_ptr, is_array_prototype_method_value,
+    scan_error_constructor_root_mut, ERROR_CONSTRUCTOR_PTR,
 };
 mod global_this_tables;
 mod groupby;