fix(array): a typed array is not concat-spreadable, and must not be dropped

changelog.d/2879-concat-typed-array-not-spreadable.md

@@ -0,0 +1,78 @@
+### Fixed
+
+- **`Array.prototype.concat` no longer drops a typed-array argument.**
+  `[1, 2].concat(new Uint8Array([3, 4]))` returned `[1, 2]`; node returns
+  `[1, 2, Uint8Array(2)]`. The argument vanished with no error and no
+  diagnostic.
+
+  Two defects stacked. A typed array is **not** concat-spreadable — the spec's
+  `IsConcatSpreadable` falls back to `IsArray`, which is false for a TypedArray
+  — but this runtime's `js_array_is_array` answers true for one, so
+  `append_concat_arg` took the spread branch instead of appending a single
+  element. That spread then ran through `js_array_concat`, whose
+  `clean_arr_ptr` nulls every tracked typed array, so it contributed nothing.
+
+  Before either could be reached, the all-dense bulk path in
+  `dense_concat_array_source` cleaned the argument first: `clean_arr_ptr`
+  returned null, the `src.is_null()` arm reported "empty dense source", and the
+  bulk path returned early — so the spec-shaped flow never ran at all. The
+  typed-array rejection that function already carries sits BELOW that clean and
+  was unreachable for exactly the values it names.
+
+  This is the same shape as the comment immediately above it, which describes
+  a `class X extends Array` argument being mis-classified as an empty dense
+  source and silently dropped.
+
+  Affected files:
+
+  - `crates/perry-runtime/src/array/from_concat.rs` — reject typed arrays and
+    registered buffers in `dense_concat_array_source` before the clean, and
+    append them as one element in `append_concat_arg`.
+
+  The spread accumulator (`js_array_concat`) is deliberately untouched:
+  `[...new Uint8Array([5, 6])]` must keep materializing elements, and
+  "fixing" the ordering there instead would have traded a dropped argument for
+  a wrong element count.
+
+  Validation: byte-compared against node 26.5.1 across `Uint8Array`,
+  `Int32Array` and `Float64Array` arguments, an empty typed array, a
+  multi-argument call mixing plain arrays and a typed array, an empty receiver,
+  and controls for plain-array concat, nested arrays, string elements, Set
+  spread, and `[...typedArray]` spread — all matching.
+
+- **A `Symbol` key on a typed-array receiver was dropped in silence**, which is
+  why the `@@isConcatSpreadable` opt-in above could not be exercised by
+  assignment. ECMA-262 §10.4.5.5 routes a key that is not a
+  CanonicalNumericIndexString to OrdinarySet, and a `Symbol` is definitionally
+  not one — but `typed_array_set_numeric_index` could not tell the two apart. A
+  `Symbol` arrives as a NaN-boxed pointer, which AS AN `f64` is a NaN, so it
+  took the "canonical-invalid index" arm, coerced the value for side effects,
+  and returned `true` meaning "write handled". The store vanished:
+  `u8[sym] = 5` then read back `undefined` and
+  `Object.getOwnPropertySymbols(u8)` stayed empty, while the identical code on
+  a plain object, a plain array and a `Buffer` all worked.
+
+  Same shape as #8090/#8109/#8119/#8120/#8141: a receiver-specific fast path
+  claims the operation before the key-kind question is asked.
+
+  - `crates/perry-runtime/src/object/polymorphic_index.rs` — ask the key-kind
+    question before either typed-array arm claims the receiver, and route a
+    `Symbol` to the symbol side table, where `js_put_value_set` and
+    `js_array_set_index_or_string` already put it. Gated on the receiver, and
+    on BOTH typed-array registries, since either arm alone would still claim
+    the write.
+  - `crates/perry-runtime/src/typedarray_props.rs` — make the numeric-index
+    arm's contract honest: decline a key it cannot classify instead of
+    reporting it handled. Inert for this module's own callers, which reach it
+    only under `is_int32()` / `is_finite()`.
+
+  This makes the `@@isConcatSpreadable === true` opt-in documented above
+  actually reachable by assignment: `[1].concat(u8)` with the flag set now
+  gives node's `[1,9,10]`.
+
+  Validation: `test-files/test_gap_typed_array_symbol_key.ts` byte-compared
+  against node 26.5.1 across all four receiver kinds, the element-store
+  control, both opt-in forms and the default. Sabotage: with the routing
+  removed the compiled probe diverges from node (`ta set/get: undefined |
+  ownSyms: 0`); with the numeric-arm guard removed the unit test fails.
+  `perry-runtime --lib` 2385 passed / 0 failed.

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

