arr.push(f()) / arr.push(...g()) evaluate the receiver AFTER the argument, so a reassigning argument pushes onto the wrong array

Found while auditing crates/perry-codegen/src/expr/array_push.rs for the Layer 1
rooting migration (#7615, slice 3). Not a rooting bug — the opposite: the arm is
rooting-safe because it evaluates in an order the spec does not permit.

The divergence

let a: number[] = [1];
function f(): number { a = [9]; return 2; }
a.push(f());
console.log(JSON.stringify(a));

let b: number[] = [1];
function g(): number[] { b = [9]; return [2, 3]; }
b.push(...g());
console.log(JSON.stringify(b));

	output
`node --experimental-strip-types` (26.5.1)	`[9]` / `[9]`
perry (`main`, `PERRY_NO_AUTO_OPTIMIZE=1`)	`[9,2]` / `[9,2,3]`

Per ES2024 the MemberExpression a.push is evaluated to a Reference before
the argument list, so the push lands on the array a named at that moment — the
one the program then throws away. Perry pushes onto the array a names
afterwards.

Where

Both arms of expr/array_push.rs lower the value first and the receiver second:

// Expr::ArrayPush
let (v, v_bits) = lower_array_push_value(ctx, value, ...)?;   // arbitrary user code
let arr_box = lower_expr(ctx, &array_expr)?;                  // Expr::LocalGet, a slot read

// Expr::ArrayPushSpread
let src_box = lower_expr(ctx, source)?;                       // arbitrary user code
let arr_box = lower_expr(ctx, &Expr::LocalGet(*array_id))?;

It is only observable when the argument reassigns the receiver's binding, which
requires the binding to be captured-and-mutated (so it is boxed) or a module
global — otherwise the two orders name the same array.

Why it is not fixed in #7615's slice 3

Reading the receiver after the value is exactly what makes the arm free of
rooting work today: the load observes whatever an evacuating cycle wrote back
into the local's alloca / box / module global, so no register is stale and
operand_protection answers Reuse. Restoring spec order means the receiver is
live across the value's lowering, i.e. it has to be rooted:

rooting::with_operands_rooted_across(
    ctx,
    &[&array_expr],          // the receiver, rooted first
    &[value],                // does the window collect?
    |ctx| lower_array_push_value(ctx, value, layout_note_needed, write_barrier_needed),
    |ctx, vals, (v, v_bits)| { /* the existing tiers, on vals[0] */ },
)

That is expressible with the existing combinator and needs no new one. What it
costs is the question: a.push(<anything that can collect>) is one of the most
frequently emitted lowerings in the compiler, and every such push would gain a
temp-root push / re-read / release. Numeric pushes (a.push(i * 2)) are
unaffected — operand_protection still answers Reuse when the value provably
cannot collect — but out.push(f(x)) and rows.push({...}) are not.

So the fix needs a measured before/after on the pinned mini, which a
behaviour-preserving refactor slice has no mandate to do. Filed here instead, per
the same rule that produced #7628.

Suggested acceptance

a test-files/test_gap_*.ts covering both arms (the snippet above), registered
in the gap corpus;
the reorder above, with the temp-root cost measured on churn_alloc /
push_cls before and after.

refactor(codegen): migrate the object/array literal modules onto the …

refactor(codegen): migrate the computed read/write modules onto the L…

Fixed in #7699.

Node A/B, `node 26.5.1` (`.node-version`), byte-for-byte

test-files/test_gap_7634_push_receiver_order.ts:

node 26.5.1                 perry before          perry after
[9]                         [9,2]                 [9]        a.push(f()),   f: a = [9]
[9]                         [9,2,3]               [9]        b.push(...g()), g: b = [9]
[9]                         [9,2]                 [9]        captured-and-mutated local
[9]                         [9,2]                 [9]        d.push((d = [9], 3))
[9] [1,2]                   —                     [9] [1,2]  the alias keeps the array pushed onto
4 / []                      —                     4 / []     push's own result is the new length of THAT array
[1,2,3,4]                   [1,2,3,4]             [1,2,3,4]  plain local — unobservable, unchanged

The cost question, answered by gating rather than by measuring a regression

The issue's own framing is right — the spec fix and the rooting fix are one change, and a blanket reorder puts every pointer-valued push into a rooting window. #7511 measured keep.push(ptr) as 28% of a top family, 98.5% of it from the loop function's push, so that is not a rounding error.

So the reorder is gated on the divergence being observable. push_receiver_is_rebindable answers true only when:

the argument assigns the receiver's id itself (Expr::LocalSet / Expr::Update, or an Expr::Closure literal, answered conservatively because walk_expr_children does not descend into one); or
the binding is boxed — collect_boxed_vars' rule is "captured AND mutated", so a captured-but-never-assigned array stays on the fast path and items.forEach(x => rows.push(f(x))) costs nothing — or a module global, and the argument can reach a collection point.

When it answers false, the historical argument-then-receiver order is kept with its inline tiers and its Reuse verdict. Evidence: --trace llvm over a 1000-iteration out.push(mk(i)) loop plus a captured rows.push(i * 2) arrow contains zero call i32 @js_gc_temp_root_push and zero spec-order blocks. an_unreachable_binding_keeps_the_historical_order_and_roots_nothing pins it.

★ One thing the issue did not anticipate: the reorder alone is not correct

Every fast tier publishes the reallocated array head back into the binding unconditionally. Once the argument may have rebound the binding, that store lands on the wrong array — a.push(f()) with f setting a = [9] would overwrite [9] with the grown [1,2], turning a spec fix into a different wrong answer.

The spec-ordered arm therefore drops the inline tiers and guards its write-back on the binding still naming the array that was pushed onto. When it does not, the store is skipped and aliases stay valid through the forwarding pointer js_array_push_f64 installs (#233) — the same mechanism that already keeps const x = a; a.push(1) correct. The [9] [1,2] row above is that case, and it matches node.

The five-way storage write-back chain was duplicated between the two arms and is now one emit_push_writeback shared by three callers.

fix(codegen): evaluate arr.push's receiver before its argument (#7634)

fix(codegen): the lowering-window cluster — push order, member update…

Navigation Menu

Repository navigation

arr.push(f()) / arr.push(...g()) evaluate the receiver AFTER the argument, so a reassigning argument pushes onto the wrong array #7634

The divergence

Where

Why it is not fixed in #7615's slice 3

Suggested acceptance

Node A/B, `node 26.5.1` (`.node-version`), byte-for-byte

The cost question, answered by gating rather than by measuring a regression

★ One thing the issue did not anticipate: the reorder alone is not correct

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

arr.push(f()) / arr.push(...g()) evaluate the receiver AFTER the argument, so a reassigning argument pushes onto the wrong array #7634

Description

The divergence

Where

Why it is not fixed in #7615's slice 3

Suggested acceptance

Activity

proggeramlug commented on Aug 9, 2026

Node A/B, node 26.5.1 (.node-version), byte-for-byte

The cost question, answered by gating rather than by measuring a regression

★ One thing the issue did not anticipate: the reorder alone is not correct

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Node A/B, `node 26.5.1` (`.node-version`), byte-for-byte