fix(strings): implode() reads its array's real element layout, not an assumed one by Guikingone · Pull Request #1054 · illegalstudio/elephc

Guikingone · 2026-09-16T13:51:11Z

Fixes #689. Fixes #640. Fixes #1081.

function f(?array $a): void { echo implode(',', $a); }
f([1, 2]);        // PHP: 1,2      elephc: SIGSEGV
f([1.5, 2.5]);    // PHP: 1.5,2.5  elephc: SIGSEGV
f([true, false]); // PHP: 1,       elephc: ,
f(["a", "b"]);    // PHP: a,b      elephc: a,b   <- the only one that worked

Dropping the ? made the identical program correct, which is the tell: the declared
NULLABILITY is what boxes the operand, and a boxed operand tells the renderer nothing
about its elements.

What the renderer was assuming

__rt_implode dispatched on exactly one element layout: boxed Mixed cells (tag 7) took a
per-element cast, and everything else was read as a 16-byte {pointer, length} string
slot. A raw scalar element is 8 bytes and carries no length, so an int's VALUE was
dereferenced as a pointer, a double's bits likewise, and a bool pair read as one
{ptr, len} produced a zero length — 1, became ,.

Strings matched the assumption, which is why the bug looked type-specific rather than
layout-specific.

Statically typed arrays escaped it because the LOWERING picks a renderer from the element
type: array<int> gets __rt_implode_int, array<bool> gets __rt_implode_bool. A
mixed or union operand has no element type to pick from, so it fell through to the
string renderer.

The layout is written down; it just was not read

Every indexed array carries its element value_type in the packed kind word at
[ptr - 8] — the same field the tag-7 check already read. __rt_implode now dispatches
on all of it: 0 int, 2 float, 3 bool, 7 boxed Mixed, string slot otherwise.

The scalar arms format through __rt_itoa/__rt_ftoa and obey the CURSOR invariant this
emitter already documents: the live destination is published as _concat_off before each
conversion, because those helpers format into the very buffer the join is filling. Without
it the next element would be written over the separator just copied. false renders as the
empty string rather than "0", so it skips the formatter entirely.

That also fixes #640, the same mechanism reached from the other side: a homogeneous
array<float> had no renderer at all (unsupported EIR backend feature: implode array element PHP type Float) and now goes through the generic one's float arm. A dedicated
__rt_implode_float would have had to re-implement the cursor discipline the generic loop
already has.

The header has to be TRUE, which it was not for appends

__rt_array_push_int takes float and bool payloads too — same 8-byte slot — and stamps
value_type 0 on the first append, which says "int". An array built by appending therefore
described itself wrongly:

$f = []; for (...) { $f[] = $n / 8; }
implode(',', $f)   // 4593671619917905920,4598175219545276416, ...

so the append restamps the header for float and bool elements. When the checker still sees
the array as empty — $a = [] appended inside a loop keeps its array<never> back-edge
type at every push — the value being written is the only description, and that is what gets
stamped.

The other half of the argument path

A boxed operand can also hold a HASH, or no array at all, and both were read as an indexed
array: a hash printed its header words, and a genuinely null ?array segfaulted where PHP
raises a TypeError.

emit_boxed_implode_array_source decides at run time. A hash is converted to its values
exactly as a statically typed one is; an indexed array is retained. Both branches answer
with an OWNED array, which is what lets the join site release its operand with one
unconditional decref instead of a run-time flag.

The TypeError is raised from a separate guard emitted before the join site parks the
glue on the stack or takes any reference — from there the frame is what the surrounding
try left, so the throw is catchable and leaks nothing.

PHP's messages are reproduced exactly, including that null is worded against the
string-separator overload, that everything else is worded against the declared ?array,
and that a boolean is named by its VALUE (true given, never bool given). An object is
named by its class, read from the same metadata table get_class() uses.

The throw has to be visible to the optimizer

