Skip to content

fix(ir_lower): widen mismatched array/array branch merges to boxed mixed - #583

Merged
nahime0 merged 4 commits into
illegalstudio:mainfrom
mirchaemanuel:fix/549-match-array-merge-widen
Jul 22, 2026
Merged

fix(ir_lower): widen mismatched array/array branch merges to boxed mixed#583
nahime0 merged 4 commits into
illegalstudio:mainfrom
mirchaemanuel:fix/549-match-array-merge-widen

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Summary

Fixes #549match arms (and ternary / ?? / ??= merges) returning indexed arrays with different element types merged to a single element type; reading the other arm's array through the merged type misinterpreted the slot bytes (SIGSEGV on current main when the int arm is read as strings).

Root cause

Two cooperating gaps:

  • wider_type_for_merge resolved (Array, Array) and (AssocArray, AssocArray) as right.clone() — the last arm's element type won wholesale.
  • coerce_value_for_temp performed no container conversion, so the other arm's runtime array was stored raw and merely relabeled; element reads then decoded the wrong slot width/content (int slots read as string {ptr,len} pairs → fault in __rt_str_persist).

Fix

  • wider_type_for_merge now merges array/array and assoc/assoc elementwise, reusing the heterogeneous-literal rules (merge_ir_indexed_element_type / merge_ir_assoc_value_type): equal reprs keep the type, Never/Void yields the other side, any mismatch widens to boxed mixed.
  • The merge-temp store path routes containers through a new widen_container_value_for_temp helper: typed containers stored into Mixed-payload temps box their slots via the existing ArrayToMixed / HashToMixed ops. Borrowed and provisionally-owned sources are retained first so the conversion's copy-on-write split rewrites a private copy (per the PR fix(ir): release the producer's reference when boxing an owned value as Mixed (#484) #489 ownership rules); whole-boxed ?array cells flowing through ?? unbox with the owned-payload coercion before converting when the cell does not own its payload, so a caller's live array is never rewritten in place.
  • lower_array_to_mixed / lower_hash_to_mixed pass null and in-band null-container sentinels through unconverted on both targets (call-site guard, issue fix(codegen): guard container consumers against the null-container sentinel (chained-read miss segfault) #533 convention), so a missed read forwarded by an arm is never dereferenced.

This fixes match, full ternary, short ternary, ??, and ??= in one place — they share the same merge/store helpers. Target-neutral: AArch64 and x86_64 emissions for both guards.

Tests

18 regression tests in tests/codegen/control_flow/{match_expressions,ternary,nulls}.rs: both arm orders for match and ternary, assoc value-type mismatch, empty-array arm, nested arrays, variable (borrowed) arms, missed-read sentinel forwarding, and ?? with a ?array value side — owning and borrowed cells, including a live caller array passed twice and a 20-iteration loop — with heap-clean assertions under --heap-debug. Focused neighbor suites are green locally on macOS ARM64 (match 151, ternary 45, coalesce 63, nulls 32, plus array/hash/assoc/spread/optimizer filters, ~1,600 tests). The issue's repro prints PHP-parity output on both arms.

Notes

Match, ternary, `??`, and `??=` merges of indexed arrays (or hashes)
with different element types let the last arm's type win wholesale, so
the other arm's runtime slots were relabeled and read through the wrong
element repr — SIGSEGV when array<int> was read as array<string>.

`wider_type_for_merge` now merges container types elementwise, reusing
the heterogeneous-literal rules, and the merge-temp store path boxes
typed container slots via ArrayToMixed / HashToMixed. Borrowed sources
retain the payload first so the conversion's copy-on-write split leaves
live caller arrays untouched; whole-boxed `?array` cells flowing
through `??` route through the owned-payload unbox coercion for the
same reason. Null-container sentinels forwarded by missed reads pass
through both conversions unconverted (issue illegalstudio#533 convention).

Closes illegalstudio#549
@mirchaemanuel

Copy link
Copy Markdown
Contributor Author

Follow-ups from the adversarial review of this fix, all verified pre-existing on main (none are regressions of this PR): #585 (unguarded null-container sentinel in __rt_mixed_array_get on ternary-forwarded missed reads), #586 (merge temp leaks when read through ?? default), #587 (checker types heterogeneous merges as mixed, rejecting valid array uses that now run correctly at runtime).

@nahime0

nahime0 commented Jul 22, 2026

Copy link
Copy Markdown
Member

Maintainer follow-up after the current-main refresh:

Local validation on the refreshed branch:

  • cargo build — passed
  • 20 focused codegen/ownership regression tests — 20 passed, including both arm orders, associative arrays, COW preservation, heap-debug balance, sentinel passthrough, ternary/short-ternary, and nullable-array ??
  • assembly-comment alignment — passed
  • git diff --check origin/main...HEAD — clean

GitHub now sees head df2608fe6. Fresh CI is currently queued/in progress on that exact SHA; once the required matrix is green, I see no remaining blocker to merge.

@nahime0
nahime0 merged commit 52ea700 into illegalstudio:main Jul 22, 2026
113 checks passed
nahime0 pushed a commit that referenced this pull request Jul 26, 2026
… instead of bare mixed

The type checker collapsed a heterogeneous-element match/ternary/`??` array
merge (e.g. `[1, 2]` vs `["a", "b"]`) to bare `Mixed`, so PHP-valid array uses
of the result were rejected at compile time: passing it to a by-ref `array`
parameter, `in_array()`/`array_sum()`, and spread (`[...$r]`). The spread
rejection additionally emitted a misleading follow-on "Undefined variable"
diagnostic naming the assignment target.

`merge_match_arm_result_type` now mirrors the lowering-side `wider_type_for_merge`
array/array and assoc/assoc rules through a new pure `merge_array_branch_types`
helper: differing element (or key/value) types widen elementwise to `Mixed`, so
the merged value keeps an `array<mixed>` type and array operations type-check
just as they do for homogeneous merges. Non-array merges are untouched — scalar
unions stay `Mixed` and object|null merges stay nullable unions — and an
indexed-vs-associative mix still falls through to `Mixed`, matching lowering.

Runtime correctness of heterogeneous merges is tracked by #549 / PR #583; this
change only aligns the static type. The residual "Undefined variable" cascade
for genuinely-failing spreads (repro: `$x = 5; $s = [...$x]; count($s);`) is a
separate latent defect left for a follow-up.

Claude-Session: https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L
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. 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.

match arms returning arrays with different element types merge to a single element type: reading the other arm's array segfaults

2 participants