From 0538a76aea96aeb5fb2c4c9f5288889c29317802 Mon Sep 17 00:00:00 2001 From: Andrew DiZenzo Date: Wed, 3 Jun 2026 09:45:02 +0000 Subject: [PATCH] Respect symbol enumerability in Object.assign --- crates/perry-runtime/src/object/alloc.rs | 3 + .../perry-runtime/src/object/descriptors.rs | 28 +++++ crates/perry-runtime/src/object/object_ops.rs | 50 ++++++-- crates/perry-runtime/src/object/tests.rs | 87 ++++++++++++++ crates/perry-runtime/src/symbol.rs | 113 ++++++++++++++++++ crates/perry-runtime/src/symbol/accessors.rs | 10 ++ .../object/assign-descriptor-toobject.ts | 103 ++++++++++++++++ 7 files changed, 381 insertions(+), 13 deletions(-) create mode 100644 test-parity/node-suite/object/assign-descriptor-toobject.ts diff --git a/crates/perry-runtime/src/object/alloc.rs b/crates/perry-runtime/src/object/alloc.rs index bf8971dfb..ab85972f1 100644 --- a/crates/perry-runtime/src/object/alloc.rs +++ b/crates/perry-runtime/src/object/alloc.rs @@ -805,6 +805,9 @@ pub unsafe extern "C" fn js_object_assign_one(target_f64: f64, source_f64: f64) // Mutex; holding the lock across the iteration would deadlock. let entries = crate::symbol::clone_symbol_entries_for_obj_ptr(src_raw); for (sym_ptr, value_bits) in entries { + if !crate::symbol::symbol_property_is_enumerable(src_raw, sym_ptr) { + continue; + } let sym_f64 = f64::from_bits(JSValue::pointer(sym_ptr as *const u8).bits()); let value_f64 = f64::from_bits(value_bits); crate::symbol::js_object_set_symbol_property(target_f64, sym_f64, value_f64); diff --git a/crates/perry-runtime/src/object/descriptors.rs b/crates/perry-runtime/src/object/descriptors.rs index f51376bb3..22def70d6 100644 --- a/crates/perry-runtime/src/object/descriptors.rs +++ b/crates/perry-runtime/src/object/descriptors.rs @@ -127,6 +127,34 @@ pub extern "C" fn js_object_get_own_property_descriptor(obj_value: f64, key_valu } } + if crate::symbol::js_is_symbol(key_value) != 0 { + let owner = crate::symbol::obj_key_from_f64(obj_value); + let sym_key = crate::symbol::sym_key_from_f64(key_value); + if owner == 0 || sym_key == 0 { + return f64::from_bits(crate::value::TAG_UNDEFINED); + } + let attrs = crate::symbol::get_symbol_property_attrs(owner, sym_key) + .unwrap_or(PropertyAttrs::new(true, true, true)); + if let Some((get, set)) = crate::symbol::symbol_accessor_descriptor_bits(owner, sym_key) + { + return build_accessor_descriptor( + f64::from_bits(get), + f64::from_bits(set), + attrs.enumerable(), + attrs.configurable(), + ); + } + if let Some(value_bits) = crate::symbol::symbol_property_root_bits(owner, sym_key) { + return build_data_descriptor( + f64::from_bits(value_bits), + attrs.writable(), + attrs.enumerable(), + attrs.configurable(), + ); + } + return f64::from_bits(crate::value::TAG_UNDEFINED); + } + if let Some(class_id) = class_ref_id(obj_value) { let method_name = metadata_key_to_string(key_value); if let Some(method_name) = method_name { diff --git a/crates/perry-runtime/src/object/object_ops.rs b/crates/perry-runtime/src/object/object_ops.rs index 29eb06e37..810a3dd11 100644 --- a/crates/perry-runtime/src/object/object_ops.rs +++ b/crates/perry-runtime/src/object/object_ops.rs @@ -1006,13 +1006,16 @@ pub extern "C" fn js_object_define_property( js_object_get_field_by_name(desc_ptr as *const ObjectHeader, get_key); let set_field = js_object_get_field_by_name(desc_ptr as *const ObjectHeader, set_key); - if !get_field.is_undefined() || !set_field.is_undefined() { - let get_bits = if get_field.is_undefined() { + let has_get = own_key_present(desc_ptr, get_key); + let has_set = own_key_present(desc_ptr, set_key); + let has_accessor = has_get || has_set; + if has_accessor { + let get_bits = if !has_get || get_field.is_undefined() { 0 } else { crate::closure::clone_closure_rebind_this(get_field.bits(), obj_value) }; - let set_bits = if set_field.is_undefined() { + let set_bits = if !has_set || set_field.is_undefined() { 0 } else { crate::closure::clone_closure_rebind_this(set_field.bits(), obj_value) @@ -1020,17 +1023,38 @@ pub extern "C" fn js_object_define_property( crate::symbol::set_symbol_accessor_property( obj_value, key_value, get_bits, set_bits, ); + } else { + let value_key = crate::string::js_string_from_bytes(b"value".as_ptr(), 5); + if own_key_present(desc_ptr, value_key) { + let value_field = js_object_get_field_by_name( + desc_ptr as *const ObjectHeader, + value_key, + ); + crate::symbol::js_object_set_symbol_property( + obj_value, + key_value, + f64::from_bits(value_field.bits()), + ); + } } - let value_key = crate::string::js_string_from_bytes(b"value".as_ptr(), 5); - let value_field = - js_object_get_field_by_name(desc_ptr as *const ObjectHeader, value_key); - if !value_field.is_undefined() { - crate::symbol::js_object_set_symbol_property( - obj_value, - key_value, - f64::from_bits(value_field.bits()), - ); - } + let read_bool = |name: &[u8]| -> Option { + let k = + crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + let v = js_object_get_field_by_name(desc_ptr as *const ObjectHeader, k); + if v.is_undefined() { + None + } else { + Some(crate::value::js_is_truthy(f64::from_bits(v.bits())) != 0) + } + }; + let writable = read_bool(b"writable").unwrap_or(has_accessor); + let enumerable = read_bool(b"enumerable").unwrap_or(false); + let configurable = read_bool(b"configurable").unwrap_or(false); + crate::symbol::set_symbol_property_attrs( + obj as usize, + raw_ptr as usize, + PropertyAttrs::new(writable, enumerable, configurable), + ); } return obj_value; } diff --git a/crates/perry-runtime/src/object/tests.rs b/crates/perry-runtime/src/object/tests.rs index 5d7b86d3b..1d32e4bff 100644 --- a/crates/perry-runtime/src/object/tests.rs +++ b/crates/perry-runtime/src/object/tests.rs @@ -147,6 +147,93 @@ fn closure_name_can_be_redefined_with_define_property() { } } +#[test] +fn symbol_define_property_attrs_round_trip_descriptor() { + crate::symbol::test_clear_symbol_side_table_roots(); + unsafe { + let obj = js_object_alloc(0, 0); + assert!(!obj.is_null()); + let obj_value = crate::value::js_nanbox_pointer(obj as i64); + let symbol_key = crate::symbol::js_symbol_new_empty(); + let symbol_ptr = crate::symbol::sym_key_from_f64(symbol_key); + assert_ne!(symbol_ptr, 0); + + let value_key = crate::string::js_string_from_bytes(b"value".as_ptr(), 5); + let writable_key = crate::string::js_string_from_bytes(b"writable".as_ptr(), 8); + let enumerable_key = crate::string::js_string_from_bytes(b"enumerable".as_ptr(), 10); + let configurable_key = crate::string::js_string_from_bytes(b"configurable".as_ptr(), 12); + + let descriptor = js_object_alloc(0, 0); + assert!(!descriptor.is_null()); + js_object_set_field_by_name(descriptor, value_key, 42.0); + js_object_set_field_by_name( + descriptor, + writable_key, + f64::from_bits(crate::value::TAG_FALSE), + ); + js_object_set_field_by_name( + descriptor, + enumerable_key, + f64::from_bits(crate::value::TAG_FALSE), + ); + js_object_set_field_by_name( + descriptor, + configurable_key, + f64::from_bits(crate::value::TAG_TRUE), + ); + + let descriptor_value = crate::value::js_nanbox_pointer(descriptor as i64); + js_object_define_property(obj_value, symbol_key, descriptor_value); + + assert_eq!( + crate::symbol::symbol_property_root_bits(obj as usize, symbol_ptr), + Some(42.0f64.to_bits()) + ); + assert!(!crate::symbol::symbol_property_is_enumerable( + obj as usize, + symbol_ptr + )); + + let own_descriptor = js_object_get_own_property_descriptor(obj_value, symbol_key); + let own_descriptor_obj = + crate::value::js_nanbox_get_pointer(own_descriptor) as *const ObjectHeader; + assert!(!own_descriptor_obj.is_null()); + let value = js_object_get_field_by_name(own_descriptor_obj, value_key); + assert!(value.is_number()); + assert_eq!(value.as_number(), 42.0); + assert_eq!( + js_object_get_field_by_name(own_descriptor_obj, writable_key).bits(), + crate::value::TAG_FALSE + ); + assert_eq!( + js_object_get_field_by_name(own_descriptor_obj, enumerable_key).bits(), + crate::value::TAG_FALSE + ); + assert_eq!( + js_object_get_field_by_name(own_descriptor_obj, configurable_key).bits(), + crate::value::TAG_TRUE + ); + + let attr_descriptor = js_object_alloc(0, 0); + assert!(!attr_descriptor.is_null()); + js_object_set_field_by_name( + attr_descriptor, + enumerable_key, + f64::from_bits(crate::value::TAG_TRUE), + ); + let attr_descriptor_value = crate::value::js_nanbox_pointer(attr_descriptor as i64); + js_object_define_property(obj_value, symbol_key, attr_descriptor_value); + assert_eq!( + crate::symbol::symbol_property_root_bits(obj as usize, symbol_ptr), + Some(42.0f64.to_bits()) + ); + assert!(crate::symbol::symbol_property_is_enumerable( + obj as usize, + symbol_ptr + )); + } +} + #[test] fn test_object_alloc_and_fields() { let obj = js_object_alloc(1, 3); diff --git a/crates/perry-runtime/src/symbol.rs b/crates/perry-runtime/src/symbol.rs index 64f454f5a..d7ea1d069 100644 --- a/crates/perry-runtime/src/symbol.rs +++ b/crates/perry-runtime/src/symbol.rs @@ -203,6 +203,12 @@ pub(crate) fn is_global_registered_symbol(ptr: usize) -> bool { // Storage stays intentionally linear because per-object symbol keys are rare. static SYMBOL_PROPERTIES: Mutex>>> = Mutex::new(None); +// Descriptor attributes for symbol-keyed properties installed through +// Object.defineProperty. Direct symbol assignment uses the normal data-property +// defaults, so absence here means writable/enumerable/configurable are all true. +static SYMBOL_PROPERTY_ATTRS: Mutex>> = + Mutex::new(None); + // Monotonic id counter for fresh symbols. Not thread-safe per-thread but // Symbol semantics are compatible with coarse locking. static NEXT_SYMBOL_ID: Mutex = Mutex::new(1); @@ -483,6 +489,50 @@ pub(crate) fn clone_symbol_entries_for_obj_ptr(src_obj_ptr: usize) -> Vec<(usize .unwrap_or_default() } +pub(crate) fn symbol_property_root_bits(owner: usize, sym_key: usize) -> Option { + let guard = crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTIES); + guard.as_ref().and_then(|map| { + map.get(&owner) + .and_then(|entries| entries.iter().find(|(key, _)| *key == sym_key)) + .map(|(_, value_bits)| *value_bits) + }) +} + +pub(crate) fn get_symbol_property_attrs( + owner: usize, + sym_key: usize, +) -> Option { + let guard = crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTY_ATTRS); + guard + .as_ref() + .and_then(|map| map.get(&(owner, sym_key)).copied()) +} + +pub(crate) fn set_symbol_property_attrs( + owner: usize, + sym_key: usize, + attrs: crate::object::PropertyAttrs, +) { + if owner == 0 || sym_key == 0 { + return; + } + let mut guard = crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTY_ATTRS); + if guard.is_none() { + *guard = Some(HashMap::new()); + } + guard.as_mut().unwrap().insert((owner, sym_key), attrs); +} + +pub(crate) fn symbol_property_is_enumerable(owner: usize, sym_key: usize) -> bool { + get_symbol_property_attrs(owner, sym_key) + .map(|attrs| attrs.enumerable()) + .unwrap_or(true) +} + +pub(crate) fn symbol_accessor_descriptor_bits(owner: usize, sym_key: usize) -> Option<(u64, u64)> { + accessors::symbol_accessor_property_by_key(owner, sym_key).map(|acc| (acc.get, acc.set)) +} + /// Extract the raw object pointer from a NaN-boxed JSValue. Returns 0 if the /// value isn't a pointer-tagged object (and 0 is also a valid "no entries" /// sentinel for the side table). @@ -764,6 +814,7 @@ pub fn scan_symbol_side_table_roots(mark: &mut dyn FnMut(f64)) { pub fn scan_symbol_side_table_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { scan_symbol_property_roots_mut(visitor); + scan_symbol_property_attrs_mut(visitor); accessors::scan_symbol_accessor_roots_mut(visitor); scan_class_static_symbol_roots_mut(visitor); scan_symbol_pointer_metadata_roots_mut(visitor); @@ -802,6 +853,31 @@ fn scan_symbol_property_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_ } } +fn scan_symbol_property_attrs_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + let mut rewrites = Vec::new(); + let mut guard = crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTY_ATTRS); + let Some(map) = guard.as_mut() else { + return; + }; + + for (old_owner, old_sym_key) in map.keys().copied().collect::>() { + let mut new_owner = old_owner; + let mut new_sym_key = old_sym_key; + let owner_changed = + visitor.visit_metadata_usize_slot(&mut new_owner) && new_owner != old_owner; + let sym_changed = visitor.visit_usize_slot(&mut new_sym_key) && new_sym_key != old_sym_key; + if owner_changed || sym_changed { + rewrites.push(((old_owner, old_sym_key), (new_owner, new_sym_key))); + } + } + + for (old_key, new_key) in rewrites { + if let Some(attrs) = map.remove(&old_key) { + map.insert(new_key, attrs); + } + } +} + fn scan_class_static_symbol_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { let mut key_rewrites = Vec::new(); let mut guard = crate::gc::lock_gc_root_registry(&CLASS_STATIC_SYMBOLS); @@ -851,6 +927,7 @@ fn scan_symbol_pointer_metadata_roots_mut(visitor: &mut crate::gc::RuntimeRootVi enum SymbolSideTableRootSlot { SymbolPropertyOwner { owner: usize }, SymbolPropertyEntry { owner: usize, sym_key: usize }, + SymbolPropertyAttrs { owner: usize, sym_key: usize }, ClassStaticSymbol { class_id: u32, sym_key: usize }, SymbolPointer { ptr: usize }, } @@ -898,6 +975,15 @@ fn symbol_side_table_root_snapshot() -> Vec { } } + { + let guard = crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTY_ATTRS); + if let Some(map) = guard.as_ref() { + for &(owner, sym_key) in map.keys() { + slots.push(SymbolSideTableRootSlot::SymbolPropertyAttrs { owner, sym_key }); + } + } + } + { let guard = crate::gc::lock_gc_root_registry(&CLASS_STATIC_SYMBOLS); if let Some(map) = guard.as_ref() { @@ -939,6 +1025,9 @@ fn scan_symbol_side_table_root_slot( visitor.visit_usize_slot(entry_sym); visitor.visit_nanbox_u64_slot(value_bits); } + SymbolSideTableRootSlot::SymbolPropertyAttrs { owner, sym_key } => { + rewrite_symbol_property_attrs_if_forwarded(visitor, owner, sym_key); + } SymbolSideTableRootSlot::ClassStaticSymbol { class_id, sym_key } => { rewrite_class_static_symbol_entry_if_forwarded(visitor, class_id, sym_key); } @@ -971,6 +1060,29 @@ fn rewrite_symbol_property_owner_if_forwarded( } } +fn rewrite_symbol_property_attrs_if_forwarded( + visitor: &mut crate::gc::RuntimeRootVisitor<'_>, + owner: usize, + sym_key: usize, +) { + let mut guard = crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTY_ATTRS); + let Some(map) = guard.as_mut() else { + return; + }; + if !map.contains_key(&(owner, sym_key)) { + return; + } + let mut new_owner = owner; + let mut new_sym_key = sym_key; + let owner_moved = visitor.visit_metadata_usize_slot(&mut new_owner); + let sym_moved = visitor.visit_usize_slot(&mut new_sym_key); + if (owner_moved && new_owner != owner) || (sym_moved && new_sym_key != sym_key) { + if let Some(attrs) = map.remove(&(owner, sym_key)) { + map.insert((new_owner, new_sym_key), attrs); + } + } +} + fn rewrite_class_static_symbol_entry_if_forwarded( visitor: &mut crate::gc::RuntimeRootVisitor<'_>, class_id: u32, @@ -1013,6 +1125,7 @@ fn rewrite_symbol_pointer_metadata_if_forwarded( #[cfg(test)] pub(crate) fn test_clear_symbol_side_table_roots() { *crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTIES) = None; + *crate::gc::lock_gc_root_registry(&SYMBOL_PROPERTY_ATTRS) = None; *crate::gc::lock_gc_root_registry(&CLASS_STATIC_SYMBOLS) = None; accessors::test_clear_symbol_accessor_roots(); diff --git a/crates/perry-runtime/src/symbol/accessors.rs b/crates/perry-runtime/src/symbol/accessors.rs index abc682342..2e3334109 100644 --- a/crates/perry-runtime/src/symbol/accessors.rs +++ b/crates/perry-runtime/src/symbol/accessors.rs @@ -78,6 +78,16 @@ pub(super) unsafe fn symbol_accessor_property( .and_then(|m| m.get(&(obj_key, sym_key)).copied()) } +pub(super) fn symbol_accessor_property_by_key( + obj_key: usize, + sym_key: usize, +) -> Option { + let guard = crate::gc::lock_gc_root_registry(&SYMBOL_ACCESSOR_PROPERTIES); + guard + .as_ref() + .and_then(|m| m.get(&(obj_key, sym_key)).copied()) +} + pub(super) unsafe fn invoke_symbol_accessor_getter(get_bits: u64, receiver: f64) -> f64 { if get_bits == 0 { return f64::from_bits(TAG_UNDEFINED); diff --git a/test-parity/node-suite/object/assign-descriptor-toobject.ts b/test-parity/node-suite/object/assign-descriptor-toobject.ts new file mode 100644 index 000000000..3407adfe2 --- /dev/null +++ b/test-parity/node-suite/object/assign-descriptor-toobject.ts @@ -0,0 +1,103 @@ +function show(label: string, fn: () => unknown) { + try { + console.log(label + ":", JSON.stringify(fn())); + } catch (err: any) { + console.log(label + ": throw", err.name); + } +} + +show("primitive target string", () => { + const result: any = Object.assign("ab" as any, { x: 1 }); + return [ + typeof result, + Object.prototype.toString.call(result), + result.valueOf(), + result.x, + Object.keys(result).join("|"), + result === "ab", + ]; +}); + +show("primitive target number", () => { + const result: any = Object.assign(5 as any, { x: 1 }); + return [ + typeof result, + Object.prototype.toString.call(result), + result.valueOf(), + result.x, + result === 5, + ]; +}); + +show("non-enumerable getter order", () => { + const log: string[] = []; + const source: any = {}; + Object.defineProperty(source, "hidden", { value: 1, enumerable: false }); + Object.defineProperty(source, "a", { + enumerable: true, + get() { + log.push("get-a"); + return 2; + }, + }); + source.b = 3; + const output: any = Object.assign({}, source); + return [Object.keys(output).join("|"), output.a, output.b, output.hidden, log.join("|")]; +}); + +show("getter abrupt partial", () => { + const target: any = { before: 1 }; + const source: any = { + a: 2, + get boom() { + throw new Error("boom"); + }, + after: 3, + }; + try { + Object.assign(target, source); + } catch (err: any) { + return [err.name, Object.keys(target).join("|"), target.a, target.after]; + } + return ["no throw", Object.keys(target).join("|"), target.a, target.after]; +}); + +show("readonly target abrupt", () => { + const target: any = {}; + Object.defineProperty(target, "a", { + value: 1, + writable: false, + enumerable: true, + configurable: true, + }); + try { + Object.assign(target, { a: 2, b: 3 }); + } catch (err: any) { + return [err.name, target.a, target.b, Object.keys(target).join("|")]; + } + return ["no throw", target.a, target.b, Object.keys(target).join("|")]; +}); + +show("symbols enumerable only", () => { + const visible = Symbol("visible"); + const hidden = Symbol("hidden"); + const source: any = { a: 1 }; + Object.defineProperty(source, visible, { value: 2, enumerable: true }); + Object.defineProperty(source, hidden, { value: 3, enumerable: false }); + const visibleDescriptor = Object.getOwnPropertyDescriptor(source, visible); + const hiddenDescriptor = Object.getOwnPropertyDescriptor(source, hidden); + const output: any = Object.assign({}, source); + return [ + output.a, + output[visible], + output[hidden], + Object.getOwnPropertySymbols(output).length, + visibleDescriptor?.enumerable, + hiddenDescriptor?.enumerable, + ]; +}); + +show("source string order", () => { + const output: any = Object.assign({ z: 0 }, "xy" as any, { a: 1 }); + return [Object.keys(output).join("|"), output[0], output[1], output.a]; +});