feat(objects): a property default may hold array literals, at any depth - #1069
Conversation
github-actions
Bot
added
area:codegen
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| LiteralDefaultValue::BoxedAssocArray { | ||
| value_type, | ||
| entries, | ||
| } => { | ||
| emit_assoc_array_literal_default_to_result(ctx, value_type, entries)?; | ||
| // The OWNED boxer, for the same reason as the positional arm above. | ||
| crate::codegen::emit_box_current_owned_value_as_mixed( | ||
| ctx.emitter, | ||
| &PhpType::AssocArray { | ||
| key: Box::new(PhpType::Mixed), | ||
| value: Box::new(value_type.clone()), | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new static BoxedAssocArray emitter is not exercised by the added tests. The only static fixture uses a positional, non-boxed public static array default, so regressions in this separate keyed boxing and ownership path would not be caught. Please add a static keyed default on a nullable, mixed, or union property.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/codegen/block_emit.rs
Line: 1168-1181
Comment:
**Static Boxed Path Untested**
The new static `BoxedAssocArray` emitter is not exercised by the added tests. The only static fixture uses a positional, non-boxed `public static array` default, so regressions in this separate keyed boxing and ownership path would not be caught. Please add a static keyed default on a nullable, mixed, or union property.
---
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!
There was a problem hiding this comment.
Addressed in 7b4328e: test_static_keyed_defaults_on_boxed_slots in tests/codegen/objects/nested_array_property_defaults.rs covers a static ?array keyed default (flat), a static mixed keyed default with a nested element, and a static ?array keyed default two levels deep, all through the static BoxedAssocArray emitter in block_emit.rs.
class C { public array $x = [[1], [2]]; } // was: compile error
Declaring the class was enough to be refused -- no read, no write, no
instantiation -- while the same literal was already accepted as a local, a
parameter default and a class constant.
`LiteralArrayElement` is the set of things a default can materialize without
evaluating code, and it was flat: Int, Bool, Float, Str, Null. A container
element had nowhere to go, so it fell through to the unsupported-default error,
which names the PROPERTY's type and so read as if the slot were at fault. It was
never the slot: a plain `array` property failed exactly as `?array` and `mixed`
did.
Make the form recursive. A container element materializes into its own
container, which the enclosing one then owns, so the tree is allocated with the
object and released exactly once with it. The two enclosing paths transfer that
ownership differently and both had to be taught it: the indexed path boxes into
a Mixed cell with the OWNED boxer, so the box's retain plus the builder's
release nets to a transfer; the hash path hands `__rt_hash_set` the pointer
directly, because it does not retain what it stores -- it only releases what it
overwrites.
Also adds `BoxedAssocArray`, the keyed counterpart of `BoxedArray`. The issue's
`?array $x = ["k" => [1]];` row was two independent gaps: a keyed literal had no
boxed form at ALL, so `public ?array $x = ["k" => 1];` was refused on its own
while `public ?array $x = [1, 2];` beside it compiled.
Verified over the issue's table plus 13 further spellings, every one
byte-identical to host PHP 8.5.10. Ownership measured rather than assumed: 600
objects carrying three different nested shapes closed at allocs=7401 frees=7401,
`leak summary: clean`, with a flat 2832-byte peak. Instances do not share
storage, and a copy taken out of a default outlives the object it came from.
Out of scope, left as it was: `public iterable $x = ["k" => 1];` fails earlier
and differently (`prop_set assigning PHP type AssocArray`), on main as much as
here.
Fixes #1052.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
500c2c6 to
c0447be
Compare
|
Rebased onto current Heads-up on an overlap found while rebasing. This PR and #1069 both add the same
They are the same addition, so whichever merges first, the other will conflict on that variant |
Raised in review. The static fixture used a positional default on a plain `array` slot, which never reaches the keyed boxing emitter: `block_emit`'s static path is separate from `property_defaults`' instance one, and the `BoxedAssocArray` arm is separate again from the `BoxedArray` arm beside it. A regression in the static keyed boxing or its ownership would not have been caught. Covers a nullable flat keyed default, a `mixed` slot holding a nested indexed literal, and a two-level keyed one. The flat spelling is included because it was refused on its own before this change, independently of any nesting. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
…al-property-default # Conflicts: # examples/classes/main.php # src/codegen/literal_defaults.rs # src/codegen/lower_inst/objects/property_defaults.rs Co-authored-by: Vincenzo Petrucci <nahime0@users.noreply.github.com>
| ctx.emitter.instruction("mov x3, x0"); // pass the nested container pointer as the hash value low word | ||
| ctx.emitter.instruction("mov x4, xzr"); // container hash values do not use the high payload word |
There was a problem hiding this comment.
The new nested-container instruction group lacks the required // -- description -- block comment. The same omission occurs in the corresponding x86_64 group and both Mixed-storage groups at lines 1033–1034, 1075–1076, and 1117–1118. The repository’s assembly comment policy requires these block comments before related emitter.instruction(...) calls, so this requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/codegen/literal_defaults.rs
Line: 987-988
Comment:
**Assembly Group Comments Missing**
The new nested-container instruction group lacks the required `// -- description --` block comment. The same omission occurs in the corresponding x86_64 group and both Mixed-storage groups at lines 1033–1034, 1075–1076, and 1117–1118. The repository’s assembly comment policy requires these block comments before related `emitter.instruction(...)` calls, so this requirement must be satisfied 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!
There was a problem hiding this comment.
Not applied, on purpose. In literal_defaults.rs no arm of these four materializers uses a // -- description -- group line: each arm is two instructions with the mandatory inline comment at column 81, introduced by an explanatory comment on the arm itself. The new Array/AssocArray arms follow exactly that layout. Adding group lines to the four new arms alone would leave the file inconsistent with its own siblings; if the file should switch to group lines, that is a file-wide change for a separate PR.
There was a problem hiding this comment.
You're right. The surrounding materializers consistently use an arm-level explanatory comment plus column-81 inline comments; they do not use // -- description -- group headers. The four new arms follow that established file-local convention, so my comment does not apply and no change is needed.
Co-authored-by: Vincenzo Petrucci <nahime0@users.noreply.github.com>
…al-property-default # Conflicts: # tests/codegen/runtime_gc.rs Co-authored-by: Vincenzo Petrucci <nahime0@users.noreply.github.com>
…al-property-default # Conflicts: # tests/codegen/runtime_gc.rs
3 participants
Declaring the class was enough to be refused — no read, no write, no
instantiation — while the same literal was already fine as a local, a parameter
default and a class constant.
The cause, as filed
LiteralArrayElementis the set of things a default can materialize withoutevaluating code, and it was flat (
Int,Bool,Float,Str,Null). Acontainer element had nowhere to go and fell through to
unsupported_literal_default, whose message names the property's type —which is why it read as if the slot were the problem. It never was: a plain
arrayproperty failed exactly as?arrayandmixeddid.The fix
Make the form recursive: a container element materializes into its own
container, which the enclosing one then owns. The tree is allocated during the
object's initialization and released exactly once with it.
The two enclosing paths transfer that ownership differently, and both had to
be taught it:
where the box's retain plus a release of the builder's reference nets to a
transfer;
__rt_hash_setthe pointer directly, because thathelper does not retain what it stores; it only releases what it overwrites.
Getting either backwards fails on opposite sides: retain-without-release leaks
the whole subtree per object, release-without-retain frees a child the object
still points at.
A second gap in the same table, landed by #1053
The issue's
?array $x = ["k" => [1]];row turned out to be two independentgaps: a keyed literal had no boxed form at all, only the positional spelling had
been taught to box. That half is
LiteralDefaultValue::BoxedAssocArray, which#1053 landed first (the same variant, arrived at independently); this branch is
merged on top of it and only adds the nesting. The two "second gap" rows below
therefore pass on
mainalready and are listed for completeness.Verification
The issue's table, plus 13 further spellings, every one byte-identical to host
PHP 8.5.10:
public array $x = [[1], [2]];2✓public ?array $x = [[1], [2]];public mixed $x = [[1], [2]];public array $x = ["k" => [1]];1✓public ?array $x = ["k" => [1]];public mixed $x = ["k" => [1]];public ?array $x = ["k" => 1];public mixed $x = ["k" => 1];public array $x = [[1], 2];public array $x = [[[1]]];public array $x = [[]];public array $x = [[1, "s", 2.5, null, true]];public array $x = [0 => [1], 5 => [2], "k" => [3]];public static array $s = [[1], [2]];public array $x = [1, 2];/["k" => 1]Ownership measured, not assumed. 600 objects carrying three different nested
shapes:
allocs=7401 frees=7401,leak summary: clean, peak 2832 bytes flatacross the run. Instances do not share storage (
$a->x[0][] = 99leaves a secondinstance at its default), and a copy taken out of a default outlives the object
it came from — the over-release side of the same contract.
Tests: 6 behaviour in
objects/nested_array_property_defaults.rs, 4 heap-debugin
runtime_gc/nested_property_defaults.rs. Suites green:objects260,runtime_gc302,oop619,-p elephc --lib1680 includinglowers_examples_corpus.docs/php/classes.mdreplaces the stated limitation, andexamples/classesgains a
Gridshowing both spellings and the per-instance storage.Out of scope
public iterable $x = ["k" => 1];fails earlier and differently(
prop_set assigning PHP type AssocArray), onmainas much as here. Not inthe issue's table; left alone rather than folded in.
Two pre-existing defects on the WRITE side become easier to reach now that a
nested default can be declared, and are deliberately not touched here since
neither involves the default path (both reproduce with the literal assigned in
the constructor instead):
$o->x[0][] = 9on a nested property element leaks the previous copy perstore, Array read-copy leaks: isset() ternary read-append-store and nested $prop[$k][] append leak the previous copy per iteration #923 (reproduction with a literal default added there);
$o->x[0][1] = 9on a nested property element is silently dropped, A nested index write into an array property element ($o->x[0][1] = 9) is silently dropped: the object keeps the stale inner array #1206.The heap-debug fixtures here cover the defaults' own allocation and release; the
behaviour test that writes through a default (
$a->x[0][] = 99) checksper-instance storage, not the heap.
Fixes #1052.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr