fix(preprocessor): enforce property-hook placement rules for class properties (#63) --skip-tests

* 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.
master
Alessio Giacobbe 1 day ago committed by GitHub
parent ea0ea4414a
commit c578b1d6b0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
  1. 4
      phpunit/code/hook_rule_abstract_all_bodies.php
  2. 4
      phpunit/code/hook_rule_abstract_final.php
  3. 4
      phpunit/code/hook_rule_abstract_no_hooks.php
  4. 4
      phpunit/code/hook_rule_abstract_nonabstract_class.php
  5. 4
      phpunit/code/hook_rule_abstract_private.php
  6. 4
      phpunit/code/hook_rule_abstract_private_trait.php
  7. 6
      phpunit/code/hook_rule_abstract_protected_valid.php
  8. 4
      phpunit/code/hook_rule_bodyless.php
  9. 4
      phpunit/code/hook_rule_final_private.php
  10. 4
      phpunit/code/hook_rule_readonly.php
  11. 4
      phpunit/code/hook_rule_readonly_class.php
  12. 4
      phpunit/code/hook_rule_static.php
  13. 4
      phpunit/code/hook_rule_valid.php
  14. 83
      phpunit/src/PropertyHookPlacementTest.php
  15. 98
      src/Preprocessor.php

@ -0,0 +1,4 @@
<?php
abstract class Box { abstract public int $x { get => 1; } }
function main() {}

@ -0,0 +1,4 @@
<?php
abstract class Box { abstract public int $x { final get; } }
function main() {}

@ -0,0 +1,4 @@
<?php
abstract class Box { abstract public int $x; }
function main() {}

@ -0,0 +1,4 @@
<?php
class Box { abstract public int $x { get; } }
function main() {}

@ -0,0 +1,4 @@
<?php
abstract class Box { abstract private int $x { get; } }
function main() {}

@ -0,0 +1,4 @@
<?php
trait Boxed { abstract private int $x { get; } }
function main() {}

@ -0,0 +1,6 @@
<?php
abstract class Box { abstract protected int $x { get; } }
class Crate extends Box { protected int $x { get => 1; } }
trait Boxed { abstract protected int $y { get; } }
function main() {}

@ -0,0 +1,4 @@
<?php
class Box { public int $x { get; } }
function main() {}

@ -0,0 +1,4 @@
<?php
abstract class Box { abstract private int $x { final get; } }
function main() {}

@ -0,0 +1,4 @@
<?php
class Box { public readonly int $x { get => 1; } }
function main() {}

@ -0,0 +1,4 @@
<?php
readonly class Box { public int $x { get => 1; } }
function main() {}

@ -0,0 +1,4 @@
<?php
class Box { public static int $x { get => 1; } }
function main() {}

@ -0,0 +1,4 @@
<?php
abstract class Box { private int $b = 0; public int $x { get => $this->b; set { $this->b = $value; } } abstract public string $s { get; } }
function main() {}

@ -0,0 +1,83 @@
<?php
/**
* Zend property-hook placement rules for class/trait properties: no
* hooks on static or readonly properties, abstract hooked properties
* only in abstract containers with at least one bodiless hook, and a
* mandatory body on every non-abstract hook.
*/
class PropertyHookPlacementTest extends BaseTest
{
public function testHooksOnStaticPropertyAreRejected(): void
{
$this->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');
}
}

@ -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();

Loading…
Cancel
Save