land #8761: honor exotic indices in Array shift

changelog.d/8761-array-shift-exotic-indices.md

@@ -0,0 +1,11 @@
+Fixed `Array.prototype.shift` on arrays whose indexed operations are
+observable. The dense `memmove` fast path is kept for ordinary arrays, but a
+receiver carrying index accessors / custom-attribute descriptors, sparse
+storage past the dense backing store, or an indexed property inherited from
+`Array.prototype` / `Object.prototype` now runs the specified live
+`HasProperty` / `Get` / `Set` / `Delete` sequence, so inherited indices,
+holes and getter side effects (freezing the receiver, or making `length`
+non-writable before the final `Set(O, "length", …)`) are all observed in spec
+order. The dense path additionally translates the internal `TAG_HOLE`
+sentinel to `undefined`, and the spec path roots the receiver and the carried
+values across every accessor that can allocate or move them.

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

@@ -1110,8 +1110,19 @@ pub extern "C" fn js_array_shift_f64(arr: *mut ArrayHeader) -> f64 {
             return TAG_UNDEFINED_F64;
         }
 
+        // A raw memmove is only equivalent to Shift when every observable
+        // indexed operation is an ordinary dense-array access. Indexed
+        // descriptors and prototype properties require the specified live
+        // HasProperty/Get/Set/Delete order; their accessors can also freeze the
+        // receiver or make `length` non-writable before the final length Set.
+        if crate::array::array_iteration_is_exotic(arr) {
+            return shift_array_spec_path(arr);
+        }
+
+        // `TAG_HOLE` is an internal storage sentinel. Even on the dense path,
+        // Get(O, "0") must expose it as `undefined`.
+        let value = crate::array::js_array_get_f64(arr, 0);
         let elements_ptr = (arr as *mut u8).add(std::mem::size_of::<ArrayHeader>()) as *mut f64;
-        let value = *elements_ptr;
 
         // Shift all elements down
         // GC_STORE_AUDIT(BARRIERED): shift memmove is followed by layout/barrier rebuild.
@@ -1122,6 +1133,77 @@ pub extern "C" fn js_array_shift_f64(arr: *mut ArrayHeader) -> f64 {
     }
 }
 
+/// ECMA-262 Array.prototype.shift for a real array whose indexed operations
+/// are observable. The loop keeps the original length while consulting live
+/// presence and values, and roots both the receiver and carried values across
+/// accessors which may allocate or move either one.
+unsafe fn shift_array_spec_path(arr: *mut ArrayHeader) -> f64 {
+    let scope = crate::gc::RuntimeHandleScope::new();
+    let arr_handle = scope.root_raw_mut_ptr(arr);
+    let first_handle = scope.root_nanbox_f64(f64::from_bits(crate::value::TAG_UNDEFINED));
+    let from_value_handle = scope.root_nanbox_f64(f64::from_bits(crate::value::TAG_UNDEFINED));
+    let len = (*arr).length;
+
+    let (first, _) = arr_handle.across_mut::<ArrayHeader, _>(|| {
+        arr_handle.with_mut_ptr(|current| crate::array::array_spec_get(current, 0))
+    });
+    first_handle.set_nanbox_f64(first);
+
+    for from in 1..len {
+        let from_present = arr_handle.with_mut_ptr::<ArrayHeader, _>(|current| {
+            crate::array::array_spec_has_index(current, from)
+        });
+        let to = from - 1;
+        if from_present {
+            let (value, _) = arr_handle.across_mut::<ArrayHeader, _>(|| {
+                arr_handle.with_mut_ptr(|current| crate::array::array_spec_get(current, from))
+            });
+            from_value_handle.set_nanbox_f64(value);
+            shift_array_spec_set(&arr_handle, to, &from_value_handle);
+        } else {
+            shift_array_spec_delete(&arr_handle, to);
+        }
+    }
+
+    shift_array_spec_delete(&arr_handle, len - 1);
+
+    // Set(O, "length", len - 1, true) occurs after every indexed operation.
+    // Re-read the receiver state because any getter/setter above may have
+    // frozen it or replaced `length` with a non-writable descriptor.
+    arr_handle.with_mut_ptr::<ArrayHeader, _>(|current| {
+        let current = clean_arr_ptr_mut(current);
+        if array_is_frozen(current) {
+            throw_frozen_array_mutation();
+        }
+        guard_writable_length(current);
+        (*current).length = len - 1;
+        rebuild_array_layout(current);
+    });
+    first_handle.get_nanbox_f64()
+}
+
+fn shift_array_spec_set(
+    arr_handle: &crate::gc::RuntimeHandle<'_>,
+    index: u32,
+    value_handle: &crate::gc::RuntimeHandle<'_>,
+) {
+    let _ = arr_handle.across_mut::<ArrayHeader, _>(|| {
+        let value = value_handle.get_nanbox_f64();
+        arr_handle.with_mut_ptr(|current| {
+            crate::array::js_array_set_f64_extend(current, index, value);
+        });
+    });
+}
+
+fn shift_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 {
+        throw_cannot_delete_array_index(index);
+    }
+}
+
 /// Unshift an element to the beginning of an array, growing if needed
 /// Returns a pointer to the (possibly reallocated) array
 #[no_mangle]

