Skip to content

fix(ir,codegen): autovivify missing parents in nested array writes#570

Merged
nahime0 merged 4 commits into
illegalstudio:mainfrom
mirchaemanuel:fix/555-autovivify-missing-parent
Jul 23, 2026
Merged

fix(ir,codegen): autovivify missing parents in nested array writes#570
nahime0 merged 4 commits into
illegalstudio:mainfrom
mirchaemanuel:fix/555-autovivify-missing-parent

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

fix(ir,codegen): autovivify missing parents in nested array writes

Fixes #555.

Symptom

$a = ['x'];
$a[7][1] = 'patched';   // PHP: autovivifies $a[7] as an array, no warning
echo $a[7][1];          // elephc: empty line, "Undefined array key 7" twice,
                        // and the dropped value leaked

Root cause

The parent chain of a nested assignment was lowered as a plain read. A missing element produced a detached Mixed(null) box (with an undefined-key warning), and __rt_mixed_array_set released the value in its incompatible-target path. Nothing installed a container into the missing slot: there were no write-context lookup semantics for intermediate elements. Additionally, a string key on an indexed payload branched to the same drop path, losing legal writes like $a[0]['new'] = 'v'.

Fix

The parent chain now lowers with fetch-for-write semantics:

  • New __rt_mixed_array_get_for_write autovivifies missing indexed elements (grow, zero-fill, length update), null gap slots, boxed-null slots, and missing hash keys, installing an empty-array cell into the parent storage and returning the STORED cell (retained) so the three-operand set writes through the parent. Indexed payloads are first normalized via __rt_array_to_mixed (COW split + boxed slots, republished into the cell) — which also lands writes through concrete intermediates of 3+ level chains, the residual limitation documented in fix(ir): write nested array assignments through the parent cell (#529) #553.
  • Concrete array<mixed>/assoc locals go through a new __rt_array_ensure_elem_for_write wrapper plus a local storeback; a literal string key on an indexed local promotes it via Op::ArrayToHash.
  • __rt_mixed_array_set now promotes indexed payloads to hash storage for string keys (new __rt_mixed_cell_promote_to_hash) instead of dropping the write, matching PHP array-key semantics.
  • Write-context reads emit no undefined-key warning, matching PHP.
  • Both AArch64 and x86_64 emitters implemented symmetrically.

Tests

tests/codegen/arrays/nested_autovivify.rs (14 tests): the exact issue repro, append position, null gap and boxed-null parents, assoc missing key, multi-level int and string chains from [], Mixed indexed/assoc receivers, string-key child promotion, existing-parent and shared-array COW controls, and function-return visibility; all assert no warning and heap-debug cleanliness. (The json_decode variant asserts stdout/warnings only — Mixed builtin results stored into locals leak one owner reference on current main independently of nested writes, reproducible with a bare read.) 13/13 of the original tests failed before the fix.

Limitations (documented)

  • Incompatible scalar intermediates are intentionally not converted (the write is still dropped; PHP raises Error: Cannot use a scalar value as an array — no error machinery added here).
  • Negative indexes on dense indexed storage stay unsupported (pre-existing storage model).
  • Statically-Mixed key expressions on concrete locals and ArrayAccess-object intermediates keep the previous plain-read path.
  • Autovivified gaps remain dense (zero-filled) per elephc's array model, so count() can differ from PHP's sparse arrays (pre-existing).

QA

  • Issue repro prints patched, zero warnings, leak summary: clean, under --ir-opt=on and =off.
  • After rebasing onto current main (post-fix(ir): write nested array assignments through the parent cell (#529) #553, including its extended nested_mixed_write suite): nested_autovivify 14, nested_mixed_write 8, arrays:: 366, mixed 262 — all green. Pre-rebase full slices: nested 155, json 445, cargo test --lib 930 (one pre-existing environment-dependent DNS test failure unrelated to this change, fails identically on pristine main). No new clippy warnings.

https://claude.ai/code/session_01QyzntRke1Cgxgq1ueuz4X9

@github-actions github-actions Bot added area:codegen Touches target-aware assembly or backend lowering. area:eir Touches EIR definitions, lowering, validation, or passes. area:runtime Touches runtime helpers, GC, ownership, or bridge runtimes. size:m Medium-sized pull request. type:fix Corrects broken or incompatible behavior. labels Jul 19, 2026
…llegalstudio#555)

A nested assignment through a missing intermediate element
($a[7][1] = 'patched' when $a[7] does not exist) lowered the parent
chain as a plain READ: the miss produced a detached Mixed(null) box
plus an "Undefined array key" warning, __rt_mixed_array_set hit its
incompatible-target drop path, and the write was silently lost. PHP
autovivifies the missing element as an array and emits no warning.

Lower the parent chain with fetch-for-write semantics instead:

- New runtime helper __rt_mixed_array_get_for_write mirrors the plain
  reader but installs an empty-array cell into missing indexed slots
  (growing and zero-filling gaps), null gap slots, boxed-null slots,
  and missing hash keys, and returns the STORED cell retained so the
  three-operand set writes through the parent storage. Indexed
  payloads are normalized through __rt_array_to_mixed first, which
  also fixes write-through for concrete intermediates of 3+ level
  chains (the residual limitation documented in the issue illegalstudio#529 fix).
  Non-container receivers tail-call the plain reader unchanged.
- New wrapper __rt_array_ensure_elem_for_write applies the same logic
  to concrete array<mixed>/assoc locals through a transient stack
  cell and returns the possibly promoted or reallocated container for
  the local storeback (prepare/store_prepared bracketing, so boxed
  Mixed-storage slot owners are released exactly once).
- New helper __rt_mixed_cell_promote_to_hash converts an indexed cell
  payload to hash storage (array_to_hash + hash_to_mixed, COW-safe);
  __rt_mixed_array_set now uses it for string keys on indexed
  payloads instead of dropping the write, matching PHP array-key
  semantics. A literal string key on an indexed local promotes the
  local through Op::ArrayToHash before the ensure call.
- The write-context parent read emits no undefined-key warning, and
  the post-ensure re-read is in bounds, so legal autovivifying writes
  are silent like PHP.

Both AArch64 and x86_64 emitters are implemented symmetrically.
Incompatible scalar intermediates are intentionally not converted
(the leaf set still drops the write; PHP raises an Error there).

Claude-Session: https://claude.ai/code/session_01QyzntRke1Cgxgq1ueuz4X9
@mirchaemanuel
mirchaemanuel force-pushed the fix/555-autovivify-missing-parent branch from 3ac326a to 074c195 Compare July 20, 2026 10:18

nahime0 commented Jul 22, 2026

Copy link
Copy Markdown
Member

Maintainer follow-up after bringing this branch current with main:

  • Merged origin/main at 40737447b into the contributor branch (1d4073f21). The only conflict was src/ir/mod.rs; the resolution preserves both main's PhpTypePredicate export and this PR's fetch-for-write integration.
  • Re-audited the implementation against Chained writes through a missing array element fail to autovivify the parent #555. The fix still addresses the root cause: intermediate elements are fetched in write context and installed into parent storage, rather than mutating a detached read result. No P0/P2 correctness issue was found in the covered missing-parent semantics.
  • Aligned the PR with the typed EIR runtime boundary introduced by refactor(builtins): complete the typed EIR backend migration #582: the magic Immediate::I64(0x4657) dispatch is gone, replaced by RuntimeCallTarget::ArrayFetchForWrite with a typed signature, stable textual EIR identity, and typed backend dispatch.
  • Added an EIR regression that pins runtime.array.fetch_for_write.
  • Added an explicit CLI regression that runs the issue repro with both --ir-opt=on and --ir-opt=off, checking output, warning suppression, and heap cleanliness. The focused autovivification module is now 15 tests.
  • Updated the stale comment on the three-level concrete-intermediate regression: that path is now covered by fetch-for-write normalization.

Local validation on the new head ce515a915:

  • cargo build
  • cargo test --test codegen_tests arrays::nested_autovivify — 15/15
  • cargo test --test codegen_tests arrays::nested_mixed_write — 8/8
  • typed EIR lowering regression — 1/1
  • cargo fmt --all -- --check
  • git diff --check
  • assembly-comment alignment check for the touched codegen/runtime files

GitHub now reports the PR as mergeable. Build/archive is green on macOS ARM64, Linux ARM64, and Linux x86_64; builtin-doc and image-sync checks are also green. The downstream test matrix is currently queued because repository runners are not taking jobs, with no failures reported yet. The remaining merge gate is therefore a green result from that queued matrix.

The documented scope limits (incompatible scalar intermediates, dynamic Mixed keys on concrete locals/ArrayAccess intermediates, and typed non-boxed associative entries) remain unchanged and are separate from the missing-parent case fixed here.

…ssing-parent

# Conflicts:
#	tests/codegen/arrays/mod.rs
@nahime0
nahime0 merged commit 3746fe9 into illegalstudio:main Jul 23, 2026
113 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:codegen Touches target-aware assembly or backend lowering. area:eir Touches EIR definitions, lowering, validation, or passes. area:runtime Touches runtime helpers, GC, ownership, or bridge runtimes. size:m Medium-sized pull request. type:fix Corrects broken or incompatible behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chained writes through a missing array element fail to autovivify the parent

2 participants