fix(runtime,hir): observable spec-order argument coercion for reflective String.prototype methods (#5902)

changelog.d/7451-string-reflective-arg-coercion.md

@@ -0,0 +1 @@
+fix(runtime,hir): reflective `String.prototype` methods now apply the observable spec coercions to their arguments, in spec order (#5902). `dispatch_string`'s `replace`/`replaceAll` arm extracted only already-string arguments — a number pattern was a silent no-op, a `{valueOf(){throw}}` pattern never threw — and now runs `ToString(searchValue)` before `ToString(replaceValue)` (§22.1.3.19/20) with RegExp patterns and callable replacements classified first, un-coerced. `padStart`/`padEnd` read the raw NaN-boxed maxLength (any object → NaN → target 0, its `valueOf` never invoked) and coerced the fill first; now `ToNumber(maxLength)` runs first with the spec early-return before `ToString(fillString)`. `lastIndexOf` gets a real `ToNumber(position)`, and every `indexOf`-family sub-arm re-extracts its needle/receiver pointers from their GC roots only after the observable coercions (a rooted-pointer-goes-stale hazard). `call_replace_callback` ToString-coerces the replacer-function result instead of dropping every non-string return to `""` (`undefined` → `"undefined"`). On the HIR side, `is_primitive_wrapper_brand_method` gains the `String` arm so `String.prototype.toString`/`valueOf` `.call(x)` (direct or via an extracted local) stays reflective and reaches the `thisStringValue` brand thunks instead of folding to `x.toString()` — Number/Boolean have been guarded since #4100. test262 `built-ins/String`: pass 903→916, runtime-fail 29→16 (13 flips, zero regressions), parity 96.9%→98.3%. New per-PR coverage: 6 behavioral unit tests on `js_native_call_method` plus a `perry-hir` fold-predicate test.

crates/perry-hir/src/lower/expr_call/intrinsics/apply_call.rs

@@ -319,6 +319,13 @@ fn is_primitive_wrapper_brand_method(recv: &ast::Expr, method: &str) -> bool {
     match base.sym.as_ref() {
         "Number" => matches!(method, "valueOf" | "toString" | "toLocaleString"),
         "Boolean" => matches!(method, "valueOf" | "toString"),
+        // `String.prototype.toString`/`valueOf` are `thisStringValue` brand
+        // checks (ECMA-262 §22.1.3), NOT ToString-coercing like the rest of
+        // `String.prototype` — the installed thunks throw a `TypeError` for a
+        // non-String receiver, so `String.prototype.toString.call(42)` (and
+        // the extracted-local form) must stay reflective (test262
+        // built-ins/String/prototype/{toString,valueOf}/non-generic.js).
+        "String" => matches!(method, "valueOf" | "toString"),
         _ => false,
     }
 }
@@ -785,3 +792,46 @@ fn is_builtin_prototype_receiver(ctx: &LoweringContext, recv: &ast::Expr) -> boo
         _ => false,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use swc_common::DUMMY_SP;
+
+    fn prototype_member(base: &str) -> ast::Expr {
+        ast::Expr::Member(ast::MemberExpr {
+            span: DUMMY_SP,
+            obj: Box::new(ast::Expr::Ident(ast::Ident::new(
+                base.into(),
+                DUMMY_SP,
+                Default::default(),
+            ))),
+            prop: ast::MemberProp::Ident(ast::IdentName {
+                span: DUMMY_SP,
+                sym: "prototype".into(),
+            }),
+        })
+    }
+
+    /// #5902: `String.prototype.toString`/`valueOf` are `thisStringValue`
+    /// brand checks — the `.call`/`.apply` fold (and the extracted-local
+    /// tracking in `as_builtin_proto_method_ref`, which consults the same
+    /// predicate) must leave them reflective so the runtime thunk can throw
+    /// a TypeError on a non-String receiver (test262 non-generic.js).
+    /// Deleting the `"String"` arm from `is_primitive_wrapper_brand_method`
+    /// turns this red.
+    #[test]
+    fn string_to_string_and_value_of_are_brand_methods() {
+        let recv = prototype_member("String");
+        assert!(is_primitive_wrapper_brand_method(&recv, "toString"));
+        assert!(is_primitive_wrapper_brand_method(&recv, "valueOf"));
+        // The ToString-coercing generics are guarded by the sibling
+        // `is_string_prototype_generic_method` predicate, not the brand one —
+        // and the brand pair must stay OUT of the generic list (a generic
+        // thunk would coerce instead of throwing).
+        assert!(!is_primitive_wrapper_brand_method(&recv, "charAt"));
+        assert!(is_string_prototype_generic_method(&recv, "charAt"));
+        assert!(!is_string_prototype_generic_method(&recv, "toString"));
+        assert!(!is_string_prototype_generic_method(&recv, "valueOf"));
+    }
+}

crates/perry-runtime/src/object/native_call_method.rs

@@ -16,6 +16,9 @@ mod object_proto;
 mod primitive_methods;
 mod proto_dispatch;
 mod string_methods;
+
+#[cfg(test)]
+mod dispatch_arg_coercion_tests;
 mod typed_array;
 
 use disposal::{
@@ -571,21 +574,6 @@ pub unsafe extern "C" fn js_native_call_method_value_apply(
     js_native_call_method_value(object, key, args_ptr, args_len)
 }
 
-#[inline]
-fn root_string_arg_handle<'scope>(
-    scope: &'scope crate::gc::RuntimeHandleScope,
-    arg_handles: &[crate::gc::RuntimeHandle<'scope>],
-    index: usize,
-) -> Option<crate::gc::RuntimeHandle<'scope>> {
-    let value = arg_handles.get(index)?.get_nanbox_f64();
-    let ptr = crate::value::js_get_string_pointer_unified(value) as *const crate::StringHeader;
-    if ptr.is_null() {
-        None
-    } else {
-        Some(scope.root_string_ptr(ptr))
-    }
-}
-
 fn throw_type_error_message(message: &[u8]) -> ! {
     let msg = crate::string::js_string_from_bytes(message.as_ptr(), message.len() as u32);
     let err = crate::error::js_typeerror_new(msg);

crates/perry-runtime/src/object/native_call_method/dispatch_arg_coercion_tests.rs

@@ -0,0 +1,128 @@
+//! Argument-coercion contract for `dispatch_string` (#5902): the reflective /
+//! any-typed String-method dispatch must apply the observable spec coercions
+//! (`ToString` on `replace`/`replaceAll` pattern+replacement, `ToLength` on
+//! `padStart`/`padEnd` maxLength) to its arguments instead of silently
+//! degrading non-string / non-number values. Every case here fails on the
+//! pre-fix shape: `replace` extracted only already-string args (a number
+//! pattern became a null pointer → receiver returned unchanged) and the pad
+//! arms passed the raw NaN-boxed arg through (a string/object maxLength read
+//! as NaN → target 0 → no padding). The user-code side of the same contract
+//! (a `{ valueOf }` maxLength must execute, in spec order, before the fill
+//! coercion) needs a JS closure and is covered by test262
+//! `built-ins/String/prototype/{replace,padStart,padEnd}` via the parity
+//! sweep, not here.
+
+use crate::value::JSValue;
+
+unsafe fn call_string_method(receiver: &str, method: &str, args: &[f64]) -> f64 {
+    let s = crate::string::js_string_from_bytes(receiver.as_ptr(), receiver.len() as u32);
+    let recv = f64::from_bits(JSValue::string_ptr(s).bits());
+    super::js_native_call_method(
+        recv,
+        method.as_ptr() as *const i8,
+        method.len(),
+        if args.is_empty() {
+            std::ptr::null()
+        } else {
+            args.as_ptr()
+        },
+        args.len(),
+    )
+}
+
+unsafe fn assert_string_result(result: f64, expected: &str, label: &str) {
+    let v = JSValue::from_bits(result.to_bits());
+    if v.is_string() {
+        let s = crate::object::has_own_helpers::str_from_string_header(v.as_string_ptr())
+            .unwrap_or_default();
+        assert_eq!(s, expected, "{label}");
+    } else if v.is_short_string() {
+        let mut buf = [0u8; crate::value::SHORT_STRING_MAX_LEN];
+        let n = v.short_string_to_buf(&mut buf);
+        assert_eq!(std::str::from_utf8(&buf[..n]).unwrap(), expected, "{label}");
+    } else {
+        panic!(
+            "{label}: expected a string result, got bits {:#x}",
+            v.bits()
+        );
+    }
+}
+
+fn short(bytes: &[u8]) -> f64 {
+    f64::from_bits(JSValue::try_short_string(bytes).unwrap().bits())
+}
+
+#[test]
+fn replace_coerces_a_non_string_pattern_via_to_string() {
+    // "a1b".replace(1, "X") — ToString(1) === "1" → "aXb".
+    unsafe {
+        let r = call_string_method("a1b", "replace", &[1.0, short(b"X")]);
+        assert_string_result(r, "aXb", "\"a1b\".replace(1, \"X\")");
+    }
+}
+
+#[test]
+fn replace_all_coerces_a_non_string_pattern_via_to_string() {
+    // "1a1".replaceAll(1, "X") — ToString(1) === "1" → "XaX".
+    unsafe {
+        let r = call_string_method("1a1", "replaceAll", &[1.0, short(b"X")]);
+        assert_string_result(r, "XaX", "\"1a1\".replaceAll(1, \"X\")");
+    }
+}
+
+#[test]
+fn replace_coerces_a_missing_replacement_to_undefined() {
+    // "ab".replace("b") — §22.1.3.19 runs ToString(replaceValue) even for an
+    // absent arg: ToString(undefined) === "undefined" → "aundefined" (Node
+    // agrees). The old shape substituted an empty replacement.
+    unsafe {
+        let r = call_string_method("ab", "replace", &[short(b"b")]);
+        assert_string_result(r, "aundefined", "\"ab\".replace(\"b\")");
+    }
+}
+
+#[test]
+fn pad_end_coerces_a_string_target_length_via_to_number() {
+    // "abc".padEnd("11", "def") — ToLength(ToNumber("11")) === 11 →
+    // "abcdefdefde".
+    unsafe {
+        let r = call_string_method("abc", "padEnd", &[short(b"11"), short(b"def")]);
+        assert_string_result(r, "abcdefdefde", "\"abc\".padEnd(\"11\", \"def\")");
+    }
+}
+
+#[test]
+fn pad_start_coerces_target_and_defaults_the_fill() {
+    // "5".padStart("3") — string target coerces to 3, absent fill defaults
+    // to " " → "  5".
+    unsafe {
+        let r = call_string_method("5", "padStart", &[short(b"3")]);
+        assert_string_result(r, "  5", "\"5\".padStart(\"3\")");
+    }
+}
+
+extern "C" fn undef_replacer(
+    _closure: *const crate::closure::ClosureHeader,
+    _matched: f64,
+    _offset: f64,
+    _whole: f64,
+) -> f64 {
+    f64::from_bits(JSValue::undefined().bits())
+}
+
+#[test]
+fn replace_fn_result_is_to_string_coerced() {
+    // "gnulluna".replace("null", () => undefined) — the callback result is
+    // ToString'd (§22.1.3.19): undefined renders as "undefined" →
+    // "gundefineduna" (test262 S15.5.4.11_A1_T5's shape; the old
+    // `call_replace_callback` dropped every non-string result to "").
+    unsafe {
+        let func_ptr = undef_replacer as *const u8;
+        let closure = crate::closure::js_closure_alloc(func_ptr, 0);
+        assert!(!closure.is_null());
+        crate::closure::js_register_closure_arity(func_ptr, 3);
+        let cb = crate::value::js_nanbox_pointer(closure as i64);
+        let r = call_string_method("gnulluna", "replace", &[short(b"null"), cb]);
+        assert_string_result(r, "gundefineduna", "\"gnulluna\".replace(\"null\", fn)");
+    }
+}