diff --git a/src/Reflection/Php/PhpPropertyReflection.php b/src/Reflection/Php/PhpPropertyReflection.php index c667c6e5e3..05228b7016 100644 --- a/src/Reflection/Php/PhpPropertyReflection.php +++ b/src/Reflection/Php/PhpPropertyReflection.php @@ -256,6 +256,11 @@ public function hasHook(string $hookType): bool return $this->setHook !== null; } + public function isHooked(): bool + { + return $this->getHook !== null || $this->setHook !== null; + } + public function getHook(string $hookType): ExtendedMethodReflection { if ($hookType === 'get') { diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 8240e35e95..a376744bc2 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -4,6 +4,7 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; +use PHPStan\Php\PhpVersion; use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; @@ -20,6 +21,7 @@ final class UnsetRule implements Rule public function __construct( private PropertyReflectionFinder $propertyReflectionFinder, + private PhpVersion $phpVersion, ) { } @@ -103,6 +105,36 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc') ->build(); } + + if ($propertyReflection->isHooked()) { + return RuleErrorBuilder::message( + sprintf( + 'Cannot unset hooked %s::$%s property.', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($node->getStartLine()) + ->identifier('unset.hookedProperty') + ->build(); + } elseif ($this->phpVersion->supportsPropertyHooks()) { + if ( + !$propertyReflection->isPrivate() + && !$propertyReflection->isFinal()->yes() + && !$propertyReflection->getDeclaringClass()->isFinal() + ) { + return RuleErrorBuilder::message( + sprintf( + 'Cannot unset property %s::$%s because it might have hooks in a subclass.', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($node->getStartLine()) + ->identifier('unset.possiblyHookedProperty') + ->build(); + } + } } return null; diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 0b5861c1c0..0564fbe509 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -2,9 +2,12 @@ namespace PHPStan\Rules\Variables; +use PHPStan\Php\PhpVersion; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; +use function array_merge; +use const PHP_VERSION_ID; /** * @extends RuleTestCase @@ -14,7 +17,10 @@ class UnsetRuleTest extends RuleTestCase protected function getRule(): Rule { - return new UnsetRule(self::getContainer()->getByType(PropertyReflectionFinder::class)); + return new UnsetRule( + self::getContainer()->getByType(PropertyReflectionFinder::class), + self::getContainer()->getByType(PhpVersion::class), + ); } public function testUnsetRule(): void @@ -55,7 +61,18 @@ public function testBug2752(): void public function testBug4289(): void { - $this->analyse([__DIR__ . '/data/bug-4289.php'], []); + $errors = []; + + if (PHP_VERSION_ID >= 80400) { + $errors = [ + [ + 'Cannot unset property Bug4289\BaseClass::$fields because it might have hooks in a subclass.', + 25, + ], + ]; + } + + $this->analyse([__DIR__ . '/data/bug-4289.php'], $errors); } public function testBug5223(): void @@ -94,7 +111,15 @@ public function testBug4565(): void public function testBug12421(): void { - $this->analyse([__DIR__ . '/data/bug-12421.php'], [ + $errors = []; + if (PHP_VERSION_ID >= 80400) { + $errors[] = [ + 'Cannot unset property Bug12421\RegularProperty::$y because it might have hooks in a subclass.', + 7, + ]; + } + + $errors = array_merge($errors, [ [ 'Cannot unset readonly Bug12421\NativeReadonlyClass::$y property.', 11, @@ -120,6 +145,38 @@ public function testBug12421(): void 34, ], ]); + + $this->analyse([__DIR__ . '/data/bug-12421.php'], $errors); + } + + public function testUnsetHookedProperty(): void + { + if (PHP_VERSION_ID < 80400) { + $this->markTestSkipped('Test requires PHP 8.4 or later.'); + } + + $this->analyse([__DIR__ . '/data/unset-hooked-property.php'], [ + [ + 'Cannot unset hooked UnsetHookedProperty\User::$name property.', + 6, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$fullName property.', + 7, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\Foo::$ii property.', + 9, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\Foo::$iii property.', + 10, + ], + [ + 'Cannot unset property UnsetHookedProperty\NonFinalClass::$publicProperty because it might have hooks in a subclass.', + 13, + ], + ]); } } diff --git a/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php new file mode 100644 index 0000000000..109b09b507 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php @@ -0,0 +1,66 @@ += 8.4 + +namespace UnsetHookedProperty; + +function doUnset(Foo $foo, User $user, NonFinalClass $nonFinalClass, FinalClass $finalClass): void { + unset($user->name); + unset($user->fullName); + + unset($foo->ii); + unset($foo->iii); + + unset($nonFinalClass->publicFinalProperty); + unset($nonFinalClass->publicProperty); + + unset($finalClass->publicFinalProperty); + unset($finalClass->publicProperty); +} + +class User +{ + public string $name { + set { + if (strlen($value) === 0) { + throw new \ValueError("Name must be non-empty"); + } + $this->name = $value; + } + } + + public string $fullName { + get { + return "Yennefer of Vengerberg"; + } + } + + public function __construct(string $name) { + $this->name = $name; + } +} + +abstract class Foo +{ + abstract protected int $ii { get; } + + abstract public int $iii { get; } +} + +class NonFinalClass { + private string $privateProperty; + public string $publicProperty; + final public string $publicFinalProperty; + + function doFoo() { + unset($this->privateProperty); + } +} + +final class FinalClass { + private string $privateProperty; + public string $publicProperty; + final public string $publicFinalProperty; + + function doFoo() { + unset($this->privateProperty); + } +}