From fafd7f25dc072dbec485d3ba42ca3cf3cced0e10 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Wed, 2 Sep 2026 13:43:22 +0200 Subject: [PATCH] fix: enforce readonly declaration and inheritance rules (#66) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(preprocessor): enforce Zend readonly property declaration rules 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. * fix(translator): enforce readonly-class inheritance in both directions Zend seals readonly-ness across a hierarchy: a non-readonly class cannot extend a readonly one and vice versa. Both directions compiled silently. * fix(translator): complete readonly-class contracts for traits, internal 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. --- .../code/inheritance_error_prop_readonly.php | 2 +- phpunit/code/readonly_class_allow_dynamic.php | 5 ++ phpunit/code/readonly_class_extends.php | 5 ++ .../code/readonly_class_extends_internal.php | 4 + phpunit/code/readonly_class_extends_rev.php | 5 ++ phpunit/code/readonly_class_extends_valid.php | 5 ++ .../readonly_class_trait_nonreadonly_prop.php | 5 ++ ...adonly_class_trait_readonly_prop_valid.php | 5 ++ .../code/readonly_class_trait_static_prop.php | 5 ++ phpunit/code/readonly_rule_class_static.php | 4 + phpunit/code/readonly_rule_class_untyped.php | 4 + phpunit/code/readonly_rule_default.php | 4 + .../code/readonly_rule_promoted_untyped.php | 4 + phpunit/code/readonly_rule_static.php | 4 + phpunit/code/readonly_rule_untyped.php | 4 + phpunit/code/readonly_rule_valid.php | 4 + phpunit/src/ClassKindInheritanceTest.php | 40 ++++++++++ phpunit/src/ReadonlyDeclarationRulesTest.php | 80 +++++++++++++++++++ src/Preprocessor.php | 30 +++++++ src/Translator.php | 53 ++++++++++++ tests/compiler/class/readonly-class.phpt | 1 - 21 files changed, 271 insertions(+), 2 deletions(-) create mode 100644 phpunit/code/readonly_class_allow_dynamic.php create mode 100644 phpunit/code/readonly_class_extends.php create mode 100644 phpunit/code/readonly_class_extends_internal.php create mode 100644 phpunit/code/readonly_class_extends_rev.php create mode 100644 phpunit/code/readonly_class_extends_valid.php create mode 100644 phpunit/code/readonly_class_trait_nonreadonly_prop.php create mode 100644 phpunit/code/readonly_class_trait_readonly_prop_valid.php create mode 100644 phpunit/code/readonly_class_trait_static_prop.php create mode 100644 phpunit/code/readonly_rule_class_static.php create mode 100644 phpunit/code/readonly_rule_class_untyped.php create mode 100644 phpunit/code/readonly_rule_default.php create mode 100644 phpunit/code/readonly_rule_promoted_untyped.php create mode 100644 phpunit/code/readonly_rule_static.php create mode 100644 phpunit/code/readonly_rule_untyped.php create mode 100644 phpunit/code/readonly_rule_valid.php create mode 100644 phpunit/src/ClassKindInheritanceTest.php create mode 100644 phpunit/src/ReadonlyDeclarationRulesTest.php diff --git a/phpunit/code/inheritance_error_prop_readonly.php b/phpunit/code/inheritance_error_prop_readonly.php index f9bca1e0..0e54ffd3 100644 --- a/phpunit/code/inheritance_error_prop_readonly.php +++ b/phpunit/code/inheritance_error_prop_readonly.php @@ -6,7 +6,7 @@ class A class B extends A { - public readonly int $x = 2; + public readonly int $x; } function main() {} diff --git a/phpunit/code/readonly_class_allow_dynamic.php b/phpunit/code/readonly_class_allow_dynamic.php new file mode 100644 index 00000000..51e2a2bd --- /dev/null +++ b/phpunit/code/readonly_class_allow_dynamic.php @@ -0,0 +1,5 @@ +port = 80; } } + +function main() {} diff --git a/phpunit/code/readonly_class_extends.php b/phpunit/code/readonly_class_extends.php new file mode 100644 index 00000000..bb9e21bf --- /dev/null +++ b/phpunit/code/readonly_class_extends.php @@ -0,0 +1,5 @@ +port = 80; } } + +function main() {} diff --git a/phpunit/code/readonly_class_trait_static_prop.php b/phpunit/code/readonly_class_trait_static_prop.php new file mode 100644 index 00000000..f59432bf --- /dev/null +++ b/phpunit/code/readonly_class_trait_static_prop.php @@ -0,0 +1,5 @@ +port = 80; } } + +function main() {} diff --git a/phpunit/src/ClassKindInheritanceTest.php b/phpunit/src/ClassKindInheritanceTest.php new file mode 100644 index 00000000..df52ca1c --- /dev/null +++ b/phpunit/src/ClassKindInheritanceTest.php @@ -0,0 +1,40 @@ +exec( + 'Non-readonly class `B` cannot extend readonly class `A`', + 'readonly_class_extends.php' + ); + } + + public function testReadonlyCannotExtendNonReadonly(): void + { + $this->exec( + 'Readonly class `B` cannot extend non-readonly class `A`', + 'readonly_class_extends_rev.php' + ); + } + + + public function testReadonlyExtendsReadonlyIsValid(): void + { + $this->compile('readonly_class_extends_valid.php'); + } + + public function testReadonlyCannotExtendNonReadonlyInternalClass(): void + { + // Internal parents are not in the symbol table; host reflection + // (ReflectionClass::isReadOnly) is authoritative for them. + $this->exec( + 'Readonly class `Cfg` cannot extend non-readonly class `ArrayObject`', + 'readonly_class_extends_internal.php' + ); + } +} diff --git a/phpunit/src/ReadonlyDeclarationRulesTest.php b/phpunit/src/ReadonlyDeclarationRulesTest.php new file mode 100644 index 00000000..a65b3a20 --- /dev/null +++ b/phpunit/src/ReadonlyDeclarationRulesTest.php @@ -0,0 +1,80 @@ +exec('Readonly property `Cfg::$port` cannot have default value', 'readonly_rule_default.php'); + } + + public function testReadonlyPropertyMustHaveType(): void + { + $this->exec('Readonly property `Cfg::$port` must have type', 'readonly_rule_untyped.php'); + } + + public function testStaticPropertyCannotBeReadonly(): void + { + $this->exec('Static property `Cfg::$port` cannot be readonly', 'readonly_rule_static.php'); + } + + public function testPromotedReadonlyParamMustHaveType(): void + { + $this->exec('Readonly property `Cfg::$port` must have type', 'readonly_rule_promoted_untyped.php'); + } + + public function testReadonlyClassPropertyMustHaveType(): void + { + $this->exec('Readonly property `Cfg::$port` must have type', 'readonly_rule_class_untyped.php'); + } + + public function testReadonlyClassCannotDeclareStaticProperty(): void + { + $this->exec('Static property `Cfg::$port` cannot be readonly', 'readonly_rule_class_static.php'); + } + + public function testWellFormedReadonlyDeclarationsStillCompile(): void + { + // Promoted readonly params may keep a parameter default: it belongs + // to the constructor argument, not to the property. + $this->compile('readonly_rule_valid.php'); + } + + public function testReadonlyClassCannotUseTraitWithNonReadonlyProperty(): void + { + // A trait property keeps its own declaration; the consuming class's + // readonly modifier does not upgrade it. + $this->exec( + 'Readonly class `Cfg` cannot use trait with a non-readonly property `Settings::$port`', + 'readonly_class_trait_nonreadonly_prop.php' + ); + } + + public function testReadonlyClassCannotUseTraitWithStaticProperty(): void + { + // Static properties can never be readonly, so a trait declaring one + // is unusable in a readonly class (Zend reports the same mismatch). + $this->exec( + 'Readonly class `Cfg` cannot use trait with a non-readonly property `Settings::$port`', + 'readonly_class_trait_static_prop.php' + ); + } + + public function testReadonlyClassUsingTraitWithReadonlyPropertyIsValid(): void + { + $this->compile('readonly_class_trait_readonly_prop_valid.php'); + } + + public function testAllowDynamicPropertiesOnReadonlyClassIsRejected(): void + { + // Dynamic properties and readonly semantics are mutually exclusive. + $this->exec( + 'Cannot apply #[AllowDynamicProperties] to readonly class `Cfg`', + 'readonly_class_allow_dynamic.php' + ); + } +} diff --git a/src/Preprocessor.php b/src/Preprocessor.php index fc3dc691..5bd3ecdc 100644 --- a/src/Preprocessor.php +++ b/src/Preprocessor.php @@ -1209,6 +1209,21 @@ class Preprocessor extends CompilerBase if (isset($this->symbolDeclInFile[$fullClassNameLower])) { $this->fatalError($class, "Duplicate class `{$fullClassName}`"); } + // Dynamic properties and readonly semantics are mutually exclusive: + // every property of a readonly class is readonly and declared, so + // Zend rejects the attribute at compile time. + if ($class instanceof Node\Stmt\Class_ && ($flags & Modifiers::READONLY)) { + foreach ($class->attrGroups as $group) { + foreach ($group->attrs as $attribute) { + if (strcasecmp($this->getResolvedPhpName($attribute->name), 'AllowDynamicProperties') === 0) { + $this->fatalError( + $attribute, + "Cannot apply #[AllowDynamicProperties] to readonly class `{$fullClassName}`", + ); + } + } + } + } $this->classDef = new ClassDef($this->class, $flags, $this->namespace); $this->classDef->nativeObject = NativeClassAttributeLowering::isNative($class); @@ -1674,6 +1689,21 @@ class Preprocessor extends CompilerBase ); } $flags = $this->parseModifiers($flags); + // A `readonly class` marks every property readonly, so the class-level + // flag participates in the same Zend declaration rules as an explicit + // per-property `readonly` modifier. + if (($flags | $this->classDef->flags) & Modifiers::READONLY) { + $className = $this->classDef->getNamespacedName(false); + if ($flags & Modifiers::STATIC) { + $this->fatalError($errorNode, "Static property `{$className}::\${$name}` cannot be readonly"); + } + if ($typeNode === null) { + $this->fatalError($errorNode, "Readonly property `{$className}::\${$name}` must have type"); + } + if ($defaultNode !== null) { + $this->fatalError($errorNode, "Readonly property `{$className}::\${$name}` cannot have default value"); + } + } $this->validateAsymmetricPropertyDeclaration($name, $flags, $typeNode, $errorNode); [$type, $class] = $this->resolveTypeDecl($typeNode, self::DECL_TYPE_OF_PROPERTY); $this->assertSupportedNativeObjectTypeNode($typeNode, self::DECL_TYPE_OF_PROPERTY, $errorNode); diff --git a/src/Translator.php b/src/Translator.php index b2997287..dedd87fb 100644 --- a/src/Translator.php +++ b/src/Translator.php @@ -3368,6 +3368,21 @@ CODE; } continue; } + // A trait property keeps its own declaration: the + // consuming class's `readonly` modifier does not + // upgrade it, so Zend refuses to compose a + // non-readonly (or static, which can never be + // readonly) trait property into a readonly class. + // Zend names the directly used trait, even when + // the property originated in a nested trait. + if (($classDef->flags & Modifiers::READONLY) + && !($traitStmt->flags & Modifiers::READONLY) + ) { + $this->fatalError( + $traitStmt, + "Readonly class `{$compositionOwner}` cannot use trait with a non-readonly property `{$traitFullName}::\${$prop->name->toString()}`", + ); + } $traitProperties[$propName] = [$traitStmt, $prop]; } } @@ -4084,9 +4099,30 @@ CODE; if ($parent->flags & Modifiers::FINAL) { $this->fatalError($class, "Class `{$this->class}` cannot extend final class `{$parentClass}`"); } + // Readonly-ness is part of the inheritance contract in both + // directions (Zend: a readonly class seals its property + // semantics for the whole hierarchy). + $this->assertReadonlyInheritanceContract( + $class, + $parentClass, + (bool) ($parent->flags & Modifiers::READONLY), + ); } else { $this->fatalError($class, "Class `{$this->class}` inherits from a non-existent class `{$parentClass}`"); } + } elseif ($this->classDef->extends and $this->classDef->inheritedFromInternalClass and $class instanceof Node\Stmt\Class_) { + // Internal parents are not in the symbol table; the host runtime's + // reflection is authoritative for their readonly-ness (e.g. + // BcMath\Number is an internal readonly class, ArrayObject is not), + // so the contract holds in both directions here as well. + $parentClass = $this->getNamespacedClassName($this->parseIdentifier($class->extends)); + if (class_exists($parentClass)) { + $this->assertReadonlyInheritanceContract( + $class, + $parentClass, + (new \ReflectionClass($parentClass))->isReadOnly(), + ); + } } if (is_array($this->classDef->implements)) { @@ -6190,6 +6226,23 @@ CODE; } } + /** + * Zend seals readonly-ness across a class hierarchy in both directions: a + * readonly class cannot extend a non-readonly one and vice versa. The + * parent's readonly-ness comes from the symbol table for compiled classes + * and from host reflection for internal ones. + */ + private function assertReadonlyInheritanceContract(NodeAbstract $errorNode, string $parentClass, bool $parentReadonly): void + { + $childReadonly = (bool) ($this->classDef->flags & Modifiers::READONLY); + if ($childReadonly === $parentReadonly) { + return; + } + $this->fatalError($errorNode, $parentReadonly + ? "Non-readonly class `{$this->class}` cannot extend readonly class `{$parentClass}`" + : "Readonly class `{$this->class}` cannot extend non-readonly class `{$parentClass}`"); + } + private function installComposedTraitDataMembers(Node\Stmt\ClassLike $class): void { foreach ($class->stmts as $stmt) { diff --git a/tests/compiler/class/readonly-class.phpt b/tests/compiler/class/readonly-class.phpt index acafc911..004bd56a 100644 --- a/tests/compiler/class/readonly-class.phpt +++ b/tests/compiler/class/readonly-class.phpt @@ -4,7 +4,6 @@ Readonly Classes (PHP 8.2+)