feat(enums): accept an enum case as a declared property default - #1026
Conversation
Greptile SummaryThis PR adds PHP-compatible enum-case defaults for directly declared instance and static properties while preserving semantic validation, lazy singleton materialization, and ownership.
Confidence Score: 5/5The PR appears safe to merge; both previous findings are fully addressed and no new actionable issue was introduced. The current lowering retains enum singletons before transferring them into owned property slots, deferred validation still rejects missing or incompatible constants, and focused regressions cover identity and reassignment behavior. The stale module documentation was also corrected. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
PHP["Property default: Level::Low"] --> Schema["Schema construction"]
Schema --> Deferred["Defer scoped-constant validation"]
Deferred --> Semantic["Resolve enum and case semantically"]
Semantic --> EIR["LiteralDefaultValue::EnumCase"]
EIR --> Validate["Validate enum metadata"]
Validate --> Load["Lazily materialize canonical case"]
Load --> Retain["Retain for property-slot ownership"]
Retain --> Store["Store singleton in instance/static slot"]
Reviews (4): Last reviewed commit: "fix(enums): retain the singleton an enum..." | Re-trigger Greptile |
Closes #566. enum Level { case Low; } class Config { public Level $level = Level::Low; // PHP accepts this; elephc did not } Two layers refused it, and the issue predicted the second. ## The checker `validate_schema_declared_default_type` ran while class schemas were being built, where enum cases do not exist yet -- so `infer_expr_type_syntactic` answered `Str` for `Level::Low` and the declared `Object("Level")` slot rejected it with "expects Object(\"Level\"), got Str". PARAMETERS already had the answer. `validate_schema_parameter_default_type` deferred a `ScopedConstantAccess` default to `schema::defaults`, which revalidates it once the schemas are complete and can resolve the constant semantically (PR #565). Constructor promotion worked for exactly that reason: its default is a parameter default. The deferral now lives in the declared-default validator, so properties and static properties get it too, and `validate_class_property_defaults` routes through the same `validate_deferred_default` the signature pass uses. Nothing is waved through -- it changes WHEN the default is judged, not what counts as compatible. Both negatives still report, from the pass that can tell the difference: public Level $level = Level::Missing; Undefined enum case: Level::Missing public Level $level = Holder::NAME; expects Object("Level"), got Str ## The backend Past the checker, the property-initialization path had no form for it either: unsupported EIR backend feature: object_new for default value of property $level with PHP type Object("Level") `LiteralDefaultValue::EnumCase` is that form. It is recognized on SHAPE in `literal_default_value`, which has no module, and settled where the module is available: `emit_property_default` and `emit_static_property_default_value` both verify the receiver really names an enum and the constant one of its cases, and report unsupported otherwise rather than emitting a symbol reference that would fail at assembly time. The value is loaded through `emit_lazy_case_load_unguarded`, not read out of the case slot. Cases are materialized lazily, so a default written before the case's first use anywhere else would otherwise store the still-null slot and every `$obj->prop === Level::Low` after it would be false. ## Measured Static, declared and promoted forms in one program, byte-identical to the host PHP 8.5.10 -- including `===` against the case (the assertion that separates the singleton from a fresh object), a backed enum's `->value`, cross-form identity, and reassignment afterwards. `test_error_plain_property_enum_case_default_remains_unsupported` is replaced by `test_plain_property_enum_case_default_is_accepted` plus two negatives, as the issue's acceptance criteria ask. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
CI caught what the local run did not: `examples/enums/main.php` gained the
enum-case property section, and `test_example_enums_compiles_and_runs` asserts
that file's WHOLE stdout, so extending the example without extending the
expectation fails the pin. It failed identically on all three architectures,
which is the right shape for a stale expectation rather than a codegen problem:
left: "...\nDESC\nLow High High same"
right: "...\nDESC"
The expectation now carries the new row, and the doc comment says what it is
worth: the `=== Level::Low` at the end is the assertion that separates the
singleton from a fresh object, because `->name` prints the same either way.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Review follow-up, and a real one: the default paths stored the enum global's BORROWED singleton without an incref, so the slot became a second owner of a reference it never took. That is not a leak but its opposite. The first thing that releases the slot -- a property reassignment, an object's cleanup, a web worker's static teardown -- consumes the global's only reference and frees a case that is still reachable by name. Lazy materialization then hands the freed block to the NEXT case, so two cases end up sharing one object. It is the same under-retention #349 fixed for an ordinary `Enum::Case` read, reached through the default paths instead. Measured, 500 iterations of `$c = new Config(); $c->level = Level::High;`: Level::Low->name string(0) "" <- freed and reused Level::Low === Level::Low bool(true) <- identity survives a dangling pointer, which is why `->name` is the assertion that catches it Both store sites now retain, exactly as `lower_inst::scoped_constants` does: the instance path in `emit_property_default` and the static one in `emit_static_property_default_value`. `object_reg` is preserved across `__rt_incref` the way the boxed-literal arms already preserve it; the materializer needs no such care, because it promises to preserve every caller-saved integer register. After the fix the same program matches the host PHP 8.5.10 byte for byte, and `--gc-stats` reports `allocs=504 frees=502` -- the two retained blocks are the two singletons, which are process-lifetime by design, so nothing accumulates per iteration. The module preamble of `schema/defaults.rs` still said plain property scoped-constant defaults stay outside that pass, which this PR ended; it now says what the pass covers and repeats that it changes WHEN a default is judged, not what counts as compatible. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
7ee1f90 to
67079c9
Compare
|
Follow-ups from the Grok review leftovers (not merge blockers for this PR):
|
|
i've opened 5 follow-ups. the rest is good for me |
Closes #566.
Two layers refused it, and the issue predicted the second.
The checker
validate_schema_declared_default_typeran while class schemas were being built, where enum cases do not exist yet — soinfer_expr_type_syntacticansweredStrforLevel::Lowand the declaredObject("Level")slot rejected it with "expects Object("Level"), got Str".Parameters already had the answer.
validate_schema_parameter_default_typedeferred aScopedConstantAccessdefault toschema::defaults, which revalidates it once the schemas are complete and can resolve the constant semantically (PR #565). Constructor promotion worked for exactly that reason — its default is a parameter default. The deferral now lives in the declared-default validator, so properties and static properties get it too, andvalidate_class_property_defaultsroutes through the samevalidate_deferred_defaultthe signature pass uses.Nothing is waved through: it changes when the default is judged, not what counts as compatible. Both negatives still report, from the pass that can tell the difference:
public Level $level = Level::Missing;Undefined enum case: Level::Missingpublic Level $level = Holder::NAME;expects Object("Level"), got StrThe backend
Past the checker, the property-initialization path had no form for it either — the gap the issue names:
LiteralDefaultValue::EnumCaseis that form. It is recognized on shape inliteral_default_value, which has no module, and settled where the module is available:emit_property_defaultandemit_static_property_default_valueboth verify the receiver really names an enum and the constant one of its cases, reporting unsupported otherwise rather than emitting a symbol reference that would fail at assembly time.The value is loaded through
emit_lazy_case_load_unguarded, not read out of the case slot. Cases are materialized lazily, so a default written before the case's first use anywhere else would otherwise store the still-null slot — and every$obj->prop === Level::Lowafter it would be false, which is the failure mode that looks like it works until it doesn't.Measured
Static, declared and promoted forms in one program, byte-identical to the host PHP 8.5.10:
===is the assertion that matters: a default that allocated a fresh object, or stored the unmaterialized slot, would still print the right->name.Acceptance criteria
test_error_plain_property_enum_case_default_remains_unsupportedreplaced bytest_plain_property_enum_case_default_is_acceptedemit_lazy_case_load_unguardedhelper both architectures already useDocs in
docs/php/classes.md(a new "Enum cases as defaults" section covering all four positions), and theexamples/enumsexample extended with the three property forms.🤖 Generated with Claude Code
https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr