Skip to content

fix(ir_lower): a keyed spread binds by name through a dynamic call target - #1048

Open
Guikingone wants to merge 2 commits into
mainfrom
fix/685-dynamic-spread-hash
Open

Guikingone wants to merge 2 commits into
mainfrom
fix/685-dynamic-spread-hash

Conversation

@Guikingone

Copy link
Copy Markdown
Collaborator

Fixes #685.

new $class(...$named) failed to compile when $named held a string-keyed array:

EIR lowering error: EIR validation failed: OperandTypeMismatch {
    expected: "Heap(Array)", actual: Heap(Hash) }

The same call with named arguments written out, with a literal array, or with a
statically known class name all worked, so it needed a dynamic target AND a spread
AND string keys together.

Cause

The argument container was chosen from the SPELLING of the call: written-out named
arguments got the hash, everything else got the indexed array. A spread is not
syntactically named, so a string-keyed source was spread into an indexed container
and the mismatch was raised there.

A literal new $c(...["b" => 8]) escaped it because that spread is expanded into
real named arguments before lowering. A variable operand cannot be, so it survives
as a spread and reached the wrong container.

The change

The container now follows what the invoker will find in it rather than how the call
was written: a spread whose operand is statically an associative array needs the
hash, because those keys bind by name. The hash builder gained the matching spread
arm, using the __rt_hash_spread path the array-literal lowering already had --
which preserves string keys and reindexes integer ones from the destination's own
largest integer key, exactly the binding PHP performs.

A spread whose type is only known as Mixed keeps the indexed container it has
always had. That is pre-existing behaviour for a boxed operand and is deliberately
not changed here.

Two builders, not one

$obj->$method(...) and $class::method(...) desugar into call_user_func, whose
container is built by a DIFFERENT function -- the one that also places by-reference
argument markers. It chose the same way and failed the same way, so it gets the same
routing and its own spread arm. The tests cover the two builders separately for that
reason: fixing one does not imply the other.

Verified

Fourteen shapes against host PHP 8.5.10, all byte-identical: a dynamic constructor,
a dynamic method and a dynamic static target; one key, several keys, integer keys; a
leading positional before the spread; two spreads in one call; a spread naming only
some parameters; a typed int parameter; a string value; and a call inside a loop so
the container is rebuilt per iteration.

The routing was confirmed load-bearing by disabling it: all three tests fail with the
original OperandTypeMismatch.

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

Three pre-existing bugs found while testing, all filed

None is caused by this change, and each was confirmed with --emit-ir to run on a
path the change does not touch:

The heap-debug assertion this commit would otherwise carry is absent because of
#1047: asserting a clean heap here would pin that leak to increments it has nothing
to do with.

Docs and example

docs/php/functions.md shows the mapping through all three dynamic targets next to
the existing static-target examples. examples/dynamic-dispatch/main.php gains a
Viewport reached through a variable class, a variable method and a variable static
call, plus a spread mixing integer and string keys to show that the integer ones stay
positional. The whole example is byte-identical to host PHP 8.5.10.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

@github-actions github-actions Bot added area:eir Touches EIR definitions, lowering, validation, or passes. size:s Small pull request. type:fix Corrects broken or incompatible behavior. 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; no outstanding blocking failure remains in the reviewed changes.

Summary

This PR fixes dynamic constructor, method, and static calls whose spread operand is statically known to be associative, routing those arguments through hash-backed descriptor containers so string keys bind by parameter name.

  • Adds associative-spread handling to both descriptor argument-container builders.
  • Preserves the existing indexed path for spreads whose static type is indexed or Mixed.
  • Adds end-to-end PHP compatibility coverage for dynamic constructors, methods, static calls, partial named arguments, typed parameters, mixed keys, and repeated invocation.
  • Updates the dynamic-dispatch example and PHP function documentation.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Dynamic call arguments] --> B{Named argument or statically associative spread?}
    B -->|Yes| C[Build hash descriptor container]
    C --> D[Spread with __rt_hash_spread]
    D --> E[String keys bind by parameter name]
    D --> F[Integer keys remain positional]
    B -->|No| G[Build indexed descriptor container]
    G --> H[Existing indexed or Mixed spread behavior]
    E --> I[Dynamic constructor / method / static invoker]
    F --> I
    H --> I
Loading

Reviews (4) · Last reviewed commit: "test(oop): repin the dynamic-dispatch ex..."

Comment thread src/ir_lower/expr/descriptor_calls.rs
@Guikingone Guikingone self-assigned this Sep 16, 2026
@Guikingone
Guikingone requested a review from nahime0 September 16, 2026 16:44
@Guikingone
Guikingone force-pushed the fix/685-dynamic-spread-hash branch from 362c91d to 516cf92 Compare September 16, 2026 21:01
…rget

