fix(hir): stop the Array fast paths from hijacking same-named prototype methods#6397
Conversation
…pe methods
A method installed the classic way — `Fn.prototype.push = function (...)` — was
hijacked by Perry's Array fast paths whenever its name collided with an Array
builtin (`push`/`pop`/`shift`/`unshift`). The user's method never ran, the call
returned a bogus array length, and `js_array_push_f64` wrote the receiver's
ObjectHeader *as an ArrayHeader*, so the instance was corrupted and its fields
subsequently read `undefined`. `shift()` even leaked a raw NaN-boxed double
(`2.12e-314`) into user code.
function Q() { this.n = 0; }
Q.prototype.push = function (x) { this.n++; return "Q.push:" + x; };
new Q().push("a"); // node: "Q.push:a" perry: 0, and `n` is now undefined
Two gaps, both closed here:
* Local receiver — `is_user_class_instance` knew about class decls, interfaces
and imported classes, but not function-constructors, so a local typed
`Named("Q")` was not `Any`/`Unknown` and sailed past the wall-49 guard into
the array block.
* Property receiver — the #5139 deferral only inspected a *bare identifier*
receiver. `this._commands.push(c)` had no gate at all and committed to the
eager `array.push_single` arm. `push` now folds only when the receiver is
provably an array; otherwise it defers to `js_native_call_method`, which
selects on the receiver's real runtime shape. The rest of the mutator family
already reached that dispatch for property receivers.
The fast paths are preserved where they matter: `arr.push(x)` on a typed array
local still folds to `Expr::ArrayPush`, and `this.items.push(v)` on a declared
`items: number[]` field still folds to `array.push_single` (`infer_type_from_expr`
resolves the field through `class_field_types`). Only receivers that cannot be
proven to be arrays now defer — exactly the population where correctness was at
risk.
This is the shape denque uses, which is how it reached mysql2: every
`this._commands.push(cmd)` was silently dropped, so the queue always reported
empty and a real MySQL query never issued its command. It bites any queue,
stack, ring-buffer or linked-list written in the `Fn.prototype.<name> = fn`
idiom, and most minified/transpiled bundles.
Found while compiling a real Next.js + MySQL app; `test_gap_function_ctor_array_named_methods`
covers local/property/parameter receivers, a denque-shaped ring buffer, and the
preserved real-array cases. Byte-identical to node.
📝 WalkthroughWalkthroughChangesThe HIR array-method fast paths now require stronger receiver classification before folding Array lowering safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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.
🧹 Nitpick comments (1)
crates/perry-hir/src/lower/expr_call/array_only_methods.rs (1)
714-715: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider including
Type::Tuplein the provably array check.The
provably_arraycheck currently verifies if the receiver isType::ArrayorType::Generic { base: "Array", .. }. If a receiver is statically typed as a Tuple (e.g.[number, string]), this check will evaluate tofalseand bail out to dynamic dispatch. Since tuples are array-like and other parts of the codebase (such as line 486 in this file and checks inlocal_array_methods.rs) allow them in array fast paths, consider addingType::Tuple(_)to this check to maintain optimal performance for tuples.♻️ Proposed refactor
- let provably_array = matches!(recv_ty, Type::Array(_)) + let provably_array = matches!(recv_ty, Type::Array(_) | Type::Tuple(_)) || matches!(&recv_ty, Type::Generic { base, .. } if base == "Array");🤖 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/array_only_methods.rs` around lines 714 - 715, Update the provably_array check in the array method lowering path to also match Type::Tuple(_), preserving the existing Type::Array and generic Array cases so statically typed tuples use the array fast path instead of dynamic dispatch.
🤖 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.
Nitpick comments:
In `@crates/perry-hir/src/lower/expr_call/array_only_methods.rs`:
- Around line 714-715: Update the provably_array check in the array method
lowering path to also match Type::Tuple(_), preserving the existing Type::Array
and generic Array cases so statically typed tuples use the array fast path
instead of dynamic dispatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 620ea1b7-a6ed-4834-906f-e3f82a6c70e9
📒 Files selected for processing (3)
crates/perry-hir/src/lower/expr_call/array_only_methods.rscrates/perry-hir/src/lower/expr_call/local_array_methods.rstest-files/test_gap_function_ctor_array_named_methods.ts
|
The It races the libuv threadpool: the three callback-form fd mutators are dispatched concurrently, so their callbacks fire in completion order. The gap harness compares perry against a freshly run node oracle, and the oracle itself is nondeterministic — across 40 runs, Fix is up as #6400 (buffer the three callback results, flush in a fixed order; assertions and expected output unchanged). Once that lands, rebase or rerun and this should go green. Note |
…ol (#6400) The three callback-form fd mutators are dispatched to the libuv threadpool concurrently, so their callbacks fire in completion order, which is not deterministic. The gap harness compares perry against a freshly run node oracle -- and the *oracle itself* is unstable: across 40 runs of `node --experimental-strip-types` this test produced 4 distinct outputs, emitting `fchown closed cb` before `futimes closed cb` on roughly one run in six. Perry is deterministic here (always submission order), so the test red-lit at random on unrelated PRs -- it took out #6397, #6398 and #6399 today, and cost a full bisect before the oracle was suspected. The test is about the error surface (err.code / err.syscall) of each fd mutator, not about libuv's scheduling. Buffer the three callback results and flush them in a fixed order once all three have landed. Every assertion is preserved and the emitted 10 lines are byte-for-byte what they were; only the print order is pinned. After: node emits 1 distinct output across 60 runs, perry 1 across 40, and the two are byte-identical. Co-authored-by: Ralph Küpper <ralph3@skelpo.com>
1 participant
The bug
A method installed the classic way —
Fn.prototype.push = function (...)— is hijacked by the Array fast paths whenever its name collides with an Array builtin (push/pop/shift/unshift). The user's method never runs, the call returns a bogus array length, and — worse —js_array_push_f64writes the receiver'sObjectHeaderas anArrayHeader, corrupting the instance so its fields subsequently readundefined.shift()even leaks a raw NaN-boxed double (2.12e-314) into user code.Class methods and object-literal methods were never affected, which is why this survived: it only bites the
Fn.prototype.<name> = fnidiom — every queue, stack, ring-buffer and linked-list written that way, plus most minified/transpiled bundles.Root cause — two gaps
Local receiver.
is_user_class_instanceknew about class decls, interfaces and imported classes, but not function-constructors.new Q()types its instancesNamed("Q"), which is neitherAnynorUnknown, so it sailed past the wall-49 guard straight into the array block.Property receiver. The #5139 deferral only inspected a bare identifier receiver.
this._commands.push(c)had no gate at all and committed to the eagerarray.push_singlearm.The fix
pushnow folds only when the receiver is provably an array; otherwise it defers tojs_native_call_method, which selects on the receiver's real runtime shape (real array → dense helper, growth via the #233 forwarding pointer; object → its own method). The rest of the mutator family already reached that dispatch for property receivers.The fast paths are preserved where they matter.
arr.push(x)on a typed array local still folds toExpr::ArrayPush, andthis.items.push(v)on a declareditems: number[]field still folds toarray.push_single(infer_type_from_exprresolves the field throughclass_field_types). Only receivers that cannot be proven to be arrays defer — exactly the population where correctness was at risk.How it was found
Compiling a real Next.js + MySQL app. This is the shape denque uses, which is how it reached mysql2: every
this._commands.push(cmd)was silently dropped, so the command queue always reported empty and a real query never issued its command.Test
test_gap_function_ctor_array_named_methods— local / property / parameter receivers, a denque-shaped ring buffer (push must enqueue so shift can dequeue), and the preserved real-array cases. Byte-identical to node.Gap suite: no new untriaged failures (verified on a branch of
maincarrying only this and the two sibling fixes; the two failures the runner flags as "new" —test_gap_fs_fd_2749,test_gap_handle_band_object_ops— reproduce byte-identically on pristinemainand are pre-existing).Summary by CodeRabbit
Bug Fixes
push,pop,shift, andunshiftfrom being incorrectly applied to array-like objects and class instances.Tests