implode() is otherwise pure, so the first working guard raised its diagnostic and nothing
caught it: a try installs no handler around a call that cannot throw, and an unused call
is eliminable along with its diagnostic. The effect summary is now decided per call site —
MAY_THROW only when the operand is boxed, which is the only way the guard can fail. A
statically typed array operand keeps the pure summary and stays eliminable.

Verified

Against host PHP 8.5.10, byte-identical: the issue's four element types plus empty,
heterogeneous, PHP_INT_MAX, -0.0 and exponential floats; through ?array, mixed,
array|string, a property, a local copy and the ?? [] workaround; the keyed and
sparse-key forms; the one-argument join() form; a three-byte separator; arrays built by
appending in a loop; and all seven TypeError spellings, caught and printed.

--heap-debug reports leak summary: clean over 64 iterations for both the retained
indexed payload and the converted hash values.

The eleven new tests were confirmed load-bearing by reverting the source changes: all
eleven fail, seven of them by aborting the test process.

cargo test --test codegen_tests -- codegen::strings codegen::arrays codegen::runtime_gc codegen::array_basics passes in full (1345), as do 1915 unit tests and 1534 error tests.
The two emitter unit tests that pin the cursor discipline now count one publish per
formatting arm plus the finalizer, named rather than a literal.

Known, unchanged, and not this

A caught TypeError leaks one block per throw. It is not this guard's: count()'s own
runtime TypeError leaks identically on main, so it belongs to the exception-object
ownership family.

Docs and example

docs/php/strings.md states that implode() takes an indexed or associative array of any
element type, including one reached through a slot whose shape is only known at run time,
and that a non-array throws \TypeError.

examples/string-ops/main.php joins ints, floats, bools and a hash through a ?array
parameter beside the existing explode/implode pair, and shows the TypeError being
caught. Its output is verbatim host PHP 8.5.10.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

greptile-apps · 2026-09-16T13:57:53Z

Confidence Score: 4/5

The implementation appears behaviorally sound, but the repository’s required builtin name-resolution coverage must be added before merging.

Findings

Builtin spelling coverage missing ▶

Fix with agent prompt

### Issue 1
tests/codegen/strings/implode_boxed.rs:92-98
The new regression coverage invokes only lowercase, unqualified `implode()` and `join()`. This violates the repository directive requiring a case-insensitive or namespaced invocation when changing a PHP-visible builtin. That required name-resolution coverage must be added before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

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

Summary

This PR corrects implode() and join() for arrays whose shape or element layout is known only at runtime.

Dispatches generic joins using the array’s recorded element layout for integers, floats, booleans, strings, and boxed mixed values.
Guards boxed operands, converts associative arrays to dense value arrays, and raises catchable PHP-compatible TypeErrors for non-arrays.
Corrects scalar layout tags on array append and models possible throws in builtin effects.
Adds output, ownership, optimizer-sensitive, nested-container, and documentation coverage.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[implode or join call] --> B{Operand statically typed?}
    B -->|Yes| C[Select renderer from element type]
    B -->|No: Mixed or Union| D[Inspect boxed runtime tag]
    D -->|Indexed array| E[Retain dense array]
    D -->|Associative array| F[Copy values into dense array]
    D -->|Not an array| G[Raise catchable TypeError]
    E --> H[Read array value_type tag]
    F --> H
    C --> I[Render elements]
    H --> I
    I --> J[Joined string]

_{Reviews (6) · Last reviewed commit: "test(strings): pin implode() over a fore..."}

Guikingone · 2026-09-18T13:26:31Z

I tried to rebase this onto current main and stopped, because it is not a mechanical rebase — and the attempt surfaced something worth recording before anyone else tries.

The defect is still live. On main at 2d2af3fdc0:

function f(?array $a): void { echo implode(',', $a), "\n"; }
f(["a", "b"]);   // a,b
f([1, 2]);       // SIGSEGV (exit 139)

So the PR is still needed; it just cannot be replayed as-is.

