From 0c35ad0b47a60bdfe3e13d0638f285951cb44608 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 28 Jun 2026 17:22:31 +0000 Subject: [PATCH 1/2] fix(runtime): improve test262 built-ins/Object parity (#5588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three targeted fixes that close a cluster of test262 built-ins/Object failures found in issue #5588: 1. `closure_dynamic_enumerable_props` (enumeration.rs): `Object.keys/ values/entries` on a function now includes `length` and `name` when they have been explicitly made enumerable via `Object.defineProperty`. Previously those built-in function properties were only surfaced via the direct field-read path, so a descriptor-only redefinition (no `value` written to the dynamic-props side table) was silently dropped. Fix: check the descriptor side table for `length`/`name` first; if enumerable, compute the value from the same cascade as the field-read path (dynamic-prop override → bound arity → builtin length → closure arity) and prepend them before user-added dynamic props. 2. `js_object_default_to_locale_string` (object_proto.rs): spec step 2 of `Object.prototype.toLocaleString` is `Invoke(O, "toString")` where O is the raw `this` (primitive, not ToObject'd). A strict callee on the patched prototype must receive the original primitive as `this` (`typeof this === "boolean"` for `toLocaleString.call(true)` after patching `Boolean.prototype.toString`). The old code called the generic `js_native_call_method` which hard-codes the native toString result. Fix: check `builtin_proto_user_method` for a user patch first; if present, dispatch via `call_primitive_closure_value` which correctly applies strict/non-strict `this` binding. 3. `js_object_set_prototype_of` (define_properties.rs): add the OrdinarySetPrototypeOf step-7 prototype-cycle check. Walking the chain of the proposed new prototype and comparing each ancestor against the target object throws `TypeError: Cyclic __proto__ value` when a cycle would be created. Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_01JWENcSJa3zenoQyBXXCF5j --- .../src/object/field_get_set/enumeration.rs | 62 ++++++++++++++++++- .../object/native_call_method/object_proto.rs | 45 +++++++++++--- .../object/object_ops/define_properties.rs | 20 ++++++ 3 files changed, 119 insertions(+), 8 deletions(-) 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 75190e559..cae275099 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -243,14 +243,71 @@ pub extern "C" fn js_for_in_keys_value(value: f64) -> *mut ArrayHeader { } fn closure_dynamic_enumerable_props(ptr: usize) -> Vec<(String, f64)> { - let mut props = crate::closure::closure_dynamic_props_snapshot(ptr) + let mut props: Vec<(String, f64)> = Vec::new(); + + // Built-in function properties `length` and `name` are non-enumerable by + // default. If the caller redefined them via `Object.defineProperty` with + // `enumerable: true`, include them here BEFORE user-added dynamic props + // so their relative order matches the spec insertion order (built-ins + // precede dynamically-added own properties). + for builtin_key in &["length", "name"] { + if crate::closure::closure_is_key_deleted(ptr, builtin_key) { + continue; + } + // Only include if the side table explicitly marks them enumerable. + // Default (no entry in descriptor side table) = non-enumerable for + // built-in function properties. + if !get_property_attrs(ptr, builtin_key) + .map(|attrs| attrs.enumerable()) + .unwrap_or(false) + { + continue; + } + // Value: prefer a side-table override written by defineProperty, then + // fall back to the built-in computed value so Object.keys / entries + // returns the right thing even when defineProperty only changed attrs. + let val = crate::closure::closure_get_dynamic_prop(ptr, builtin_key); + let value = if val.to_bits() != crate::value::TAG_UNDEFINED { + f64::from_bits(val.to_bits()) + } else if *builtin_key == "length" { + let closure_value = crate::value::js_nanbox_pointer(ptr as i64); + let len = unsafe { + super::super::native_module::bound_native_callable_value_arity(closure_value) + } + .map(|a| a as f64) + .or_else(|| super::super::native_module::builtin_closure_length(ptr).map(|l| l as f64)) + .or_else(|| { + crate::closure::closure_length(ptr as *const crate::closure::ClosureHeader) + .map(|l| l as f64) + }) + .unwrap_or(0.0); + len + } else { + // "name" + let func_ptr = + unsafe { (*(ptr as *const crate::closure::ClosureHeader)).func_ptr as usize }; + let fname = crate::builtins::function_name_for_ptr(func_ptr).unwrap_or_default(); + let s = crate::string::js_string_from_bytes(fname.as_ptr(), fname.len() as u32); + f64::from_bits(JSValue::string_ptr(s).bits()) + }; + props.push((builtin_key.to_string(), value)); + } + + // User-added dynamic props (skip "length"/"name" — handled above so we + // don't double-count if defineProperty also wrote a value to dynamic props). + let user_props = crate::closure::closure_dynamic_props_snapshot(ptr) .into_iter() .filter(|(name, _)| { + if matches!(name.as_str(), "length" | "name") { + return false; + } get_property_attrs(ptr, name) .map(|attrs| attrs.enumerable()) .unwrap_or(true) }) .collect::>(); + props.extend(user_props); + for name in super::super::accessor_descriptor_keys_for_obj(ptr) { if props.iter().any(|(existing, _)| existing == &name) { continue; @@ -258,6 +315,9 @@ fn closure_dynamic_enumerable_props(ptr: usize) -> Vec<(String, f64)> { if crate::closure::closure_is_key_deleted(ptr, &name) { continue; } + if matches!(name.as_str(), "length" | "name") { + continue; + } if get_property_attrs(ptr, &name) .map(|attrs| attrs.enumerable()) .unwrap_or(false) diff --git a/crates/perry-runtime/src/object/native_call_method/object_proto.rs b/crates/perry-runtime/src/object/native_call_method/object_proto.rs index a9c0a1a1b..42eda0ecb 100644 --- a/crates/perry-runtime/src/object/native_call_method/object_proto.rs +++ b/crates/perry-runtime/src/object/native_call_method/object_proto.rs @@ -95,13 +95,44 @@ pub(crate) unsafe fn js_object_default_to_locale_string(receiver: f64) -> f64 { return crate::temporal::dispatch::call_method(receiver, "toLocaleString", &[]); } if !jsval.is_pointer() { - return js_native_call_method( - receiver, - b"toString".as_ptr() as *const i8, - "toString".len(), - std::ptr::null(), - 0, - ); + // Spec 20.1.3.6 Object.prototype.toLocaleString: step 1 is "Let O be + // the this value" (NOT ToObject), step 2 is "Return ? Invoke(O, + // 'toString')". Invoke resolves the method on the primitive's prototype + // chain and calls it with the original primitive as `this`. A + // user-patched Boolean/Number/BigInt/String prototype toString must be + // honoured, and a strict callee must receive the raw primitive (not a + // boxed wrapper) — call_primitive_closure_value handles both. + let builtin_name: &[u8] = if jsval.is_bool() { + b"Boolean" + } else if jsval.is_bigint() { + b"BigInt" + } else if jsval.is_any_string() { + b"String" + } else if unsafe { crate::symbol::js_is_symbol(receiver) } != 0 { + b"Symbol" + } else { + b"" + }; + if !builtin_name.is_empty() { + if let Some(patched) = + unsafe { super::builtin_proto_user_method(builtin_name, "toString") } + { + if let Some(result) = + unsafe { call_primitive_closure_value(receiver, patched, std::ptr::null(), 0) } + { + return result; + } + } + } + return unsafe { + js_native_call_method( + receiver, + b"toString".as_ptr() as *const i8, + "toString".len(), + std::ptr::null(), + 0, + ) + }; } // An own `toLocaleString` closure wins over the default rendering — // notably `%TypedArray%.prototype.toLocaleString()` invoked as a method ON diff --git a/crates/perry-runtime/src/object/object_ops/define_properties.rs b/crates/perry-runtime/src/object/object_ops/define_properties.rs index 1fff03b37..dd0682e24 100644 --- a/crates/perry-runtime/src/object/object_ops/define_properties.rs +++ b/crates/perry-runtime/src/object/object_ops/define_properties.rs @@ -196,6 +196,26 @@ pub extern "C" fn js_object_set_prototype_of(obj_value: f64, proto: f64) -> f64 return obj_value; } + // OrdinarySetPrototypeOf step 7: detect prototype cycles. + // Walk the prototype chain of the proposed new prototype; if any ancestor + // equals the target object, setting the prototype would form a cycle. + if !proto_is_null { + const TAG_NULL_U64: u64 = 0x7FFC_0000_0000_0002; + let mut p_bits = proto_bits; + loop { + if p_bits == obj_bits { + throw_object_type_error(b"Cyclic __proto__ value"); + } + let p_val = f64::from_bits(p_bits); + let next = js_object_get_prototype_of(p_val); + let next_bits = next.to_bits(); + if next_bits == TAG_NULL_U64 || next_bits == p_bits { + break; + } + p_bits = next_bits; + } + } + // #2820: setting the prototype of a primitive target is a spec no-op that // returns the (boxed) primitive value. `value_is_object_like` is false for // numbers/strings/booleans, and class refs are handled by the recording From 1724d9a8726f942b1828e7d18009f2ad1a7cbf8c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 28 Jun 2026 17:39:14 +0000 Subject: [PATCH 2/2] fix: address CodeRabbit review issues from PR #5763 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness fixes raised in the code review: 1. enumeration.rs: use `closure_has_own_dynamic_prop` to separate presence from value in `closure_dynamic_enumerable_props`. The previous `closure_get_dynamic_prop` sentinel (TAG_UNDEFINED) conflated "no dynamic override" with "dynamic override whose value is undefined", and also invoked getters in a keys-only path. 2. object_proto.rs: symbols are POINTER-tagged, so the `!jsval.is_pointer()` guard made the `js_is_symbol` branch inside it dead code. Compute `is_symbol` before the guard and extend the condition to `!jsval.is_pointer() || is_symbol` so Symbol.prototype.toString dispatch is reachable. 3. define_properties.rs: replace the single-step self-edge cycle check in `js_object_set_prototype_of` with Floyd's tortoise-and-hare algorithm so a pre-existing multi-node cycle (A→B→A) terminates the walk instead of looping forever. --- .../src/object/field_get_set/enumeration.rs | 9 +++-- .../object/native_call_method/object_proto.rs | 7 +++- .../object/object_ops/define_properties.rs | 37 +++++++++++++++---- 3 files changed, 41 insertions(+), 12 deletions(-) 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 cae275099..00fd06b34 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -266,9 +266,12 @@ fn closure_dynamic_enumerable_props(ptr: usize) -> Vec<(String, f64)> { // Value: prefer a side-table override written by defineProperty, then // fall back to the built-in computed value so Object.keys / entries // returns the right thing even when defineProperty only changed attrs. - let val = crate::closure::closure_get_dynamic_prop(ptr, builtin_key); - let value = if val.to_bits() != crate::value::TAG_UNDEFINED { - f64::from_bits(val.to_bits()) + // Use `closure_has_own_dynamic_prop` to distinguish "has an explicit + // dynamic value (possibly undefined)" from "no override" — using + // `closure_get_dynamic_prop` as a sentinel conflates both cases and + // also invokes getters, which is wrong for the keys-only path. + let value = if crate::closure::closure_has_own_dynamic_prop(ptr, builtin_key) { + f64::from_bits(crate::closure::closure_get_dynamic_prop(ptr, builtin_key).to_bits()) } else if *builtin_key == "length" { let closure_value = crate::value::js_nanbox_pointer(ptr as i64); let len = unsafe { diff --git a/crates/perry-runtime/src/object/native_call_method/object_proto.rs b/crates/perry-runtime/src/object/native_call_method/object_proto.rs index 42eda0ecb..9b2f8862b 100644 --- a/crates/perry-runtime/src/object/native_call_method/object_proto.rs +++ b/crates/perry-runtime/src/object/native_call_method/object_proto.rs @@ -94,7 +94,10 @@ pub(crate) unsafe fn js_object_default_to_locale_string(receiver: f64) -> f64 { if crate::temporal::is_temporal_value(receiver) { return crate::temporal::dispatch::call_method(receiver, "toLocaleString", &[]); } - if !jsval.is_pointer() { + // Symbols are POINTER-tagged, so `!jsval.is_pointer()` would be false for + // them — check before the pointer guard so the branch is reachable. + let is_symbol = unsafe { crate::symbol::js_is_symbol(receiver) } != 0; + if !jsval.is_pointer() || is_symbol { // Spec 20.1.3.6 Object.prototype.toLocaleString: step 1 is "Let O be // the this value" (NOT ToObject), step 2 is "Return ? Invoke(O, // 'toString')". Invoke resolves the method on the primitive's prototype @@ -108,7 +111,7 @@ pub(crate) unsafe fn js_object_default_to_locale_string(receiver: f64) -> f64 { b"BigInt" } else if jsval.is_any_string() { b"String" - } else if unsafe { crate::symbol::js_is_symbol(receiver) } != 0 { + } else if is_symbol { b"Symbol" } else { b"" diff --git a/crates/perry-runtime/src/object/object_ops/define_properties.rs b/crates/perry-runtime/src/object/object_ops/define_properties.rs index dd0682e24..974b64fce 100644 --- a/crates/perry-runtime/src/object/object_ops/define_properties.rs +++ b/crates/perry-runtime/src/object/object_ops/define_properties.rs @@ -199,20 +199,43 @@ pub extern "C" fn js_object_set_prototype_of(obj_value: f64, proto: f64) -> f64 // OrdinarySetPrototypeOf step 7: detect prototype cycles. // Walk the prototype chain of the proposed new prototype; if any ancestor // equals the target object, setting the prototype would form a cycle. + // Use Floyd's tortoise-and-hare so a pre-existing multi-node cycle in the + // chain (A→B→A) terminates instead of looping forever. The `tortoise` + // advances one step; the `hare` advances two. If they meet, the chain is + // cyclic and contains a loop (so it will never reach null), meaning we also + // can't form a fresh cycle by setting obj's proto to `proto`. if !proto_is_null { const TAG_NULL_U64: u64 = 0x7FFC_0000_0000_0002; - let mut p_bits = proto_bits; + let advance = |bits: u64| -> u64 { + let val = f64::from_bits(bits); + let next = js_object_get_prototype_of(val); + let nb = next.to_bits(); + if nb == TAG_NULL_U64 { + TAG_NULL_U64 + } else { + nb + } + }; + let mut tortoise = proto_bits; + let mut hare = proto_bits; loop { - if p_bits == obj_bits { + // Check current tortoise position first (catches `proto == obj` + // on the very first iteration without an extra advance). + if tortoise == obj_bits { throw_object_type_error(b"Cyclic __proto__ value"); } - let p_val = f64::from_bits(p_bits); - let next = js_object_get_prototype_of(p_val); - let next_bits = next.to_bits(); - if next_bits == TAG_NULL_U64 || next_bits == p_bits { + if tortoise == TAG_NULL_U64 { + break; + } + // Advance tortoise one step, hare two steps. + tortoise = advance(tortoise); + hare = advance(advance(hare)); + // If they meet, the existing chain already has a cycle — the walk + // will never reach null, so we also can never form a new one by + // setting obj's proto. Just break; the set is safe. + if hare == tortoise { break; } - p_bits = next_bits; } }