Skip to content

fix: enforce readonly declaration and inheritance rules - #66

Open
AlessioGiacobbe wants to merge 3 commits into
swoole:masterfrom
AlessioGiacobbe:split/readonly-declaration-rules
Open

fix: enforce readonly declaration and inheritance rules#66
AlessioGiacobbe wants to merge 3 commits into
swoole:masterfrom
AlessioGiacobbe:split/readonly-declaration-rules

Conversation

@AlessioGiacobbe

Copy link
Copy Markdown
Contributor

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 via readonly 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.

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focused readonly tests pass (11/11), but three readonly class contracts still compile:

  1. 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.

  1. 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.

  1. #[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.

@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/readonly-declaration-rules branch from f9f6e78 to 85fd24e Compare September 2, 2026 08:21
@AlessioGiacobbe

Copy link
Copy Markdown
Contributor Author

All three covered, rebased on current master:

  1. Trait properties — rejected during composition with Zend's message ("Readonly class C cannot use trait with a non-readonly property T::$value"). Placement detail: Zend names the directly used trait for nested traits (probed with an Inner→Outer chain — it reports Outer::$v), so the check runs in the composition pass where that name is in hand rather than off the origin attribute. Static trait properties are rejected too (they can never be readonly — probed); an explicitly-readonly trait property still composes (positive test).
  2. Internal parents — the inheritance check no longer skips them: an internal-parent branch queries host ReflectionClass::isReadOnly(), two-directional in principle (today's only internal readonly classes, BcMath\Number and Dom\NamespaceInfo, are both final, so only the readonly-child direction is constructible — reflection keeps the reverse correct regardless). readonly class C extends ArrayObject {} now fails with Zend's message.
  3. #[AllowDynamicProperties] — rejected on readonly classes with Zend's wording, and the invalid combination sitting in the pre-existing readonly-class.phpt is fixed (body re-validated under Zend, output unchanged); the negative test lives in the phpunit suite per the repo's convention for front-end rejections.

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.
@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/readonly-declaration-rules branch from 85fd24e to 7fba828 Compare September 2, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants