diff --git a/changelog.d/9932-typed-array-own-property-order.md b/changelog.d/9932-typed-array-own-property-order.md new file mode 100644 index 0000000000..9dcdf7aea3 --- /dev/null +++ b/changelog.d/9932-typed-array-own-property-order.md @@ -0,0 +1,5 @@ +### Fixed + +- Preserve property creation order when enumerating ordinary properties on + buffers, data views, and buffer-backed typed arrays. Updating a property now + keeps its position, while deleting and recreating it appends the key. diff --git a/crates/perry-runtime/src/buffer/own_props.rs b/crates/perry-runtime/src/buffer/own_props.rs index f022ab0f9b..64a114ee76 100644 --- a/crates/perry-runtime/src/buffer/own_props.rs +++ b/crates/perry-runtime/src/buffer/own_props.rs @@ -30,7 +30,13 @@ use std::collections::HashMap; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Mutex, OnceLock}; -type BufferProps = HashMap>; +#[derive(Default)] +struct BufferOwnProps { + values: HashMap, + order: Vec, +} + +type BufferProps = HashMap; fn buffer_props() -> &'static Mutex { static PROPS: OnceLock> = OnceLock::new(); @@ -93,10 +99,11 @@ pub fn buffer_define_own_data_prop(addr: usize, prop: &str, value: f64) { } BUFFER_OWN_PROPS_EVER.store(true, Ordering::Release); if let Ok(mut props) = buffer_props().lock() { - props - .entry(addr) - .or_default() - .insert(prop.to_string(), value.to_bits()); + let own = props.entry(addr).or_default(); + if !own.values.contains_key(prop) { + own.order.push(prop.to_string()); + } + own.values.insert(prop.to_string(), value.to_bits()); } } @@ -108,7 +115,12 @@ pub fn buffer_get_own_prop(addr: usize, prop: &str) -> Option { buffer_props() .lock() .ok() - .and_then(|props| props.get(&addr).and_then(|m| m.get(prop)).copied()) + .and_then(|props| { + props + .get(&addr) + .and_then(|own| own.values.get(prop)) + .copied() + }) .map(f64::from_bits) } @@ -140,8 +152,7 @@ pub fn buffer_read_own_prop(addr: usize, prop: &str) -> Option { buffer_get_own_prop(addr, prop) } -/// Every own dynamic prop key recorded for `addr`, in insertion-independent -/// (sorted) order. +/// Every own dynamic prop key recorded for `addr`, in property-creation order. /// /// #8149: `Object.keys` / `getOwnPropertyNames` / `for…in` need these. Before, /// the enumeration paths had no registered-buffer arm at all and walked a @@ -156,13 +167,11 @@ pub fn buffer_own_prop_names(addr: usize) -> Vec { if addr == 0 || !buffer_own_props_possible() { return Vec::new(); } - let mut names: Vec = buffer_props() + buffer_props() .lock() .ok() - .and_then(|props| props.get(&addr).map(|m| m.keys().cloned().collect())) - .unwrap_or_default(); - names.sort(); - names + .and_then(|props| props.get(&addr).map(|own| own.order.clone())) + .unwrap_or_default() } /// Whether the buffer carries any own dynamic prop under `prop`. @@ -183,10 +192,13 @@ pub fn buffer_delete_own_prop(addr: usize, prop: &str) -> bool { let Some(entries) = props.get_mut(&addr) else { return false; }; - let removed = entries.remove(prop).is_some(); + let removed = entries.values.remove(prop).is_some(); + if removed { + entries.order.retain(|key| key != prop); + } crate::object::clear_accessor_descriptor(addr, prop); crate::object::clear_property_attrs(addr, prop); - if entries.is_empty() { + if entries.values.is_empty() { props.remove(&addr); } removed @@ -210,7 +222,7 @@ pub fn scan_buffer_own_props_roots_mut(visitor: &mut crate::gc::RuntimeRootVisit }; let mut new_owner = owner; visitor.visit_metadata_usize_slot(&mut new_owner); - for bits in entries.values_mut() { + for bits in entries.values.values_mut() { let mut v = f64::from_bits(*bits); visitor.visit_nanbox_f64_slot(&mut v); *bits = v.to_bits(); @@ -253,3 +265,33 @@ pub fn clear_buffer_own_props(addr: usize) { props.remove(&addr); } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn own_property_names_preserve_creation_order() { + let owner_marker = Box::new(0_u8); + let owner = (&*owner_marker as *const u8) as usize; + + clear_buffer_own_props(owner); + buffer_define_own_data_prop(owner, "second", 2.0); + buffer_define_own_data_prop(owner, "first", 1.0); + buffer_define_own_data_prop(owner, "second", 22.0); + assert_eq!( + buffer_own_prop_names(owner), + ["second", "first"], + "updating a property must keep its original position" + ); + + assert!(buffer_delete_own_prop(owner, "second")); + buffer_define_own_data_prop(owner, "second", 222.0); + assert_eq!( + buffer_own_prop_names(owner), + ["first", "second"], + "deleting and recreating a property must append it" + ); + clear_buffer_own_props(owner); + } +} diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index 1d84623080..beb29a4cf6 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -1154,10 +1154,9 @@ pub(super) fn strip_nanbox_addr(obj: *const ObjectHeader) -> usize { /// `Object.keys(new DataView(new ArrayBuffer(8)))` in any program that had also /// allocated a `Buffer`. /// -/// Expando ordering among the non-index keys is alphabetical, not insertion -/// order: `buffer::own_props` is a `HashMap`, so insertion order was never -/// recorded. Node uses insertion order. Deterministic-but-different beats the -/// previous nondeterministic-and-crashing. +/// Expando ordering among the non-index keys follows property creation order, +/// recorded by `buffer::own_props`. Canonical indices are still separated and +/// sorted below, as required by `OrdinaryOwnPropertyKeys`. pub(crate) fn registered_buffer_own_keys(addr: usize) -> Option> { if addr == 0 || !crate::buffer::is_registered_buffer(addr) { return None;