Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7470-boxed-wrapper-array-fold.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fix(hir): a boxed primitive wrapper receiver is not an array, so the array fast path no longer hijacks its methods (#5902). `try_local_array_methods` gates the `Expr::Array*` fold on `is_known_not_string`, which reads "definitely not a string" as positive evidence of *array*-ness — so a local typed `Named("Object")` / `Named("Number")` / `Named("Boolean")` (`new Object(true)`, `new Number(1)`, `new Boolean`) walked straight in and lowered `inst.indexOf(x)` to `Expr::ArrayIndexOf`, which reads the wrapper's `ObjectHeader` as an `ArrayHeader` and answers `-1` — so a borrowed `String.prototype.indexOf` stored on the receiver (or on `Number.prototype`) never ran. `Named("String")` was already excluded one arm earlier by `is_boxed_string_wrapper`, which routes a boxed String to the `ToString`-coercing *string* dispatch; the new `receiver_is_non_array_builtin_wrapper` predicate extends that reasoning to the remaining boxed wrappers and to `Object`, routing them to the generic runtime dispatch, which resolves the own/inherited property first and still reaches the array engine for a genuine array. Real arrays are structurally unaffected — `Expr::New { class_name: "Array" }` already infers `Type::Array`, not `Type::Named("Array")` — and `--print-hir` confirms the `Array*` folds are still emitted, so the optimization is preserved rather than disabled. test262 `built-ins/String`: pass 916→922, runtime-fail 16→10 (6 flips, zero regressions), parity 98.3%→98.9%; the flipped cases are `indexOf/S15.5.4.7_{A1_T1,A1_T2,A4_T4}` and `lastIndexOf/S15.5.4.8_{A1_T1,A1_T2,A4_T4}`. New per-PR-visible coverage: three `perry-hir` unit tests on the extracted predicate, one of which asserts `Named("String")` is deliberately *not* claimed here. Still open in this cluster and left for its own PR: `concat/S15.5.4.6_A4_T1` has a different root — `classify_own_slot` (`perry-runtime/src/array/generic.rs`) labels *any* borrowed builtin an *Array* builtin, so `obj.concat = String.prototype.concat` runs the array engine.
89 changes: 89 additions & 0 deletions crates/perry-hir/src/lower/expr_call/local_array_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,37 @@ fn receiver_is_class_instance(recv_ty: Option<&Type>) -> bool {
}
}

