fix(hir): boxed primitive wrappers are not arrays — stop the array fold from hijacking their methods (#5902) - #7470
Conversation
…ld from hijacking their methods (#5902) A local typed `Named("Object")` / `Named("Number")` / `Named("Boolean")` — i.e. `new Object(true)`, `new Number(1)`, `new Boolean` — walked straight into the array fast path in `try_local_array_methods`, because the gate that guards it (`is_known_not_string`) reads "definitely not a string" as positive evidence of array-ness. `inst.indexOf(x)` then lowered 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`, so a borrowed `String.prototype.indexOf` never ran. `Named("String")` was already excluded one arm earlier (`is_boxed_string_wrapper`, which routes to the ToString-coercing string dispatch); this extends the same reasoning to the remaining boxed primitive wrappers and to `Object`, routing them to the generic runtime dispatch instead, which resolves the own/inherited property first and still reaches the array engine for a genuine array. `new Array()` is unaffected: `Expr::New { class_name: "Array" }` already infers `Type::Array`, not `Type::Named("Array")`, so real arrays keep the fast path (verified: the `Array*` folds are still emitted, and a mixed array/string/boxed-string/class probe matches node byte-for-byte). Adds `receiver_is_non_array_builtin_wrapper` as a standalone predicate with three per-PR-visible unit tests (`cargo test -p perry-hir --lib`), including one asserting `Named("String")` is deliberately NOT claimed here.
📝 WalkthroughWalkthroughThe array-method lowering now excludes boxed ChangesArray receiver lowering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/perry-hir/src/lower/expr_call/local_array_methods.rs`:
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c402ac2-469a-4d96-b9ee-8b09d3908f8b
📒 Files selected for processing (2)
changelog.d/7470-boxed-wrapper-array-fold.mdcrates/perry-hir/src/lower/expr_call/local_array_methods.rs
| 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" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 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.
|
Reviewed and merged. Independently verified both directions on a rebased build: real arrays still emit the |
1 participant
Fixes a coherent single-root subcluster of #5902 (test262
built-ins/Stringworklist): a boxed primitive wrapper receiver is not an array, but the array fast path claimed its methods anyway.Root cause
try_local_array_methodsdecides whether to foldrecv.<m>(…)into the denseExpr::Array*builtins. The gate that admits the fold reads "definitely not a string" as positive evidence of array-ness (is_known_not_string). So a local typedNamed("Object")/Named("Number")/Named("Boolean")— i.e.new Object(true),new Number(1),new Boolean— walked straight in, andlowered to
Expr::ArrayIndexOf, which reads the wrapper'sObjectHeaderas anArrayHeaderand answers-1. The borrowedString.prototype.indexOfnever ran.S15.5.4.7_A4_T4is the same bug one level up the chain — it puts the borrowed method onNumber.prototypeand calls it on anew Number(…)receiver.Fix
Named("String")was already excluded one arm earlier (is_boxed_string_wrapper, added so a boxed String routes to theToString-coercing string dispatch). This extends the same reasoning to the remaining boxed primitive wrappers and toObject, routing them to the generic runtime dispatch instead — which resolves the own/inherited property first and still reaches the array engine for a genuine array.Declining a fold is always semantically safe here: the generic dispatch is the fallback the rest of this file already uses for exactly this reason, so a mistyped-but-really-an-array receiver keeps working (it just doesn't take the dense path).
Real arrays are structurally unaffected —
Expr::New { class_name: "Array" }already infersType::Array, notType::Named("Array"), sonew Array()never reaches the new predicate.Verification
test262
built-ins/String(pinned4249661, node 26.5.1), full slice:Six flips, zero regressions — exactly the targeted cases:
prototype/indexOf/S15.5.4.7_A1_T1.js,_A1_T2.js,_A4_T4.jsprototype/lastIndexOf/S15.5.4.8_A1_T1.js,_A1_T2.js,_A4_T4.jsA mixed probe (typed array, inferred array, string primitive, boxed String, class instance with an own
indexOf) matches node byte-for-byte, and--print-hirconfirms theArray*folds are still emitted for the real arrays (2ArrayIndexOf, 2ArrayLastIndexOf, 1ArrayIncludes, 2ArraySlice, 1ArrayJoin) — the optimization is preserved, not disabled.New coverage is per-PR visible (
cargo test -p perry-hir --lib, notcrates/*/tests/): three unit tests on the extractedreceiver_is_non_array_builtin_wrapperpredicate, including one assertingNamed("String")is deliberately not claimed here (claiming it would send a boxed String to generic dispatch instead of the coercing string path).rustfmtclean;scripts/check_file_size.shclean (1176 lines).Deliberately out of scope
The 7th case in this cluster,
prototype/concat/S15.5.4.6_A4_T1.js, has a different root in a different crate:classify_own_slot(perry-runtime/src/array/generic.rs) labels any borrowed builtin closure a borrowed Array builtin, soobj.concat = String.prototype.concatruns the array engine on the receiver. Fixing it needs identity comparison against the realArray.prototype.concatand belongs in its own PR.No version bump /
CLAUDE.mdedit per the worklist's contributor instructions — maintainer folds metadata at merge.Refs #5902.
Summary by CodeRabbit
Object,Number,Boolean,Symbol, andBigIntvalues when calling array-like methods.