`new $class(...$named)` failed to compile when `$named` held a string-keyed array:

    EIR lowering error: EIR validation failed: OperandTypeMismatch {
        expected: "Heap(Array)", actual: Heap(Hash) }

The same call with named arguments written out, with a literal array, or with a
statically known class name all worked, so it needed a dynamic target AND a spread
AND string keys together.

## Cause

The argument container was chosen from the SPELLING of the call: written-out named
arguments got the hash, everything else got the indexed array. A spread is not
syntactically named, so a string-keyed source was spread into an indexed container
and the mismatch was raised there.

A literal `new $c(...["b" => 8])` escaped it because that spread is expanded into
real named arguments before lowering. A variable operand cannot be, so it survives
as a spread and reached the wrong container.

## The change

The container now follows what the invoker will find in it rather than how the call
was written: a spread whose operand is statically an associative array needs the
hash, because those keys bind by name. The hash builder gained the matching spread
arm, using the `__rt_hash_spread` path the array-literal lowering already had --
which preserves string keys and reindexes integer ones from the destination's own
largest integer key, exactly the binding PHP performs.

A spread whose type is only known as `Mixed` keeps the indexed container it has
always had. That is pre-existing behaviour for a boxed operand and is deliberately
not changed here.

## Two builders, not one

`$obj->$method(...)` and `$class::method(...)` desugar into `call_user_func`, whose
container is built by a DIFFERENT function -- the one that also places by-reference
argument markers. It chose the same way and failed the same way, so it gets the same
routing and its own spread arm. The tests cover the two builders separately for that
reason: fixing one does not imply the other.

## Verified

Fourteen shapes against host PHP 8.5.10, all byte-identical: a dynamic constructor,
a dynamic method and a dynamic static target; one key, several keys, integer keys; a
leading positional before the spread; two spreads in one call; a spread naming only
some parameters; a typed `int` parameter; a string value; and a call inside a loop so
the container is rebuilt per iteration.

The routing was confirmed load-bearing by disabling it: all three tests fail with the
original `OperandTypeMismatch`.

`cargo test --test codegen_tests -- codegen::callables codegen::objects` passes in
full (709), as do the 1680 lib tests and 1534 error tests.

## Three pre-existing bugs found while testing, all filed

None is caused by this change, and each was confirmed with `--emit-ir` to run on a
path the change does not touch:

- #1045 -- a program with several dynamic `new` calls carrying NAMED arguments reads
  a corrupted class name and dies with `Class "<garbage>" not found`. The repro has
  no spread in it at all.
- #1046 -- spreading an EMPTY array drops integer parameter defaults:
  `function f($a = 0){} $e = []; f(...$e);` prints nothing where PHP prints `0`.
  A plain function with a static name is enough.
- #1047 -- the descriptor invoker's hash argument container is never released, so
  every call through it leaks. A `$f(b: 8)` with no spread leaks one allocation per
  call today.

The heap-debug assertion this commit would otherwise carry is absent because of
#1047: asserting a clean heap here would pin that leak to increments it has nothing
to do with.

## Docs and example

`docs/php/functions.md` shows the mapping through all three dynamic targets next to
the existing static-target examples. `examples/dynamic-dispatch/main.php` gains a
`Viewport` reached through a variable class, a variable method and a variable static
call, plus a spread mixing integer and string keys to show that the integer ones stay
positional. The whole example is byte-identical to host PHP 8.5.10.

Fixes #685

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
`examples/dynamic-dispatch/main.php` is pinned by
`test_example_dynamic_dispatch_compiles_and_runs` through `include_str!`, and the
previous commit appended four lines to it without updating the expectation. The
example itself is correct -- it was verified byte-for-byte against host PHP 8.5.10
before being committed -- so only the assertion was stale.

Failed on all three architectures in the `16/16` codegen shard, which is why it did
not show up in the targeted runs.

The expectation is now written as a `concat!` of one line each, so the next change to
the example produces a one-line diff instead of a rewritten string literal, and the
docblock names what the new lines cover.

Of the five examples this PR series touched, `dynamic-dispatch` is the only one any
test pins; `grep -rho 'include_str!("[^"]*examples/[^"]*")' tests/` lists the twenty
that are.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@Guikingone
Guikingone force-pushed the fix/685-dynamic-spread-hash branch from 516cf92 to 6b978ed 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:eir Touches EIR definitions, lowering, validation, or passes. size:s Small pull request. type:fix Corrects broken or incompatible behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new $class(...$assoc) with a string-keyed spread fails EIR validation

1 participant