@@ -331,7 +331,26 @@ pub(crate) fn append_concat_arg(result: *mut ArrayHeader, value: f64) -> *mut Ar
         return append_spread_array(result, snap_ptr);
     }
 
-    // Arrays (and set/map/typed-array/buffer that concat treats array-like via
+    // #2879: a typed array is NOT concat-spreadable. `IsConcatSpreadable`
+    // (ECMA-262 §23.1.3.1 step 2) falls back to `IsArray`, and `IsArray` is
+    // FALSE for a TypedArray — node appends `new Uint8Array([3,4])` as one
+    // element, giving `[1,2,Uint8Array(2)]`.
+    //
+    // Checked before the `is_array` branch because this runtime's
+    // `js_array_is_array` answers true for a typed array, so it took the spread
+    // path — and that spread runs through `js_array_concat`, whose
+    // `clean_arr_ptr` nulls a tracked typed array. The argument contributed
+    // nothing and vanished: `[1,2].concat(u8)` returned `[1,2]`. An explicit
+    // `@@isConcatSpreadable === true` still opts in below, which is the one way
+    // the spec does spread one.
+    if spreadable != Some(true)
+        && (crate::typedarray::lookup_typed_array_kind(raw_addr).is_some()
+            || crate::buffer::is_registered_buffer(raw_addr))
+    {
+        return js_array_push_f64(result, value);
+    }
+
+    // Arrays (and set/map/buffer that concat treats array-like via
     // js_array_concat) spread by default, unless @@isConcatSpreadable === false.
     let is_array = js_array_is_array(value).to_bits() == 0x7FFC_0000_0000_0004;
     if is_array {
@@ -857,6 +876,28 @@ unsafe fn dense_concat_array_source(src: *const ArrayHeader) -> Option<(*const A
     if crate::array::subclass::raw_receiver_is_heap_object(src) {
         return None;
     }
+    // #2879: the same silent-drop hazard the comment above describes, for a
+    // typed array rather than a subclass. `clean_arr_ptr` nulls every tracked
+    // typed array, the `src.is_null()` arm below then reports "empty dense
+    // source", and the bulk path returns `Some(out)` — so the spec-shaped
+    // `append_concat_arg` flow never runs and the argument disappears.
+    // `[1,2].concat(new Uint8Array([3,4]))` yielded `1,2`.
+    //
+    // The rejection further down covers exactly these registries; it is simply
+    // BELOW the clean, which makes it unreachable for the values it names.
+    // Asking first is what lets the caller fall through to the spec path, where
+    // a typed array is appended as ONE element because `IsArray` is false for
+    // it.
+    let raw_before_clean = crate::array::array_receiver_addr(src as *mut ArrayHeader);
+    // `array_receiver_addr` only strips the NaN-box tag, and neither registry
+    // helper validates what it is handed, so filter the handle band with the
+    // canonical predicate first rather than open-coding a floor here.
+    if crate::value::addr_class::is_plausible_heap_addr(raw_before_clean)
+        && (crate::typedarray::lookup_typed_array_kind(raw_before_clean).is_some()
+            || crate::buffer::is_registered_buffer(raw_before_clean))
+    {
+        return None;
+    }
     let src = clean_arr_ptr(src);
     if src.is_null() {
         return Some((src, 0));

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

@@ -119,6 +119,8 @@ mod object_ops;
 pub(crate) use object_ops::{ensure_key_in_keys_array, install_builtin_getter};
 mod object_ops_frozen;
 mod polymorphic_index;
+#[cfg(test)]
+mod polymorphic_index_symbol_tests;
 mod primitive_proto_thunks;
 mod property_key;
 pub(crate) mod prototype_chain;

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

@@ -316,6 +316,30 @@ pub extern "C" fn js_object_set_index_polymorphic(obj_handle: i64, idx: f64, val
     if raw < 0x1000 {
         return;
     }
+    // Ask the KEY-KIND question before either typed-array arm claims the
+    // receiver. A Symbol is definitionally not a CanonicalNumericIndexString,
+    // so ECMA-262 §10.4.5.5 requires OrdinarySet — the symbol side table, the
+    // same place `js_put_value_set` and `js_array_set_index_or_string` already
+    // put it. Without this, `typed_array_set_numeric_index` read the NaN-boxed
+    // symbol pointer as a non-finite f64, classified it "canonical-invalid
+    // index", and returned "handled" — so `u8[sym] = v` was dropped silently:
+    // the store never landed, `u8[sym]` read back `undefined`, and
+    // `Object.getOwnPropertySymbols(u8)` stayed empty while the same code on a
+    // plain object, a plain array and a Buffer all worked.
+    //
+    // The visible consequence was `@@isConcatSpreadable`: `concat` honours the
+    // opt-in correctly (`Object.defineProperty` proves it), but the assignment
+    // form could never install the property, so a typed array could not opt in.
+    //
+    // Gated on the receiver so only the broken case changes: BOTH registries
+    // are consulted, because either arm alone would still claim the write.
+    if unsafe { crate::symbol::js_is_symbol(idx) } != 0
+        && (crate::typedarray::lookup_typed_array_kind(raw as usize).is_some()
+            || crate::typedarray_props::is_typed_array_owner(raw as usize))
+    {
+        unsafe { crate::symbol::js_object_set_symbol_property(boxed, idx, value) };
+        return;
+    }
     // #5525 fast path: a cached typed-array kind lookup + inline store, before
     // the thread-local `typed_array_set_numeric_index` registry dispatch
     // (`typed_array_owner_*` → `_tlv_get_addr`) that dominated the bcrypt

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

@@ -0,0 +1,70 @@
+//! A Symbol key on a typed-array receiver must do OrdinarySet — it must not be
+//! swallowed by the numeric-index arms of the computed-store path.
+//!
+//! ECMA-262 §10.4.5.5: an Integer-Indexed exotic object routes a key that is
+//! NOT a CanonicalNumericIndexString to OrdinarySet. A Symbol is definitionally
+//! not one. `typed_array_set_numeric_index` could not tell the two apart — a
+//! Symbol arrives as a NaN-boxed pointer, which AS AN f64 is a NaN, so it took
+//! the "canonical-invalid index" arm, coerced for side effects, and returned
+//! `true` meaning "write handled". The store was dropped in silence.
+//!
+//! # Why this test is shaped as a contract assertion
+//!
+//! The obvious end-to-end shape — allocate a typed array, call
+//! `js_object_set_index_polymorphic` with a symbol key, read it back — PASSES
+//! WITHOUT THE FIX and is therefore worthless. Measured: with the routing
+//! removed, the direct call still stored the property, while the same source
+//! compiled and run diverged from node. The direct call reaches a different
+//! sub-arm than the compiled path does, so it cannot witness the bug.
+//!
+//! The end-to-end coverage therefore lives in
+//! `test-files/test_gap_typed_array_symbol_key.ts`, which is byte-compared
+//! against node and does fail without the fix. What is left here is the piece a
+//! unit test CAN witness: that the numeric-index arm no longer claims a key it
+//! cannot classify.
+
+use crate::typedarray::{typed_array_alloc, KIND_UINT8};
+
+/// The numeric-index arm must decline a Symbol key rather than report it
+/// handled. Pre-fix this returned `true` and the write vanished.
+#[test]
+fn the_numeric_index_arm_does_not_claim_a_symbol_key() {
+    let _serialized = crate::array::test_serialize();
+    let ta = typed_array_alloc(KIND_UINT8, 2);
+    crate::typedarray::js_typed_array_set(ta, 0, 1.0);
+    crate::typedarray::js_typed_array_set(ta, 1, 2.0);
+
+    let sym = unsafe { crate::symbol::js_symbol_new_empty() };
+    assert_ne!(
+        unsafe { crate::symbol::js_is_symbol(sym) },
+        0,
+        "precondition: the key under test must actually be a Symbol"
+    );
+
+    let claimed =
+        unsafe { crate::typedarray_props::typed_array_set_numeric_index(ta as usize, sym, 5.0) };
+    assert!(
+        !claimed,
+        "a Symbol is not a CanonicalNumericIndexString, so the numeric-index \
+         arm must decline it and let the caller route it to OrdinarySet; \
+         pre-fix it read the NaN-boxed symbol as a non-finite f64, classified \
+         it a canonical-invalid index, and reported the write handled"
+    );
+}
+
+/// The control that keeps the guard honest: a real out-of-bounds numeric index
+/// must STILL be claimed and dropped per spec. Without this, making the
+/// function decline everything would satisfy the test above.
+#[test]
+fn the_numeric_index_arm_still_claims_an_out_of_bounds_numeric_key() {
+    let _serialized = crate::array::test_serialize();
+    let ta = typed_array_alloc(KIND_UINT8, 2);
+
+    let claimed =
+        unsafe { crate::typedarray_props::typed_array_set_numeric_index(ta as usize, 99.0, 5.0) };
+    assert!(
+        claimed,
+        "an out-of-bounds CanonicalNumericIndexString is still the numeric \
+         arm's to handle — it is dropped per spec, not routed to OrdinarySet"
+    );
+}

crates/perry-runtime/src/typedarray_props.rs

@@ -566,10 +566,30 @@ unsafe fn typed_array_coerce_element_for_side_effects(owner: usize, value: f64)
     }
 }
 
+/// True when `owner` is a typed-array receiver by EITHER registry — the
+/// cached-kind lookup used by the inline fast path, or the thread-local owner
+/// registry this module's slow dispatch gates on. Callers routing a key AWAY
+/// from the numeric-index arms need both, because either one alone leaves the
+/// other arm free to claim the write.
+pub(crate) fn is_typed_array_owner(owner: usize) -> bool {
+    typed_array_owner_kind(owner).is_some()
+}
+
 pub(crate) unsafe fn typed_array_set_numeric_index(owner: usize, index: f64, value: f64) -> bool {
     if typed_array_owner_kind(owner).is_none() {
         return false;
     }
+    // A Symbol key is NOT a CanonicalNumericIndexString, so ECMA-262
+    // §10.4.5.5 sends it to OrdinarySet — it is not an invalid index to be
+    // dropped. The `is_finite` test below cannot tell the two apart: a Symbol
+    // arrives as a NaN-boxed pointer, which AS AN f64 is a NaN, so it took the
+    // "canonical-invalid index" arm and returned `true` (write handled), and
+    // `u8[sym] = v` vanished with no error. Say "not mine" instead, so the
+    // caller can route it. Inert for this module's own callers, which reach
+    // here only under `is_int32()` / `is_finite()`.
+    if crate::symbol::js_is_symbol(index) != 0 {
+        return false;
+    }
     if !index.is_finite() || index.fract() != 0.0 || index < 0.0 || index > u32::MAX as f64 {
         // Canonical-invalid index: coerce the value for side effects, then drop.
         typed_array_coerce_element_for_side_effects(owner, value);

test-files/test_gap_typed_array_symbol_key.ts

@@ -0,0 +1,42 @@
+// A Symbol key on a typed-array receiver must do OrdinarySet (ECMA-262
+// §10.4.5.5: a key that is not a CanonicalNumericIndexString is not an index).
+// Perry's computed-store path let the numeric-index arm claim the write: a
+// Symbol is a NaN-boxed pointer, which as an f64 is a NaN, so it was
+// classified a "canonical-invalid index" and dropped in silence.
+
+const s: any = Symbol("x");
+
+// Every receiver kind must behave the same. Only the typed array was broken.
+const o: any = {};
+o[s] = 5;
+console.log("obj:", o[s], Object.getOwnPropertySymbols(o).length);
+
+const arr: any = [1, 2];
+arr[s] = 5;
+console.log("arr:", arr[s], Object.getOwnPropertySymbols(arr).length);
+
+const buf: any = Buffer.alloc(2);
+buf[s] = 5;
+console.log("buf:", buf[s], Object.getOwnPropertySymbols(buf).length);
+
+const u8: any = new Uint8Array([1, 2]);
+u8[s] = 5;
+console.log("u8:", u8[s], Object.getOwnPropertySymbols(u8).length);
+
+// The element store through the same helper must be undisturbed.
+u8[1] = 9;
+console.log("elements:", u8[0], u8[1]);
+
+// The user-visible consequence: a typed array could not opt in to
+// @@isConcatSpreadable by assignment, though defineProperty worked.
+const a: any = new Uint8Array([9, 10]);
+a[Symbol.isConcatSpreadable] = true;
+console.log("optin readback:", a[Symbol.isConcatSpreadable]);
+console.log("optin concat:", JSON.stringify([1].concat(a)));
+
+const b: any = new Uint8Array([9, 10]);
+Object.defineProperty(b, Symbol.isConcatSpreadable, { value: true, configurable: true });
+console.log("defineProperty concat:", JSON.stringify([1].concat(b)));
+
+// Default (no opt-in): a typed array is NOT concat-spreadable.
+console.log("default concat:", JSON.stringify([1, 2].concat(new Uint8Array([3, 4]))));