/// #5902: is `recv_ty` a boxed primitive wrapper (`new Number(1)`,
/// `new Boolean`, …) or a plain `Object` wrapper (`new Object(true)`)?
///
/// These are never arrays, but they are also not strings, and the array
/// fast-path gate reads "definitely not a string" as positive evidence of
/// array-ness (`is_known_not_string`). So `Named("Object")` / `Named("Number")`
/// / `Named("Boolean")` walked straight into the array block and lowered
/// `inst.indexOf(x)` to `Expr::ArrayIndexOf`, which reads the wrapper's
/// `ObjectHeader` as an `ArrayHeader` and answers `-1` — even when the receiver
/// owns or inherits a real `indexOf`. test262 `S15.5.4.7_A1_T1` borrows
/// `String.prototype.indexOf` onto `new Object(true)`, and `S15.5.4.7_A4_T4`
/// puts it on `Number.prototype`; both expect the borrowed method to run.
///
/// `Named("String")` is deliberately absent — it is handled one step earlier by
/// `is_boxed_string_wrapper`, which routes to the *string* dispatch (so the
/// wrapper gets `ToString`-coerced) rather than to generic dispatch.
///
/// Declining the fold is always safe: the generic runtime dispatch resolves the
/// own/inherited property first and still reaches the array engine for a
/// genuine array, so a mistyped-but-really-an-array receiver keeps working.
fn receiver_is_non_array_builtin_wrapper(recv_ty: Option<&Type>) -> bool {
matches!(
recv_ty,
Some(Type::Named(n))
if matches!(
n.as_str(),
"Object" | "Number" | "Boolean" | "Symbol" | "BigInt"
)
)
Comment on lines +65 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle union-typed wrapper receivers before enabling the array fast path.

receiver_is_non_array_builtin_wrapper matches only a top-level Type::Named. If lookup_local_type returns a Type::Union containing Type::Named("Object"), Type::Named("Number"), Type::Named("Boolean"), Type::Named("Symbol"), or Type::Named("BigInt"), is_union_with_string remains false and is_known_not_string becomes true. The gate can then emit an Expr::Array* operation for a possible wrapper receiver and recreate the ObjectHeader/ArrayHeader mismatch.

Make the predicate inspect union variants and decline the fold when any non-array wrapper is possible. Add a union case to the unit tests.

Proposed fix
 fn receiver_is_non_array_builtin_wrapper(recv_ty: Option<&Type>) -> bool {
-    matches!(
-        recv_ty,
-        Some(Type::Named(n))
-            if matches!(
-                n.as_str(),
-                "Object" | "Number" | "Boolean" | "Symbol" | "BigInt"
-            )
-    )
+    match recv_ty {
+        Some(Type::Named(n)) => matches!(
+            n.as_str(),
+            "Object" | "Number" | "Boolean" | "Symbol" | "BigInt"
+        ),
+        Some(Type::Union(variants)) => variants.iter().any(|ty| {
+            receiver_is_non_array_builtin_wrapper(Some(ty))
+        }),
+        _ => false,
+    }
 }
 
     fn real_array_receivers_keep_the_fold() {
+        assert!(receiver_is_non_array_builtin_wrapper(Some(&Type::Union(vec![
+            Type::Named("Object".to_string()),
+            Type::Named("Number".to_string()),
+        ]))));

Also applies to: 141-141, 280-281, 1131-1174

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry-hir/src/lower/expr_call/local_array_methods.rs` around lines 65
- 73, Update receiver_is_non_array_builtin_wrapper to inspect Type::Union
variants recursively or directly, returning true when any variant is the named
Object, Number, Boolean, Symbol, or BigInt wrapper, so the array fast-path gate
declines folding possible wrapper receivers. Add a unit test covering a union
containing one of these wrapper types and verify no Expr::Array* operation is
emitted.

}

pub(super) fn try_local_array_methods(
ctx: &mut LoweringContext,
call: &ast::CallExpr,
Expand Down Expand Up @@ -107,6 +138,7 @@ pub(super) fn try_local_array_methods(
// file because they aren't Array methods.
let is_boxed_string_wrapper =
matches!(type_info, Some(Type::Named(n)) if n == "String");
let is_non_array_builtin_wrapper = receiver_is_non_array_builtin_wrapper(type_info);
let is_known_string = type_info
.map(|ty| matches!(ty, Type::String))
.unwrap_or(false)
Expand Down Expand Up @@ -245,6 +277,8 @@ pub(super) fn try_local_array_methods(
false // user class — must dispatch to class method, skip array fast-path
} else if is_object_type {
false // object type literal — dispatch via method call, not array ops
} else if is_non_array_builtin_wrapper {
false // boxed primitive wrapper / plain Object — never an array
} else if is_buffer_type {
false // Buffer/Uint8Array — runtime dispatch handles byte-level methods
} else if is_node_stream_readable_type {
Expand Down Expand Up @@ -1085,3 +1119,58 @@ pub(super) fn try_local_array_methods(
}
Ok(Err(args))
}

#[cfg(test)]
mod tests {
use super::*;

fn named(n: &str) -> Option<Type> {
Some(Type::Named(n.to_string()))
}

#[test]
fn boxed_primitive_wrappers_decline_the_array_fold() {
// #5902: `new Object(true)` / `new Boolean` / `new Number(1)` are not
// strings, but they are not arrays either — the array fast path must
// not claim `indexOf`/`lastIndexOf`/`slice`/`includes` on them.
for n in ["Object", "Number", "Boolean", "Symbol", "BigInt"] {
assert!(
receiver_is_non_array_builtin_wrapper(named(n).as_ref()),
"{n} wrapper must decline the array fast path"
);
}
}

#[test]
fn string_wrapper_is_not_claimed_here() {
// `Named("String")` routes to the STRING dispatch via
// `is_boxed_string_wrapper`, one arm earlier, so this predicate must
// leave it alone — claiming it here would send a boxed String to
// generic dispatch instead of the ToString-coercing string path.
assert!(!receiver_is_non_array_builtin_wrapper(
named("String").as_ref()
));
}

#[test]
fn real_array_receivers_keep_the_fold() {
assert!(!receiver_is_non_array_builtin_wrapper(Some(&Type::Array(
Box::new(Type::Number)
))));
assert!(!receiver_is_non_array_builtin_wrapper(Some(
&Type::Generic {
base: "Array".to_string(),
type_args: vec![Type::Number],
}
)));
// An unknown receiver is gated elsewhere (`is_ambiguous_method` /
// `is_arraylike_mutator_method`), not here.
assert!(!receiver_is_non_array_builtin_wrapper(Some(&Type::Any)));
assert!(!receiver_is_non_array_builtin_wrapper(None));
// A user class named e.g. `Number`-adjacent must not be confused with
// the builtin set; only the exact builtin names are claimed.
assert!(!receiver_is_non_array_builtin_wrapper(
named("NumberLike").as_ref()
));
}
}
Loading