diff --git a/changelog.d/8400-array-reverse-exotic-indices.md b/changelog.d/8400-array-reverse-exotic-indices.md new file mode 100644 index 00000000000..3b76ccb6813 --- /dev/null +++ b/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. diff --git a/crates/perry-runtime/src/array/concat_reverse.rs b/crates/perry-runtime/src/array/concat_reverse.rs index 116042335e9..f99fd0850bf 100644 --- a/crates/perry-runtime/src/array/concat_reverse.rs +++ b/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::()) 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::(|current| { + crate::array::array_spec_has_index(current, lower) + }); + if lower_exists { + let (value, _) = arr_handle.across_mut::(|| { + 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::(|current| { + crate::array::array_spec_has_index(current, upper) + }); + if upper_exists { + let (value, _) = arr_handle.across_mut::(|| { + 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::(|| { + // 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::(|| { + 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 { diff --git a/crates/perry-runtime/src/array/header.rs b/crates/perry-runtime/src/array/header.rs index 16ca4f2c4d1..06d51cbc617 100644 --- a/crates/perry-runtime/src/array/header.rs +++ b/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; diff --git a/crates/perry-runtime/src/array/mod.rs b/crates/perry-runtime/src/array/mod.rs index 45b5af5e335..3faa98fd600 100644 --- a/crates/perry-runtime/src/array/mod.rs +++ b/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 diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index 5ed024a0bd5..183714dde34 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/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 diff --git a/crates/perry/tests/issue_5898_array_reverse_exotic.rs b/crates/perry/tests/issue_5898_array_reverse_exotic.rs new file mode 100644 index 00000000000..57460e85271 --- /dev/null +++ b/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" + ); +}