feat(arrays): array_push() takes PHP's value list, unset() reaches a by-ref table

[Guikingone]

Fixes #677 — both halves.

1. array_push() accepted only one value

$a = [1, 2];
array_push($a, 3, 4);        // error: array_push() takes exactly 2 arguments

PHP's signature is array_push(array &$array, mixed ...$values): int. The contract pinned min_args: 2, max_args: 2 to reproduce a legacy CHECK arm, so the variadic shape the golden signature already declared was unreachable — and array_push($a) with no values, legal since PHP 7.3, went with it.

The arity is now min_args: 1 with no maximum, exactly as the array_unshift sibling has always been. Values are appended in source order, one at a time: each append can reach __rt_array_grow and relocate the array, so lower_array_push_value republishes the receiver between values rather than once at the end. The ir_lower fast path for a plain local re-reads the slot per value for the same reason — appending into a stale pointer would write to freed storage.

array_push also returned Void, so $n = array_push($a, 1) read NULL where PHP gives the new element count. That is fixed here rather than separately, because it lives in the same descriptor and the same check hook the arity does: the count is read back off the array after the appends, which also gives the value-less form its answer without a helper call. array_unshift already did exactly this.

The eval interpreter was not the gap. eval_array_push_unshift_declared_call already accepted the full value list and returned the count, so this brings the AOT backend up to parity with eval rather than changing both.

2. unset() on an element of a by-reference array parameter

function u(array &$a) { unset($a["b"]); }
// EIR backend error: unsupported EIR backend feature: unset target shape with 1 lowered operands

Every other write through a by-reference array already worked — $a["c"] = 3, array_unshift(), sort() — because each routes its receiver through ReceiverPlace. lower_hash_unset does too, and has all along. The refusal was in ir_lower, which turned away a ref-bound receiver before the backend ever saw it.

That gate was written for the indexed case, and it is right there: unset() removes a key without renumbering, so the local has to become a hash, and a callee cannot retype the caller's slot, which still reads array<T>. An associative receiver has no such problem — the removal is in place and the copy-on-write split publishes through the ref cell — so the gate now applies only where its reasoning does.

The indexed case keeps its refusal, but lower_unset_builtin's message no longer describes only the untyped-property shape. It names this one too and says the associative form is supported, so "array/hash elements" in the supported list stops reading as a blanket promise.

Verified

Twelve array_push shapes and eight unset shapes against host PHP 8.5.10, all byte-identical:

push value counts	0, 1, 2, 5 values, plus the returned count each time
push growth	13 values into a 1-element array, forcing several __rt_array_grow
push payloads	int, bool
push receivers	local, by-ref parameter, property, static property, container element
push ordering	array_push($s, f(1), f(2), f(3)) evaluates left to right
unset	multiple keys, a missing key, an integer key, a &$x binding, a nested callee, copy-on-write protection of a snapshot taken before the call, unsetting every key

Both halves were confirmed load-bearing by reverting each in turn: the push tests fail with array_push() takes exactly 2 arguments, the unset tests with the backend's unsupported-shape error.

--gc-stats reports allocs=1000 frees=1000 over 200 iterations of both together, and each half carries a leak summary: clean heap-debug test of its own — the push one because growth frees the previous buffer after republishing, the unset one because the split has to account for the released payloads and the replaced table exactly once.

cargo test --test codegen_tests -- codegen::arrays passes in full (573), as do the 1680 lib tests and 1534 error tests.

Docs and examples

docs/php/arrays.md gets the real array_push() signature and a by-reference unset() section that also states the indexed limitation and its workaround. The generated builtin pages are regenerated from the contract.

examples/arrays/main.php gains a multi-value push showing the returned count; examples/assoc-arrays/main.php gains a drop_field(array &$record, string $field) that also shows the caller's earlier copy surviving. Both examples are byte-identical to host PHP 8.5.10.

One pinned expectation moved. test_error_array_push_wrong_args asserted takes exactly 2 arguments and now asserts takes at least 1 argument, which is what host PHP reports for array_push() with no arguments (ArgumentCountError: array_push() expects at least 1 argument, 0 given).

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

[greptile-apps]

Confidence Score: 4/5

The PR should not merge until the array_push() fast path safely handles value expressions that rebind or retype its by-reference receiver.

Findings

1.  Receiver retyping breaks fast path ▶

Fix with agent prompt

