Skip to content

test(types): pin a narrowed union reaching a typed object parameter - #1061

Open
Guikingone wants to merge 4 commits into
mainfrom
fix/703-narrowed-object-argument-pin
Open

Guikingone wants to merge 4 commits into
mainfrom
fix/703-narrowed-object-argument-pin

Conversation

@Guikingone

Copy link
Copy Markdown
Collaborator

Closes #703.

The issue's reproducer answers 2 on current main, not a heap address — the miscompilation is
gone. What was missing is anything that would catch it coming back, so this adds the pin.

class Box { public int $num_rows = 0; }
function get(): Box|bool { $b = new Box(); $b->num_rows = 2; return $b; }
function readnum(Box $b): int { return $b->num_rows; }
$r = get();
if (!($r instanceof Box)) { exit(1); }
echo readnum($r), "|", $r->num_rows;   // 2|2, matching PHP 8.5.10

The EIR still looks the way the issue describes

The analysis in #703 is still accurate about the IR: the argument is Heap(Mixed) php=Box|bool
and the parameter is Heap(Object), because a union slot is boxed storage and the slot's type is
flow-insensitive even where the checker has narrowed it. There is no Union→Object coercion in
call_arg_coercion.rs.

The unbox is the backend's, emitted at the call site:

ldur x0, [x29, #-72]
bl __rt_mixed_unbox
mov x0, x1
bl _fn_readnum

What is pinned

Four fixtures:

  • the issue's own reproducer;
  • the four union flavours that share that boxed representation — Box|bool, ?Box, a
    three-member Box|bool|int, and a two-class Box|Other, each of which used to hand the callee
    a different heap address;
  • the five call shapes — plain function, instance method, static method, two arguments in one
    call, and an immediately-invoked closure;
  • one that reads the emitted assembly and asserts the unbox precedes the call.

That last one earns its place. The failure was silent — no diagnostic, no crash, a different
wrong number on every run — and a value assertion alone would keep passing if num_rows ever
moved to offset zero, where the box address and the object address read the same. It matches each
target's spelling of the call (bl _fn_readnum, bl fn_readnum, call fn_readnum), and the
leading indent is deliberate: the directive .globl _fn_readnum contains bl _fn_readnum as a
substring.

All four match host PHP 8.5.10 byte for byte, with --ir-opt both on and off.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

@github-actions github-actions Bot added area:triage No primary component could be inferred from changed paths. size:s Small pull request. type:test Changes tests or test infrastructure only. labels Sep 16, 2026
@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the previous coverage and documentation concerns are fully addressed, with no new actionable issue established.

Summary

This PR adds regression coverage ensuring narrowed, boxed union values reach typed object parameters as object pointers rather than boxed storage.

  • Covers four boxed union forms and five call shapes.
  • Exercises both optimized and optimizer-disabled EIR paths.
  • Adds a target-aware assembly assertion that checks for mixed-value unboxing before the typed call.
  • Introduces a scoped thread-local optimizer override for parallel-safe test execution.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
    U[Union-typed local] --> N[instanceof narrowing]
    N --> M[Heap Mixed storage]
    M --> X[Call-site mixed unbox]
    X --> O[Typed Heap Object parameter]
    O --> P[Correct property value]
Loading

Reviews (6) · Last reviewed commit: "test(types): find the call on every targ..."

Comment thread tests/codegen/types/narrowed_object_arguments.rs Outdated
Comment thread tests/codegen/types/narrowed_object_arguments.rs
Comment thread tests/codegen/support/compiler.rs
@Guikingone
Guikingone force-pushed the fix/703-narrowed-object-argument-pin branch from a2c320c to 8af6b65 Compare September 16, 2026 20:46
@Guikingone Guikingone self-assigned this Sep 17, 2026
@Guikingone
Guikingone requested a review from nahime0 September 17, 2026 16:38
    class Box { public int $num_rows = 0; }
    function get(): Box|bool { $b = new Box(); $b->num_rows = 2; return $b; }
    function readnum(Box $b): int { return $b->num_rows; }
    $r = get();
    if (!($r instanceof Box)) { exit(1); }
    echo readnum($r);

Issue #703 reports this printing a heap address instead of `2`: the callee read
`num_rows` off the Mixed box rather than the object. It answers `2` on current
main, and so do nine other shapes of the same thing, but nothing pinned it.

The EIR still looks exactly like the report says it does -- the argument is
`Heap(Mixed) php=Box|bool` and the parameter is `Heap(Object)`, because a union
slot is boxed storage and the slot's type is flow-insensitive even where the
checker has narrowed it. The unbox is the backend's, at the call site:

    ldur x0, [x29, #-72]
    bl __rt_mixed_unbox
    mov x0, x1
    bl _fn_readnum

Four fixtures: the issue's own reproducer, the four union flavours that share
that boxed representation (`Box|bool`, `?Box`, a three-member union, and a
two-class union), the five call shapes (function, instance method, static method,
two arguments, closure), and one that reads the emitted assembly.

The last one earns its place. The failure was silent -- no diagnostic, no crash,
a different wrong number per run -- and a value assertion alone would keep
passing if `num_rows` ever moved to offset zero, where the box address and the
object address read the same. It looks for the unbox before the call, under each
target's spelling of that call.

Closes #703

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
…he IR optimizer

Collecting the four union-producing calls into one array typed the element `mixed`,
so `?Box`, `Box|bool`, `Box|bool|int` and `Box|Other` all reached the callee through
a single `mixed`-to-object coercion -- one shape tested four times. Each result now
lives in its own local and is narrowed there, and the EIR shows four call sites
carrying four distinct declared types.

The same fixture runs again with the EIR optimizer off. `ELEPHC_IR_OPT` selects the
mode for a whole test process and the harness runs tests in parallel, so the override
is a thread-local (`without_ir_opt`), matching how the compiler already isolates
per-compilation state, and it is restored on the way out.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
…t of four

The assembly fixture listed one call spelling per target and missed the fourth
combination: linux-x86_64 emits `call _fn_readnum`, with the underscore, and the
list only carried the underscore on the `bl` forms. The shard failed with the
fixture's own message, `the fixture must call readnum`.

The mnemonic and the symbol prefix vary independently, so the search now walks
both axes instead of enumerating targets. The leading indent still does the work
it always did -- it is what keeps `.globl _fn_readnum` from matching.

Verified by running the fixture under ELEPHC_TEST_TARGET for macos-aarch64,
linux-aarch64 and linux-x86_64, and by checking that the previous list still
fails on linux-x86_64 with the CI message.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@Guikingone
Guikingone force-pushed the fix/703-narrowed-object-argument-pin branch from 69360e9 to 439084d Compare September 18, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:triage No primary component could be inferred from changed paths. size:s Small pull request. type:test Changes tests or test infrastructure only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instanceof-narrowed Object|bool local passed to a typed object parameter receives the Mixed box (silent garbage)

1 participant