fix(runtime): honor exotic indices in Array reverse (rebase of #8400)

changelog.d/8400-array-reverse-exotic-indices.md

@@ -0,0 +1,20 @@
+### Fixed
+
+**`Array.prototype.reverse` now observes inherited, accessor-backed, and
+deleted indices in specification order (#5898).** The raw-slot implementation
+in `array::concat_reverse::js_array_reverse` treated holes as absent even when
+an indexed prototype property filled them, and it observed both sides before a
+getter could truncate the array. Exotic arrays now use live `HasProperty`,
+`Get`, `Set`, and `Delete` operations while ordinary dense arrays retain the
+allocation-free swap.
+
+The carried receiver and values remain in runtime handles across accessors and
+other collection points, with each value reloaded immediately before its set.
+Array-length truncation in `array::push_pop::js_array_set_length` also deletes
+descriptor-backed and sparse indices from high to low, and
+`js_array_delete` clears a sparse side-table entry even after later growth has
+made the index fit inside dense capacity. The regression in
+`crates/perry/tests/issue_5898_array_reverse_exotic.rs` covers inherited
+indices, getter-driven truncation, sparse truncation, and grow-then-delete.
+Validation passes that compiled regression, all 221 `array::` runtime tests,
+and all 50 repository lint gates.

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

@@ -224,6 +224,15 @@ pub extern "C" fn js_array_reverse(arr: *mut ArrayHeader) -> *mut ArrayHeader {
         if len <= 1 {
             return arr;
         }
+        // A raw slot swap is only equivalent to Reverse when every index is
+        // an ordinary dense own property. Holes can expose inherited
+        // Array.prototype indices, and accessors may delete later indices
+        // while Reverse is still observing them. Run the spec operation order
+        // for those rare shapes; ordinary arrays retain the allocation-free
+        // dense loop below.
+        if crate::array::array_iteration_is_exotic(arr) {
+            return reverse_array_spec_path(arr);
+        }
         let elements = (arr as *mut u8).add(std::mem::size_of::<ArrayHeader>()) as *mut f64;
         let mut i = 0usize;
         let mut j = len - 1;
@@ -240,6 +249,91 @@ pub extern "C" fn js_array_reverse(arr: *mut ArrayHeader) -> *mut ArrayHeader {
     }
 }
 
+/// ECMA-262 Array.prototype.reverse for an exotic real-array receiver.
+///
+/// Each pair is observed in the specified order: Has/Get lower, then Has/Get
+/// upper, followed by the corresponding Set/Delete operations. In particular,
+/// a lower getter is allowed to truncate the array before the upper presence
+/// check. The receiver and both read values stay rooted across every observable
+/// operation because an accessor can allocate and move them.
+unsafe fn reverse_array_spec_path(arr: *mut ArrayHeader) -> *mut ArrayHeader {
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let arr_handle = scope.root_raw_mut_ptr(arr);
+    let lower_value = scope.root_nanbox_f64(f64::from_bits(crate::value::TAG_UNDEFINED));
+    let upper_value = scope.root_nanbox_f64(f64::from_bits(crate::value::TAG_UNDEFINED));
+    let len = (*arr).length;
+
+    let mut lower = 0u32;
+    while lower < len / 2 {
+        let upper = len - lower - 1;
+
+        let lower_exists = arr_handle.with_mut_ptr::<ArrayHeader, _>(|current| {
+            crate::array::array_spec_has_index(current, lower)
+        });
+        if lower_exists {
+            let (value, _) = arr_handle.across_mut::<ArrayHeader, _>(|| {
+                arr_handle.with_mut_ptr(|current| crate::array::array_spec_get(current, lower))
+            });
+            lower_value.set_nanbox_f64(value);
+        }
+
+        let upper_exists = arr_handle.with_mut_ptr::<ArrayHeader, _>(|current| {
+            crate::array::array_spec_has_index(current, upper)
+        });
+        if upper_exists {
+            let (value, _) = arr_handle.across_mut::<ArrayHeader, _>(|| {
+                arr_handle.with_mut_ptr(|current| crate::array::array_spec_get(current, upper))
+            });
+            upper_value.set_nanbox_f64(value);
+        }
+
+        match (lower_exists, upper_exists) {
+            (true, true) => {
+                reverse_array_spec_set(&arr_handle, lower, &upper_value);
+                reverse_array_spec_set(&arr_handle, upper, &lower_value);
+            }
+            (false, true) => {
+                reverse_array_spec_set(&arr_handle, lower, &upper_value);
+                reverse_array_spec_delete(&arr_handle, upper);
+            }
+            (true, false) => {
+                reverse_array_spec_delete(&arr_handle, lower);
+                reverse_array_spec_set(&arr_handle, upper, &lower_value);
+            }
+            (false, false) => {}
+        }
+
+        lower += 1;
+    }
+
+    arr_handle.with_mut_ptr(|arr: *mut ArrayHeader| arr)
+}
+
+fn reverse_array_spec_set(
+    arr_handle: &crate::gc::RuntimeHandle<'_>,
+    index: u32,
+    value_handle: &crate::gc::RuntimeHandle<'_>,
+) {
+    let _ = arr_handle.across_mut::<ArrayHeader, _>(|| {
+        // Reload after entering the potentially collecting operation. A raw
+        // f64 copied out before this point would not be rewritten if it held a
+        // pointer to an object moved while resolving Array.prototype.
+        let value = value_handle.get_nanbox_f64();
+        arr_handle.with_mut_ptr(|current| {
+            crate::array::js_array_set_f64_extend(current, index, value);
+        });
+    });
+}
+
+fn reverse_array_spec_delete(arr_handle: &crate::gc::RuntimeHandle<'_>, index: u32) {
+    let (deleted, _) = arr_handle.across_mut::<ArrayHeader, _>(|| {
+        arr_handle.with_mut_ptr(|current| crate::array::js_array_delete(current, index))
+    });
+    if deleted == 0 {
+        reverse_throw_type_error(b"Cannot delete property");
+    }
+}
+
 /// `Array.prototype.reverse.call(value)` for generic array-like receivers.
 #[no_mangle]
 pub extern "C" fn js_array_reverse_value(receiver: f64) -> f64 {

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

@@ -537,6 +537,13 @@ pub(crate) unsafe fn array_named_property_delete(
     let Some(name) = string_header_as_str(key) else {
         return false;
     };
+    array_named_property_delete_by_name(arr, name)
+}
+
+pub(crate) unsafe fn array_named_property_delete_by_name(
+    arr: *const ArrayHeader,
+    name: &str,
+) -> bool {
     let arr = clean_arr_ptr(arr);
     if arr.is_null() {
         return false;

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

@@ -208,18 +208,19 @@ pub(crate) use self::alloc::{js_array_from_arraylike, js_array_from_string_codep
 pub(crate) use self::flat_clone::{dense_spread_copy, dense_spread_source, flattenable_array_ptr};
 pub(crate) use self::header::{
     array_byte_size, array_is_frozen, array_is_sealed_or_no_extend, array_named_property_delete,
-    array_named_property_get, array_named_property_get_by_name, array_named_property_has,
-    array_named_property_names, array_named_property_set, array_numeric_raw_f64_get,
-    array_numeric_raw_f64_push_inbounds, array_numeric_raw_f64_set_inbounds, array_object_flags,
-    array_object_flags_from_tag, array_object_flags_resolved, array_ptr_as_proxy,
-    array_receiver_addr, array_receiver_gc_tag, buffer_receiver_as_uint8_typed_array,
-    canonicalize_array_numeric_store_value, canonicalize_array_numeric_store_value_from_flags,
-    clean_arr_ptr, clean_arr_ptr_mut, clear_array_numeric_layout, clear_array_numeric_layout_ptr,
-    gc_element_slot_range, mark_array_layout_unknown, mark_array_raw_f64_holes_fresh,
-    normalize_array_receiver, note_array_slot, note_array_slot_layout_only, rebuild_array_layout,
-    rebuild_array_layout_exact, refresh_array_numeric_layout, replay_array_growth_write_barriers,
-    set_array_numeric_layout, store_array_slot, transfer_array_numeric_layout,
-    typed_array_receiver, value_bits_to_number, NumericArrayLayout, MIN_ARRAY_CAPACITY,
+    array_named_property_delete_by_name, array_named_property_get,
+    array_named_property_get_by_name, array_named_property_has, array_named_property_names,
+    array_named_property_set, array_numeric_raw_f64_get, array_numeric_raw_f64_push_inbounds,
+    array_numeric_raw_f64_set_inbounds, array_object_flags, array_object_flags_from_tag,
+    array_object_flags_resolved, array_ptr_as_proxy, array_receiver_addr, array_receiver_gc_tag,
+    buffer_receiver_as_uint8_typed_array, canonicalize_array_numeric_store_value,
+    canonicalize_array_numeric_store_value_from_flags, clean_arr_ptr, clean_arr_ptr_mut,
+    clear_array_numeric_layout, clear_array_numeric_layout_ptr, gc_element_slot_range,
+    mark_array_layout_unknown, mark_array_raw_f64_holes_fresh, normalize_array_receiver,
+    note_array_slot, note_array_slot_layout_only, rebuild_array_layout, rebuild_array_layout_exact,
+    refresh_array_numeric_layout, replay_array_growth_write_barriers, set_array_numeric_layout,
+    store_array_slot, transfer_array_numeric_layout, typed_array_receiver, value_bits_to_number,
+    NumericArrayLayout, MIN_ARRAY_CAPACITY,
 };
 
 // Sole caller is the regex-engine-gated `regex::exec_array`, so the helper and

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

@@ -940,13 +940,17 @@ pub extern "C" fn js_array_set_length(arr: *mut ArrayHeader, new_length: f64) {
             return;
         }
         if n < cur {
-            // Truncate: clear elements at indices [n..cur) to TAG_HOLE so
-            // any code that resurrects the slot via `arr[i]` reads `undefined`,
-            // not stale data. The capacity stays unchanged — JS doesn't
-            // require Perry to release the underlying buffer here, and growing
-            // back via `push` would just re-overwrite these slots anyway.
-            for i in n..cur {
-                note_array_slot(arr, i as usize, crate::value::TAG_HOLE);
+            // ArraySetLength deletes indices from high to low. This must clear
+            // descriptor-backed indices as well as dense slots: an accessor at
+            // index 0 cannot remain observable after its getter truncates the
+            // array to zero. If a non-configurable index blocks deletion, keep
+            // that index and restore length to index + 1 per §10.4.2.4.
+            for i in (n..cur).rev() {
+                if js_array_delete(arr, i) == 0 {
+                    (*arr).length = i + 1;
+                    refresh_array_numeric_layout(arr);
+                    return;
+                }
             }
             (*arr).length = n;
             refresh_array_numeric_layout(arr);
@@ -999,7 +1003,13 @@ pub extern "C" fn js_array_delete(arr: *mut ArrayHeader, index: u32) -> i32 {
                 return 0;
             }
         }
-        note_array_slot(arr, index as usize, crate::value::TAG_HOLE);
+        if index < (*arr).capacity {
+            note_array_slot(arr, index as usize, crate::value::TAG_HOLE);
+        }
+        // Sparse indices live in the named-property side table rather than the
+        // dense allocation. Always clear that representation too: a later
+        // growth can make a formerly sparse index fall below capacity.
+        array_named_property_delete_by_name(arr, &key);
         crate::object::clear_property_attrs(arr as usize, &key);
         crate::object::clear_accessor_descriptor(arr as usize, &key);
         1

crates/perry/tests/issue_5898_array_reverse_exotic.rs

@@ -0,0 +1,95 @@
+//! Regression coverage for the Array.prototype.reverse exotic-index cluster
+//! in #5898. Reverse must observe inherited indices and getter side effects in
+//! specification order instead of swapping raw dense slots.
+
+use std::path::PathBuf;
+use std::process::Command;
+
+fn perry_bin() -> PathBuf {
+    PathBuf::from(env!("CARGO_BIN_EXE_perry"))
+}
+
+#[test]
+fn reverse_observes_inherited_indices_and_live_presence() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let entry = dir.path().join("main.ts");
+    let output = dir.path().join("main_bin");
+    std::fs::write(
+        &entry,
+        r#"
+declare function gc(): void;
+
+(Array.prototype as any)[1] = 1;
+const inherited = [0];
+inherited.length = 2;
+inherited.reverse();
+console.log("inherited", inherited[0], inherited[1]);
+delete (Array.prototype as any)[1];
+
+const live = ["first", "second"];
+Object.defineProperty(live, 0, {
+  configurable: true,
+  get() {
+    live.length = 0;
+    return "first";
+  }
+});
+live.reverse();
+console.log("live", 0 in live, 1 in live, live[1]);
+
+const sparse: any[] = [];
+const sparseIndex = 1_001_025;
+sparse[sparseIndex] = "far";
+sparse.length = 0;
+console.log("sparse", sparse.length, sparseIndex in sparse);
+
+// A sparse named-property entry can become covered by dense capacity after a
+// push grows the backing store. Force a collection so the side-table owner is
+// rekeyed from the growth forwarding stub, then make sure deletion clears both
+// storage representations.
+const regrown: any[] = [];
+const farIndex = sparseIndex;
+regrown[farIndex] = "far";
+regrown.push("tail");
+gc();
+delete regrown[farIndex];
+console.log(
+  "regrown",
+  Object.prototype.hasOwnProperty.call(regrown, farIndex),
+  farIndex in regrown
+);
+"#,
+    )
+    .expect("write entry");
+
+    let compile = Command::new(perry_bin())
+        .current_dir(dir.path())
+        .arg("compile")
+        .arg(&entry)
+        .arg("-o")
+        .arg(&output)
+        .arg("--no-cache")
+        .output()
+        .expect("run perry compile");
+    assert!(
+        compile.status.success(),
+        "perry compile failed\nstdout:\n{}\nstderr:\n{}",
+        String::from_utf8_lossy(&compile.stdout),
+        String::from_utf8_lossy(&compile.stderr)
+    );
+
+    let run = Command::new(&output)
+        .current_dir(dir.path())
+        .output()
+        .expect("run compiled binary");
+    assert!(
+        run.status.success(),
+        "compiled binary failed\nstdout:\n{}\nstderr:\n{}",
+        String::from_utf8_lossy(&run.stdout),
+        String::from_utf8_lossy(&run.stderr)
+    );
+    assert_eq!(
+        String::from_utf8_lossy(&run.stdout),
+        "inherited 1 0\nlive false true first\nsparse 0 false\nregrown false false\n"
+    );
+}