### Issue 1
src/ir_lower/expr/array_builtin_args.rs:47
The fast path commits to indexed-array lowering before evaluating the remaining arguments, even though those expressions can rebind the receiver. For example, in `array_push($a, ($a = 5))`, PHP evaluates the assignment before entering `array_push()` and then reports that the receiver is not an array. This path instead reloads `$a` after the assignment and emits typed `ArrayPush` and `ArrayLen` operations without rechecking its representation, which can produce invalid EIR or treat an integer as an array. Please use representation-safe call lowering when argument evaluation can invalidate the receiver, or preserve the by-reference receiver place while retaining PHP's evaluation order.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR expands array_push() to PHP’s variadic signature and integer return value, adds associative-array unset() support through by-reference bindings, and updates generated documentation, examples, and regression coverage.

• Evaluates all pushed values before performing the first append.
• Republishes relocated array receivers between individual appends.
• Allows in-place associative removal through reference cells while retaining the indexed-array diagnostic.
• Updates builtin contracts and generated documentation to reflect the supported behavior.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Evaluate array_push arguments] --> B[Reload receiver local]
    B --> C[Append each value]
    C --> D{More values?}
    D -->|Yes| B
    D -->|No| E[Read final array length]
    E --> F[Return integer count]

Loading

_{Reviews (6) · Last reviewed commit: "test(arrays): unpin array_push()'s legac..."}

[nahime0]

Grok review (READ-ONLY)

Verdict: sound with nits — merge yes, with follow-ups.

Both halves of #677 look load-bearing and correct. Greptile’s open P1 (array_push($a, ($a = 5)) retyping the fast path) is not a merge blocker here: same-variable reassign to a different type is a hard error, and Mixed locals never enter this fast path.

What this PR changes

array_push. Contract drops the legacy min_args/max_args = 2 pin and returns Int (new count). Checker infers every value. Two lowerings append in source order, one value at a time, republishing between values because each append can __rt_array_grow:

• Fast path: plain local already typed Array(_) — values lowered first, then each ArrayPush re-reads the slot; result is ArrayLen.
• General path: RuntimeFnId::ArrayPush loop + count from header / __rt_mixed_count.

Property / static / element receivers go through the existing $tmp = place; array_push($tmp, …); place = $tmp rewrite before the fast path. Eval was already variadic + count; this brings AOT up to parity. Eval-order interleave bug from an earlier commit is fixed and tested.

unset. ir_lower no longer rejects every ref-bound array receiver — only a ref-bound indexed Array(_). AssocArray (including &$param) reaches Op::HashUnset; backend lower_hash_unset already publishes the COW-split pointer through ReceiverPlace. Indexed by-ref stays refused for the packed→hole→hash / caller-slot-type reason, with a clearer error that names the associative form as supported.

What looks fine

• Arity / value-less push / return count match PHP and array_unshift.
• Per-value republish + left-to-right order + growth fixtures.
• Unset multi-key, missing key, int keys on hash, &$x, nested callee, COW snapshot, empty-table, heap-clean.
• Docs/examples moved with the contract; error test now expects takes at least 1 argument.

Follow-ups opened

Issue	Topic
#1087	AOT array_push still rejects AssocArray (PHP/eval/array_unshift accept hashes)
#1088	Pin by-ref array_push through __rt_array_grow (current grow test is local-only)
#1089	unset($a["k"]) on array &$a depends on specialization to AssocArray
#1090	Indexed by-ref unset remains refused (document as intentional or implement)

[Guikingone]

I tried to reproduce this and could not. The scenario is unreachable, because the checker refuses exactly the reassignments that would make the receiver's representation stale — before lowering ever runs.

Your own example:

$a = [1, 2];
array_push($a, ($a = 5));

elephc: error[3:20]: Type error: cannot reassign $a from array<int> to int
PHP:    Uncaught TypeError: array_push(): Argument #1 ($array) must be of type array, int given

Both reject it. The diagnostics differ in kind — a compile-time refusal against PHP's runtime TypeError — but no invalid EIR is produced and no integer is treated as an array.

I then went looking for a rebind the checker allows that would still reach the fast path:

rebind inside the argument list	result
($a = 5) — to an int	refused: cannot reassign $a from array<int> to int
($b = ["x"]) — to a different ELEMENT type	refused: cannot reassign $b from array<int> to array<string>
($a = [9, 9, 9]) — same representation	compiles, and answers a: 4 last=7, byte-identical to PHP

So the only rebind that survives the checker is the one that leaves the receiver's representation intact, which is precisely the case the fast path is still correct for. The local-retyping rule that does the refusing is pre-existing and not introduced by this PR.

Leaving this open rather than resolving it myself, in case you have a shape I did not think of — a by-reference parameter aliasing the receiver, or a property/static receiver, would be the places I would look next. If you have one that compiles and misbehaves, post it and I will fix it.