Why the rebase is not mechanical. main has since reworked the same emitter (the
grow-into-an-owned-block work for #515): the destination can now be either the shared scratch
or a GROWN heap block, and every cursor publish branches on which. This branch adds four
formatting element arms to the same loop. The emitter halves merge cleanly — git only flags the
assertions — but the merged result is not correct:

sub x14, x9, x14      (arm64)   5 occurrences   ← one per formatting arm + finalizer
ldr x14, [sp, #88]    (arm64)   2 occurrences   ← only main's two sites

The publish happens at all five sites, but the grown-destination fallback only exists at
two of them. The arms this branch adds were written against the pre-rework emitter, where
there was only ever a scratch destination, so they restore the wrong offset once a join has
grown. Making the assertions agree would hide that, not fix it.

What it needs: the four added arms re-derived on top of the current two-destination
discipline, then the cursor tests re-pinned against the real counts. That is implementation
work on the emitter, on both architectures, not a conflict resolution — so I left the branch at
its pre-rebase state rather than push a half-merged one.

Every other PR in this batch is now rebased onto main and mergeable.

Guikingone · 2026-09-18T13:55:15Z

Re-derived on top of main's two-destination cursor discipline and pushed; this branch is mergeable again.

What the conflict actually was. This branch added three formatting element arms (raw int, raw float, raw true) and gave them their own cast prologue, which open-coded a scratch-only _concat_off publish — correct against the emitter as it stood when the branch was written, where the destination was always the shared scratch. main has since made the destination either the scratch or a GROWN owned block, and routed both of its own publish sites through a shared emit_implode_publish_offset_{aarch64,x86_64} helper that branches on which.

The merged result kept both halves and was internally inconsistent — visible as the three assertions disagreeing:

sub x14, x9, x14      5   ← every arm publishes
str x14, [x13]        5   ← every arm stores
ldr x14, [sp, #88]    2   ← only main's two sites have the grown arm

The fix: the two added prologues now delegate to the shared helper instead of open-coding, so all five sites (four formatting arms + the finalizer) obey the same rule. The cursor tests assert FORMATTING_ARMS + 1 on all three strings, which is what makes a future open-coded site fail loudly rather than skew one count.

One correction to my earlier comment on this PR, where I said the added arms "restore the wrong offset once a join has grown". I went looking for a fixture that observes that and could not build one, so here is what I actually measured. The open-coded form publishes cursor - _concat_buf; __rt_itoa then writes at _concat_buf + offset, so the arithmetic cancels back to the real cursor, and the MIXED_CAST_HEADROOM reservation that runs first keeps the write in bounds. Joins of 108 KB of ints, 148 KB of floats and 80 KB of bools — all well past the 64 KiB scratch, all through the formatting arms — are byte-identical to PHP with the open-coded form and with the helper.

So this is not a live miscompile I fixed; it is a shared invariant these three arms were silently opting out of, working only by an accident of the offset arithmetic. Going through the helper is still the right resolution — but the PR should not claim a bug it does not have.

Verification

Issue implode() on a ?array segfaults for int/float elements and drops bools (string elements are fine) #689's repro, which is still live on main: f([1, 2]) through ?array segfaults there, prints 1,2 here. All four element kinds match host PHP 8.5.10.
The grown-destination joins above, byte-identical to PHP.
strings/implode_boxed.rs 9/9, runtime_gc/implode_boxed_operand.rs 2/2, implode unit tests 7/7, codegen::strings 361, -p elephc --lib 1684 — all green.

… assumed one function f(?array $a): void { echo implode(',', $a); } f([1, 2]); // PHP: 1,2 elephc: SIGSEGV f([1.5, 2.5]); // PHP: 1.5,2.5 elephc: SIGSEGV f([true, false]); // PHP: 1, elephc: , f(["a", "b"]); // PHP: a,b elephc: a,b <- the only one that worked Dropping the `?` made the identical program correct, which is the tell: the declared NULLABILITY is what boxes the operand, and a boxed operand tells the renderer nothing about its elements. `__rt_implode` dispatched on exactly one element layout: boxed Mixed cells (tag 7) took a per-element cast, and EVERYTHING ELSE was read as a 16-byte `{pointer, length}` string slot. A raw scalar element is 8 bytes and carries no length, so an int's VALUE was dereferenced as a pointer, a double's bits likewise, and a bool pair read as one `{ptr, len}` produced a zero length -- `1,` became `,`. Strings matched the assumption, which is why the bug looked type-specific rather than layout-specific. Statically typed arrays escaped it because the LOWERING picks a renderer from the element type: `array<int>` gets `__rt_implode_int`, `array<bool>` gets `__rt_implode_bool`. A `mixed` or union operand has no element type to pick from, so it fell through to the string renderer. Every indexed array carries its element `value_type` in the packed kind word at `[ptr - 8]` -- the same field the tag-7 check already read. `__rt_implode` now dispatches on all of it: 0 int, 2 float, 3 bool, 7 boxed Mixed, string slot otherwise. The scalar arms format through `__rt_itoa`/`__rt_ftoa` and obey the CURSOR invariant this emitter already documents: the live destination is published as `_concat_off` before each conversion, because those helpers format into the very buffer the join is filling. Without it the next element would be written over the separator just copied. `false` renders as the empty string rather than `"0"`, so it skips the formatter entirely -- PHP stringifies bool as `"1"`/`""`. That also fixes #640, which is the same mechanism reached from the other side: a homogeneous `array<float>` had no renderer at all (`unsupported EIR backend feature: implode array element PHP type Float`) and now goes through the generic one's float arm. A dedicated `__rt_implode_float` would have had to re-implement the cursor discipline the generic loop already has. `__rt_array_push_int` takes float and bool payloads too -- same 8-byte slot -- and stamps `value_type` 0 on the first append, which says "int". An array built by appending therefore described itself wrongly: $f = []; for (...) { $f[] = $n / 8; } implode(',', $f) // 4593671619917905920,4598175219545276416, ... so the append restamps the header for float and bool elements. When the checker still sees the array as empty -- `$a = []` appended inside a loop keeps its `array<never>` back-edge type at every push -- the value being written is the only description, and that is what gets stamped. A boxed operand can also hold a HASH, or no array at all, and both were read as an indexed array: a hash printed its header words, and a genuinely null `?array` segfaulted where PHP raises a TypeError. `emit_boxed_implode_array_source` decides at run time. A hash is converted to its values exactly as a statically typed one is; an indexed array is retained. Both branches answer with an OWNED array, which is what lets the join site release its operand with one unconditional decref instead of a run-time flag. The TypeError is raised from a SEPARATE guard emitted before the join site parks the glue on the stack or takes any reference -- from there the frame is what the surrounding `try` left, so the throw is catchable and leaks nothing. PHP's messages are reproduced exactly, including that null is worded against the string-separator overload, that everything else is worded against the declared `?array`, and that a boolean is named by its VALUE (`true given`, never `bool given`). An object is named by its class, read from the same metadata table `get_class()` uses. `implode()` is otherwise pure, so the first working guard raised its diagnostic and nothing caught it: a `try` installs no handler around a call that cannot throw, and an unused call is eliminable along with its diagnostic. The effect summary is now decided per call site -- `MAY_THROW` only when the operand is boxed, which is the only way the guard can fail. A statically typed array operand keeps the pure summary and stays eliminable. Against host PHP 8.5.10, byte-identical: the issue's four element types plus empty, heterogeneous, `PHP_INT_MAX`, `-0.0` and exponential floats; through `?array`, `mixed`, `array|string`, a property, a local copy and the `?? []` workaround; the keyed and sparse-key forms; the one-argument `join()` form; a three-byte separator; arrays built by appending in a loop; and all seven TypeError spellings, caught and printed. `--heap-debug` reports `leak summary: clean` over 64 iterations for both the retained indexed payload and the converted hash values. The eleven new tests were confirmed load-bearing by reverting the source changes: all eleven fail, seven of them by aborting the test process. `cargo test --test codegen_tests -- codegen::strings codegen::arrays codegen::runtime_gc codegen::array_basics` passes in full (1345), as do 1915 unit tests and 1534 error tests. The two emitter unit tests that pin the cursor discipline now count one publish per formatting arm plus the finalizer, named rather than a literal. A caught `TypeError` leaks one block per throw. It is not this guard's: `count()`'s own runtime TypeError leaks identically on `main`, so it belongs to the exception-object ownership family. `docs/php/strings.md` states that `implode()` takes an indexed or associative array of any element type, including one reached through a slot whose shape is only known at run time, and that a non-array throws `\TypeError`. `examples/string-ops/main.php` joins ints, floats, bools and a hash through a `?array` parameter beside the existing `explode`/`implode` pair, and shows the TypeError being caught. Its output is verbatim host PHP 8.5.10. Fixes #689 Fixes #640 Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

…nt throw summary `implode`/`join` moved from a static effect summary to a shared one, which the generated builtin pages and the registry snapshot record. Produced by the CI sequence: `cargo build --example gen_builtins --features curl`, `python3 scripts/docs/extract_builtins.py --render --force`, `python3 scripts/docs/gen_module_sections.py` (no module page changed). Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

The boxed shapes already pinned here all reach the renderer through a declared slot -- a nullable property, a `mixed` parameter, a union. A cast of a call result is a fourth way in, and the one that fails worst: on `main`, `implode(",", (array) f())` where `f(): mixed` returns `[1, 2]` exits 139 (SIGSEGV), so there is no wrong output to notice before the process dies. All seven layouts are in the fixture because the cast carries the array's own `value_type` tag through, which is the only thing the renderer can dispatch on. Output verified byte-identical to host PHP 8.5.10. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

Guikingone · 2026-09-18T18:32:15Z

Rebased onto main @ bf5437b687, plus one test for a shape this PR fixes that was not pinned: an (array) cast of a mixed return value.

function f(): mixed { return [1, 2]; }
echo implode(",", (array) f());

On a build of main that program exits 139 (SIGSEGV) — the int elements' values are dereferenced as pointers, so there is no wrong output to notice before the process dies. With this branch it prints 1,2, and all seven layouts (int, float, bool, string, empty, single, signed) match host PHP 8.5.10 byte for byte.

Worth recording where I ran into it: PR #968's branch carries a tests/implode_element_layout_tests.rs asserting exactly these seven cases, and its CI fails there with binary failed (exit None) — it has the test but not this PR's runtime change. That file belongs with this work, so I have pinned the same coverage here in tests/codegen/strings/implode_boxed.rs and will drop the stray copy from #968.

Verified locally after the rebase: every test matching implode 42/42, codegen::strings 362/362, codegen::arrays 578/578, runtime_gc 303/303.

greptile-apps · 2026-09-18T18:34:43Z

+echo implode(",", (array) ints()), ";";
+echo implode(",", (array) floats()), ";";
+echo implode(",", (array) bools()), ";";
+echo implode(",", (array) strs()), ";";
+echo implode(",", (array) none()), ";";
+echo implode(",", (array) one()), ";";
+echo implode(",", (array) signed());


Builtin spelling coverage missing

The new regression coverage invokes only lowercase, unqualified implode() and join(). This violates the repository directive requiring a case-insensitive or namespaced invocation when changing a PHP-visible builtin. That required name-resolution coverage must be added before merging.

Context Used: AGENTS.md (source)

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/codegen/strings/implode_boxed.rs Line: 92-98 Comment: **Builtin spelling coverage missing** The new regression coverage invokes only lowercase, unqualified `implode()` and `join()`. This violates the repository directive requiring a case-insensitive or namespaced invocation when changing a PHP-visible builtin. That required name-resolution coverage must be added before merging. **Context Used:** AGENTS.md ([source](https://github.com/illegalstudio/elephc/blob/main/AGENTS.md)) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

`tests/implode_element_layout_tests.rs` asserts that `implode()` renders every element layout of a `mixed`-typed array. That is illegalstudio#1054's subject, not this branch's: the runtime layout dispatch it exercises is not here, so the test fails with `binary failed (exit None)` -- the SIGSEGV it was written to catch -- and reddened `Non-Codegen Tests` on both Linux arches. The same seven cases are now pinned in illegalstudio#1054's own `tests/codegen/strings/implode_boxed.rs`, beside the change that makes them pass, so nothing is lost by removing the copy here. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

…tainer Issue #1081 reported `implode()` segfaulting on a `foreach` value bound from `array_chunk()`. It is this PR's defect, seen from a third angle: a loop binding taken out of a container arrives boxed exactly like a nullable slot, so the renderer fell through to the string-layout assumption and dereferenced each int element's value as a pointer. Reading the same chunk BY INDEX was fine, which is what made it read as an `array_chunk()` bug rather than a renderer one. Every shape from the issue's perimeter table now matches host PHP 8.5.10: the `foreach`-bound chunk, the `preserve_keys` form, a float chunk, and a plain nested literal. Each lives in its own function because `array_chunk()`'s inner element type merges across call sites in one scope, and a merged `Mixed` inner element is a separate backend gap that would mask what this test is for. Fixes #1081 Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

github-actions Bot added the scope:multi-area Touches more compiler areas than the automatic area-label cap. label Sep 16, 2026

Guikingone self-assigned this Sep 16, 2026

Guikingone requested a review from nahime0 September 16, 2026 16:19

Guikingone force-pushed the fix/689-implode-nullable-array branch from d240bd3 to 3ea88a6 Compare September 16, 2026 20:58

Guikingone force-pushed the fix/689-implode-nullable-array branch from 3ea88a6 to 5b49b1b Compare September 18, 2026 13:54

This comment has been minimized.

Sign in to view

Guikingone added 3 commits September 18, 2026 20:16

Guikingone force-pushed the fix/689-implode-nullable-array branch from 5b49b1b to f21fa45 Compare September 18, 2026 18:31

greptile-apps Bot reviewed Sep 18, 2026

View reviewed changes

Guikingone mentioned this pull request Sep 18, 2026

feat(opcache): give the OPcache surface real behaviour #968

Open

This was referenced Sep 18, 2026

implode() on a foreach value bound from array_chunk() segfaults #1081

Open

fix(ir_lower): read a builtin's return type when it is an array-literal element #1097

Open

Navigation Menu

Uh oh!

fix(strings): implode() reads its array's real element layout, not an assumed one - #1054

fix(strings): implode() reads its array's real element layout, not an assumed one#1054
Guikingone wants to merge 4 commits into
mainfrom
fix/689-implode-nullable-array

Guikingone commented Sep 16, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

Guikingone commented Sep 18, 2026

Uh oh!

Guikingone commented Sep 18, 2026

Uh oh!

This comment has been minimized.

Guikingone commented Sep 18, 2026

Uh oh!

greptile-apps Bot Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Guikingone commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What the renderer was assuming

The layout is written down; it just was not read

The header has to be TRUE, which it was not for appends

The other half of the argument path

The throw has to be visible to the optimizer

Verified

Known, unchanged, and not this

Docs and example

Uh oh!

greptile-apps Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

Guikingone commented Sep 18, 2026

Uh oh!

Guikingone commented Sep 18, 2026

Uh oh!

This comment has been minimized.

Guikingone commented Sep 18, 2026

Uh oh!

greptile-apps Bot Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Guikingone commented Sep 16, 2026 •

edited

Loading

greptile-apps Bot commented Sep 16, 2026 •

edited

Loading