crates/perry/tests/issue_5898_array_shift_exotic.rs

@@ -0,0 +1,118 @@
+//! Regression coverage for the Array.prototype.shift exotic-index cluster in
+//! #5898. Shift must use live ordinary-property operations and observe indexed
+//! getter side effects before setting the final array length.
+
+use std::path::PathBuf;
+use std::process::Command;
+
+fn perry_bin() -> PathBuf {
+    PathBuf::from(env!("CARGO_BIN_EXE_perry"))
+}
+
+#[test]
+fn shift_observes_inherited_indices_holes_and_length_side_effects() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    let entry = dir.path().join("main.ts");
+    let output = dir.path().join("main_bin");
+    let runtime_dir = perry_bin()
+        .parent()
+        .expect("perry binary directory")
+        .to_path_buf();
+    std::fs::write(
+        &entry,
+        r#"
+(Array.prototype as any)[1] = 1;
+const inherited = [0];
+inherited.length = 2;
+console.log("inherited", inherited.shift(), inherited[0], inherited[1]);
+delete (Array.prototype as any)[1];
+
+const holey: any[] = [];
+holey[0] = 0;
+holey[3] = 3;
+console.log("holey-first", holey.shift(), holey.length, holey[0], holey[2]);
+holey.length = 1;
+console.log("holey-second", holey.shift(), holey.length);
+
+const frozen: any[] = new Array(1);
+let frozenGetterCalls = 0;
+Object.defineProperty(Array.prototype, "0", {
+  configurable: true,
+  get() {
+    Object.freeze(frozen);
+    frozenGetterCalls++;
+  }
+});
+try {
+  frozen.shift();
+  console.log("frozen no throw");
+} catch (error) {
+  console.log("frozen", error instanceof TypeError, frozen.length, frozenGetterCalls);
+}
+delete (Array.prototype as any)[0];
+
+const readonlyLength: any[] = new Array(1);
+let readonlyGetterCalls = 0;
+Object.defineProperty(Array.prototype, "0", {
+  configurable: true,
+  get() {
+    Object.defineProperty(readonlyLength, "length", { writable: false });
+    readonlyGetterCalls++;
+  }
+});
+try {
+  readonlyLength.shift();
+  console.log("readonly no throw");
+} catch (error) {
+  console.log(
+    "readonly",
+    error instanceof TypeError,
+    readonlyLength.length,
+    readonlyGetterCalls
+  );
+}
+delete (Array.prototype as any)[0];
+"#,
+    )
+    .expect("write entry");
+
+    let compile = Command::new(perry_bin())
+        .current_dir(dir.path())
+        .arg("compile")
+        .arg(&entry)
+        .arg("-o")
+        .arg(&output)
+        .arg("--no-cache")
+        .env("PERRY_LIB_DIR", &runtime_dir)
+        .env("PERRY_NO_AUTO_OPTIMIZE", "1")
+        .env("PERRY_RS4GC", "0")
+        .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),
+        concat!(
+            "inherited 0 1 1\n",
+            "holey-first 0 3 undefined 3\n",
+            "holey-second undefined 0\n",
+            "frozen true 1 1\n",
+            "readonly true 1 1\n",
+        )
+    );
+}