From c578b1d6b00a564a5a821108b0ca05866a11e8c4 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Wed, 2 Sep 2026 13:43:03 +0200 Subject: [PATCH] fix(preprocessor): enforce property-hook placement rules for class properties (#63) --skip-tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(preprocessor): enforce property-hook placement rules for class properties The interface path already validated hook placement; class and trait properties accepted every combination. parseClassPropertyDef now mirrors Zend's compile-time rules (probed on 8.4.13, including the precedence order static -> readonly -> abstract rules): - hooks on a static property ("Cannot declare hooks for static property") - hooks on a readonly property, including properties made readonly by a `readonly class` ("Hooked properties cannot be readonly") - `abstract` on a hook-less property ("Only hooked properties may be declared abstract") - abstract hooked property with a default value ("Cannot specify default value for virtual hooked property A::$x") - abstract hooked property whose hooks all have bodies ("Abstract property A::$x must specify at least one abstract hook") - abstract hooked property in a non-abstract class; traits stay exempt (the consuming class satisfies the hook) and enums are already rejected by the property ban - bodiless hook on a non-abstract property, in classes and traits ("Non-abstract property hook must have a body"); previously the lowering fabricated a concrete backing-store accessor for it * test(preprocessor): cover property-hook placement rules * fix(preprocessor): reject abstract-private, abstract-final and final-private hooks Three hook-level modifier conflicts Zend rejects at compile time were still accepted by the class/trait property path (all probed on 8.4.13): - `abstract private int $x { get; }` — an abstract (bodiless) hook must be implemented by a subclass, which private visibility forbids ("Property hook cannot be both abstract and private"). Unlike abstract private trait methods, Zend does NOT exempt traits from this rule. - `abstract public int $x { final get; }` — a bodiless hook must stay overridable to ever gain a body ("Property hook cannot be both abstract and final"). - `private int $x { final get => 1; }` — a final hook on a private property is meaningless because private members cannot be overridden ("Property hook cannot be both final and private"). Diagnostic precedence follows Zend: static, then readonly, then the per-hook final+private conflict (which wins over both abstract conflicts: `abstract private int $x { final get; }` reports final+private), then per bodiless hook abstract+private before abstract+final, all ahead of the default-value and at-least-one-abstract-hook rules. Protected abstract hooks remain legal in classes and traits. --- .../code/hook_rule_abstract_all_bodies.php | 4 + phpunit/code/hook_rule_abstract_final.php | 4 + phpunit/code/hook_rule_abstract_no_hooks.php | 4 + .../hook_rule_abstract_nonabstract_class.php | 4 + phpunit/code/hook_rule_abstract_private.php | 4 + .../code/hook_rule_abstract_private_trait.php | 4 + .../hook_rule_abstract_protected_valid.php | 6 ++ phpunit/code/hook_rule_bodyless.php | 4 + phpunit/code/hook_rule_final_private.php | 4 + phpunit/code/hook_rule_readonly.php | 4 + phpunit/code/hook_rule_readonly_class.php | 4 + phpunit/code/hook_rule_static.php | 4 + phpunit/code/hook_rule_valid.php | 4 + phpunit/src/PropertyHookPlacementTest.php | 83 ++++++++++++++++ src/Preprocessor.php | 98 +++++++++++++++++++ 15 files changed, 235 insertions(+) create mode 100644 phpunit/code/hook_rule_abstract_all_bodies.php create mode 100644 phpunit/code/hook_rule_abstract_final.php create mode 100644 phpunit/code/hook_rule_abstract_no_hooks.php create mode 100644 phpunit/code/hook_rule_abstract_nonabstract_class.php create mode 100644 phpunit/code/hook_rule_abstract_private.php create mode 100644 phpunit/code/hook_rule_abstract_private_trait.php create mode 100644 phpunit/code/hook_rule_abstract_protected_valid.php create mode 100644 phpunit/code/hook_rule_bodyless.php create mode 100644 phpunit/code/hook_rule_final_private.php create mode 100644 phpunit/code/hook_rule_readonly.php create mode 100644 phpunit/code/hook_rule_readonly_class.php create mode 100644 phpunit/code/hook_rule_static.php create mode 100644 phpunit/code/hook_rule_valid.php create mode 100644 phpunit/src/PropertyHookPlacementTest.php diff --git a/phpunit/code/hook_rule_abstract_all_bodies.php b/phpunit/code/hook_rule_abstract_all_bodies.php new file mode 100644 index 00000000..3d3d5289 --- /dev/null +++ b/phpunit/code/hook_rule_abstract_all_bodies.php @@ -0,0 +1,4 @@ + 1; } } + +function main() {} diff --git a/phpunit/code/hook_rule_abstract_final.php b/phpunit/code/hook_rule_abstract_final.php new file mode 100644 index 00000000..773cbb04 --- /dev/null +++ b/phpunit/code/hook_rule_abstract_final.php @@ -0,0 +1,4 @@ + 1; } } +trait Boxed { abstract protected int $y { get; } } + +function main() {} diff --git a/phpunit/code/hook_rule_bodyless.php b/phpunit/code/hook_rule_bodyless.php new file mode 100644 index 00000000..566bb420 --- /dev/null +++ b/phpunit/code/hook_rule_bodyless.php @@ -0,0 +1,4 @@ + 1; } } + +function main() {} diff --git a/phpunit/code/hook_rule_readonly_class.php b/phpunit/code/hook_rule_readonly_class.php new file mode 100644 index 00000000..e6b4184d --- /dev/null +++ b/phpunit/code/hook_rule_readonly_class.php @@ -0,0 +1,4 @@ + 1; } } + +function main() {} diff --git a/phpunit/code/hook_rule_static.php b/phpunit/code/hook_rule_static.php new file mode 100644 index 00000000..5b48c210 --- /dev/null +++ b/phpunit/code/hook_rule_static.php @@ -0,0 +1,4 @@ + 1; } } + +function main() {} diff --git a/phpunit/code/hook_rule_valid.php b/phpunit/code/hook_rule_valid.php new file mode 100644 index 00000000..02e0cf52 --- /dev/null +++ b/phpunit/code/hook_rule_valid.php @@ -0,0 +1,4 @@ + $this->b; set { $this->b = $value; } } abstract public string $s { get; } } + +function main() {} diff --git a/phpunit/src/PropertyHookPlacementTest.php b/phpunit/src/PropertyHookPlacementTest.php new file mode 100644 index 00000000..d5c7944b --- /dev/null +++ b/phpunit/src/PropertyHookPlacementTest.php @@ -0,0 +1,83 @@ +exec('Cannot declare hooks for static property', 'hook_rule_static.php'); + } + + public function testHooksOnReadonlyPropertyAreRejected(): void + { + $this->exec('Hooked properties cannot be readonly', 'hook_rule_readonly.php'); + } + + public function testHooksInReadonlyClassAreRejected(): void + { + $this->exec('Hooked properties cannot be readonly', 'hook_rule_readonly_class.php'); + } + + public function testAbstractHookedPropertyRequiresAbstractClass(): void + { + $this->exec('Non-abstract class `Box` contains abstract hooked property `$x`', 'hook_rule_abstract_nonabstract_class.php'); + } + + public function testAbstractPropertyNeedsAtLeastOneAbstractHook(): void + { + $this->exec('Abstract property `Box::$x` must specify at least one abstract hook', 'hook_rule_abstract_all_bodies.php'); + } + + public function testOnlyHookedPropertiesMayBeAbstract(): void + { + $this->exec('Only hooked properties may be declared abstract', 'hook_rule_abstract_no_hooks.php'); + } + + public function testNonAbstractHookMustHaveBody(): void + { + $this->exec('Non-abstract property hook must have a body', 'hook_rule_bodyless.php'); + } + + public function testWellFormedHooksStillCompile(): void + { + $this->compile('hook_rule_valid.php'); + } + + public function testAbstractPrivateHookIsRejected(): void + { + // An abstract (bodiless) hook must be implementable by a subclass, + // which a private property forbids. + $this->exec('Property hook cannot be both abstract and private', 'hook_rule_abstract_private.php'); + } + + public function testAbstractPrivateHookIsRejectedInTrait(): void + { + // Unlike abstract private trait methods, Zend does not exempt traits + // from the abstract-private hook conflict. + $this->exec('Property hook cannot be both abstract and private', 'hook_rule_abstract_private_trait.php'); + } + + public function testAbstractFinalHookIsRejected(): void + { + // A bodiless hook must be overridable to ever gain a body; it cannot + // carry final. + $this->exec('Property hook cannot be both abstract and final', 'hook_rule_abstract_final.php'); + } + + public function testFinalPrivateHookWinsDiagnosticPrecedence(): void + { + // `abstract private int $x { final get; }` violates all three rules; + // Zend reports the final+private conflict first (probed on 8.4.13). + $this->exec('Property hook cannot be both final and private', 'hook_rule_final_private.php'); + } + + public function testProtectedAbstractHookStaysLegal(): void + { + $this->compile('hook_rule_abstract_protected_valid.php'); + } +} diff --git a/src/Preprocessor.php b/src/Preprocessor.php index dceaadc3..fc3dc691 100644 --- a/src/Preprocessor.php +++ b/src/Preprocessor.php @@ -2001,6 +2001,7 @@ class Preprocessor extends CompilerBase protected function parseClassPropertyDef(Node\Stmt\Property $v): void { + $this->validateClassPropertyHookPlacement($v); $arrayDef = $this->parseArrayDefinition($v); if ($this->classDef->nativeObject) { if ($v->type === null) { @@ -2051,6 +2052,103 @@ class Preprocessor extends CompilerBase $this->context = $oriCtx; } + /** + * Mirror Zend's compile-time placement rules for property hooks on class + * (and trait) properties; the interface path enforces its own subset in + * prepareInterfaceProperty(). Check order follows Zend 8.4 precedence: + * static, readonly, then the abstract-property rules. + */ + private function validateClassPropertyHookPlacement(Node\Stmt\Property $v): void + { + $abstract = (bool) ($v->flags & Modifiers::ABSTRACT); + if ($v->hooks === [] && !$abstract) { + return; + } + + $className = $this->classDef->getNamespacedName(false); + $propName = $v->props !== [] ? $this->parseIdentifier($v->props[0]->name) : ''; + if ($v->hooks !== []) { + if ($v->flags & Modifiers::STATIC) { + $this->fatalError($v, 'Cannot declare hooks for static property'); + } + // A readonly class marks every property readonly, exactly like an + // explicit per-property modifier. + if (($v->flags | $this->classDef->flags) & Modifiers::READONLY) { + $this->fatalError($v, 'Hooked properties cannot be readonly'); + } + // Zend checks hook-level modifier conflicts right after the + // property-level placement rules, before any abstract-property + // rule: a final hook on a private property is rejected first even + // when the hook is also bodiless (probed: + // `abstract private int $x { final get; }` reports final+private). + // A private property cannot be overridden, so a final hook on it + // is meaningless; the rule applies in traits as well. + foreach ($v->hooks as $hook) { + if (($hook->flags & Modifiers::FINAL) && ($v->flags & Modifiers::PRIVATE)) { + $this->fatalError($hook, 'Property hook cannot be both final and private'); + } + } + } + + if ($abstract) { + if ($v->hooks === []) { + $this->fatalError($v, 'Only hooked properties may be declared abstract'); + } + // A bodiless hook of an abstract property is itself abstract. An + // abstract hook must be implementable by a subclass, which a + // private property forbids, and must be overridable, which final + // forbids. Zend reports these per hook, before the default-value + // and abstract-hook-presence rules (probed on 8.4.13), and — + // unlike abstract private trait METHODS — does not exempt traits. + foreach ($v->hooks as $hook) { + if ($hook->body !== null) { + continue; + } + if ($v->flags & Modifiers::PRIVATE) { + $this->fatalError($hook, 'Property hook cannot be both abstract and private'); + } + if ($hook->flags & Modifiers::FINAL) { + $this->fatalError($hook, 'Property hook cannot be both abstract and final'); + } + } + foreach ($v->props as $prop) { + if ($prop->default !== null) { + $this->fatalError( + $v, + "Cannot specify default value for virtual hooked property {$className}::\${$propName}", + ); + } + } + $hasAbstractHook = false; + foreach ($v->hooks as $hook) { + if ($hook->body === null) { + $hasAbstractHook = true; + break; + } + } + if (!$hasAbstractHook) { + $this->fatalError( + $v, + "Abstract property `{$className}::\${$propName}` must specify at least one abstract hook", + ); + } + if (!$this->classDef->trait && !($this->classDef->flags & Modifiers::ABSTRACT)) { + $this->fatalError( + $v, + "Non-abstract class `{$className}` contains abstract hooked property `\${$propName}`", + ); + } + return; + } + + // Without the abstract modifier every declared hook needs a body. + foreach ($v->hooks as $hook) { + if ($hook->body === null) { + $this->fatalError($hook, 'Non-abstract property hook must have a body'); + } + } + } + protected function prepareClassMethod(Node\Stmt\ClassMethod $v, Node\Stmt\Class_|Node\Stmt\Trait_|Node\Stmt\Enum_ $class): void { $this->resetMethod();