fix: enforce readonly declaration and inheritance rules - #66
fix: enforce readonly declaration and inheritance rules#66AlessioGiacobbe wants to merge 3 commits into
Conversation
matyhtf
left a comment
There was a problem hiding this comment.
The focused readonly tests pass (11/11), but three readonly class contracts still compile:
- Trait property origin must retain readonlyness:
trait T { public int $value; }
readonly class C { use T; }Zend: Readonly class C cannot use trait with a non-readonly property T::$value. The current composition treats the consuming class flag as if it converted the trait property to readonly. It must reject the mismatch; add positive coverage for a trait property explicitly declared readonly.
- Internal parents are skipped by the inheritance check:
readonly class C extends ArrayObject {}Zend: Readonly class C cannot extend non-readonly class ArrayObject. Query the internal parent through ReflectionClass::isReadOnly() (or equivalent metadata) so the rule applies to both TypePHP and internal parents.
#[AllowDynamicProperties]is forbidden on readonly classes:
#[AllowDynamicProperties]
readonly class C { public function __construct(public int $value) {} }Zend rejects it, while TypePHP compiles it. tests/compiler/class/readonly-class.phpt currently contains this invalid combination and should be corrected when adding the negative test.
Please cover and reject all three before merge.
f9f6e78 to
85fd24e
Compare
|
All three covered, rebased on current master:
5 new fixtures, 21/21 with the related suites; sweep and full suite identical to base. |
The readonly checks previously lived only in the Native-class branch;
ZendVM-backed classes accepted declarations Zend rejects at compile
time. addClassProperty now enforces, for declared and promoted
properties alike (probed against Zend 8.4.13):
- readonly property with a default value ("Readonly property A::$x
cannot have default value") - a readonly property carries runtime
initialization state, so a compile-time default is meaningless
- untyped readonly property, including untyped promoted readonly ctor
params ("Readonly property A::$x must have type")
- static readonly ("Static property A::$x cannot be readonly")
- a `readonly class` applies the same three rules to every property:
the class-level Modifiers::READONLY flag (already recorded on
ClassDef->flags for the Translator-side inheritance check) is OR-ed
into the per-property check
Promoted readonly params keep accepting parameter defaults: the default
belongs to the constructor argument, not the property (Zend-verified).
The inheritance_error_prop_readonly fixture used `readonly int $x = 2`,
which Zend itself rejects with the default-value error before ever
reaching the readonly-mismatch link error; the default is dropped so the
fixture still exercises the inheritance mismatch.
Zend seals readonly-ness across a hierarchy: a non-readonly class cannot extend a readonly one and vice versa. Both directions compiled silently.
…al parents and attributes
Three readonly-class rules Zend enforces at compile time were still
accepted (all probed on 8.4.13):
- A trait property keeps its own declaration; the consuming class's
readonly modifier does not upgrade it, so composing a non-readonly
(or static, which can never be readonly) trait property into a
readonly class fails: "Readonly class C cannot use trait with a
non-readonly property T::$value". The check runs in composeTraitAst's
property pass, which also matches Zend's naming of the directly used
trait when the property originates in a nested trait. A trait property
declared readonly composes fine.
- The readonly inheritance check only covered compiled parents; classes
extending internal ones skipped it entirely, so `readonly class C
extends ArrayObject {}` compiled. Internal parents now consult host
reflection (ReflectionClass::isReadOnly), keeping the contract
two-directional: the host runtime also knows internal readonly classes
(BcMath\Number, Dom\NamespaceInfo — both final in 8.4, so only the
readonly-child direction is reachable today).
- #[AllowDynamicProperties] contradicts readonly semantics (every
property is readonly and declared); Zend rejects the combination:
"Cannot apply #[AllowDynamicProperties] to readonly class C". The
pre-existing readonly-class.phpt carried exactly this invalid
combination and is adjusted to stay a valid positive test.
85fd24e to
7fba828
Compare
Readonly declaration rules were unchecked for ZendVM-backed classes (checks existed only in the Native-class branch, which rejects readonly wholesale):
public readonly int $x = 5;(Zend: "cannot have default value"), untyped readonly ("must have type"), and static readonly all compiled, for declared and promoted properties and viareadonly class— each rule probed for exact Zend behavior; promoted readonly params keep their legal parameter defaults.Readonly-class inheritance was also not sealed: a non-readonly class extending a readonly one (and the reverse) compiled; both directions now fail with Zend's messages.
One pre-existing fixture (inheritance_error_prop_readonly.php) used
readonly int $x = 2, which Zend itself rejects before the inheritance error under test; the default was dropped so the fixture still exercises the mismatch.Part of the split of #39.