From 1b778869ae056e4f7c4954925eee413c53170c73 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 28 Feb 2025 10:31:38 +0100 Subject: [PATCH 1/8] Hooked properties cannot be unset() --- src/Rules/Variables/UnsetRule.php | 13 ++++++ .../PHPStan/Rules/Variables/UnsetRuleTest.php | 27 ++++++++++++ .../Variables/data/unset-hooked-property.php | 41 +++++++++++++++++++ 3 files changed, 81 insertions(+) create mode 100644 tests/PHPStan/Rules/Variables/data/unset-hooked-property.php diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 8240e35e95..42a0b0f635 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -103,6 +103,19 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc') ->build(); } + + if ($propertyReflection->getNativeReflection()->getHooks() !== []) { + return RuleErrorBuilder::message( + sprintf( + 'Cannot unset hooked property %s::$%s.', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($node->getStartLine()) + ->identifier('unset.hookedProperty') + ->build(); + } } return null; diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 0b5861c1c0..e916d3fcca 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -5,6 +5,7 @@ use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; +use const PHP_VERSION_ID; /** * @extends RuleTestCase @@ -122,4 +123,30 @@ public function testBug12421(): void ]); } + 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 property UnsetHookedProperty\User::$name.', + 6, + ], + [ + 'Cannot unset hooked property UnsetHookedProperty\User::$fullName.', + 7, + ], + [ + 'Cannot unset hooked property UnsetHookedProperty\Foo::$ii.', + 9, + ], + [ + 'Cannot unset hooked property UnsetHookedProperty\Foo::$iii.', + 10, + ], + ]); + } + } 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..74b0c3d531 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php @@ -0,0 +1,41 @@ += 8.4 + +namespace UnsetHookedProperty; + +function doUnset(Foo $foo, User $user): void { + unset($user->name); + unset($user->fullName); + + unset($foo->ii); + unset($foo->iii); +} + +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; } +} + From 6171ebeda0825f21be8394586fe1b1c2c1a2812d Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 28 Feb 2025 10:46:43 +0100 Subject: [PATCH 2/8] more consitent error message --- src/Rules/Variables/UnsetRule.php | 2 +- tests/PHPStan/Rules/Variables/UnsetRuleTest.php | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 42a0b0f635..e668e4ec1f 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -107,7 +107,7 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError if ($propertyReflection->getNativeReflection()->getHooks() !== []) { return RuleErrorBuilder::message( sprintf( - 'Cannot unset hooked property %s::$%s.', + 'Cannot unset hooked %s::$%s property.', $propertyReflection->getDeclaringClass()->getDisplayName(), $foundPropertyReflection->getName(), ), diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index e916d3fcca..ee0deed279 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -131,19 +131,19 @@ public function testUnsetHookedProperty(): void $this->analyse([__DIR__ . '/data/unset-hooked-property.php'], [ [ - 'Cannot unset hooked property UnsetHookedProperty\User::$name.', + 'Cannot unset hooked UnsetHookedProperty\User::$name property.', 6, ], [ - 'Cannot unset hooked property UnsetHookedProperty\User::$fullName.', + 'Cannot unset hooked UnsetHookedProperty\User::$fullName property.', 7, ], [ - 'Cannot unset hooked property UnsetHookedProperty\Foo::$ii.', + 'Cannot unset hooked UnsetHookedProperty\Foo::$ii property.', 9, ], [ - 'Cannot unset hooked property UnsetHookedProperty\Foo::$iii.', + 'Cannot unset hooked UnsetHookedProperty\Foo::$iii property.', 10, ], ]); From 19e1ff9f43b2ef53fca8322648d41e280c7000f1 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 28 Feb 2025 11:14:11 +0100 Subject: [PATCH 3/8] use $propertyReflection->isHooked() --- src/Reflection/Php/PhpPropertyReflection.php | 5 +++++ src/Rules/Variables/UnsetRule.php | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) 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 e668e4ec1f..17d5414de5 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -104,7 +104,7 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ->build(); } - if ($propertyReflection->getNativeReflection()->getHooks() !== []) { + if ($propertyReflection->isHooked()) { return RuleErrorBuilder::message( sprintf( 'Cannot unset hooked %s::$%s property.', From 3f82a55b5813fa0b773cfd82bc868ed74987160a Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sat, 1 Mar 2025 18:09:07 +0100 Subject: [PATCH 4/8] Cannot unset property which might get hooked in subclass. --- src/Rules/Variables/UnsetRule.php | 19 +++++++++++++ .../PHPStan/Rules/Variables/UnsetRuleTest.php | 21 +++++++++++++-- .../Variables/data/unset-hooked-property.php | 27 ++++++++++++++++++- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 17d5414de5..86d2a6192f 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, ) { } @@ -115,6 +117,23 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ->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 %s::$%s property which might get hooked in subclass.', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($node->getStartLine()) + ->identifier('unset.maybeHookedProperty') + ->build(); + } } } diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index ee0deed279..ea8c1fb600 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Variables; +use PHPStan\Php\PhpVersion; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; @@ -15,7 +16,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 @@ -56,7 +60,12 @@ public function testBug2752(): void public function testBug4289(): void { - $this->analyse([__DIR__ . '/data/bug-4289.php'], []); + $this->analyse([__DIR__ . '/data/bug-4289.php'], [ + [ + 'Cannot unset Bug4289\BaseClass::$fields property which might get hooked in subclass.', + 25, + ], + ]); } public function testBug5223(): void @@ -96,6 +105,10 @@ public function testBug4565(): void public function testBug12421(): void { $this->analyse([__DIR__ . '/data/bug-12421.php'], [ + [ + 'Cannot unset Bug12421\RegularProperty::$y property which might get hooked in subclass.', + 7, + ], [ 'Cannot unset readonly Bug12421\NativeReadonlyClass::$y property.', 11, @@ -146,6 +159,10 @@ public function testUnsetHookedProperty(): void 'Cannot unset hooked UnsetHookedProperty\Foo::$iii property.', 10, ], + [ + 'Cannot unset UnsetHookedProperty\NonFinalClass::$publicProperty property which might get hooked in subclass.', + 13, + ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php index 74b0c3d531..109b09b507 100644 --- a/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php +++ b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php @@ -2,12 +2,18 @@ namespace UnsetHookedProperty; -function doUnset(Foo $foo, User $user): void { +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 @@ -39,3 +45,22 @@ abstract class Foo 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); + } +} From 28a16967a849424c2104f33a1de9c69bc27d7b4a Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sat, 1 Mar 2025 18:27:42 +0100 Subject: [PATCH 5/8] fix version dependent errors --- .../PHPStan/Rules/Variables/UnsetRuleTest.php | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index ea8c1fb600..885970edbb 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -6,6 +6,7 @@ use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; +use function array_merge; use const PHP_VERSION_ID; /** @@ -60,12 +61,16 @@ public function testBug2752(): void public function testBug4289(): void { - $this->analyse([__DIR__ . '/data/bug-4289.php'], [ - [ - 'Cannot unset Bug4289\BaseClass::$fields property which might get hooked in subclass.', - 25, - ], - ]); + if (PHP_VERSION_ID < 80400) { + $this->analyse([__DIR__ . '/data/bug-4289.php'], []); + } else { + $this->analyse([__DIR__ . '/data/bug-4289.php'], [ + [ + 'Cannot unset Bug4289\BaseClass::$fields property which might get hooked in subclass.', + 25, + ], + ]); + } } public function testBug5223(): void @@ -104,11 +109,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 Bug12421\RegularProperty::$y property which might get hooked in subclass.', 7, - ], + ]; + } + + $errors = array_merge($errors, [ [ 'Cannot unset readonly Bug12421\NativeReadonlyClass::$y property.', 11, @@ -134,6 +143,8 @@ public function testBug12421(): void 34, ], ]); + + $this->analyse([__DIR__ . '/data/bug-12421.php'], $errors); } public function testUnsetHookedProperty(): void From 4e25c3eb90d86694fdcbcc8b6b3f9784a570413f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Mirtes?= Date: Sun, 2 Mar 2025 09:44:18 +0100 Subject: [PATCH 6/8] Update src/Rules/Variables/UnsetRule.php --- src/Rules/Variables/UnsetRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 86d2a6192f..f75cf64cd8 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -125,7 +125,7 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ) { return RuleErrorBuilder::message( sprintf( - 'Cannot unset %s::$%s property which might get hooked in subclass.', + 'Cannot unset property %s::$%s because it might have hooks in a subclass.', $propertyReflection->getDeclaringClass()->getDisplayName(), $foundPropertyReflection->getName(), ), From b93676ff7cf9c457a7677d6cb83923560258f7c3 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sun, 2 Mar 2025 10:01:50 +0100 Subject: [PATCH 7/8] Update UnsetRule.php --- src/Rules/Variables/UnsetRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index f75cf64cd8..a376744bc2 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -131,7 +131,7 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ), ) ->line($node->getStartLine()) - ->identifier('unset.maybeHookedProperty') + ->identifier('unset.possiblyHookedProperty') ->build(); } } From 149b829ddf6cd54810a555730d84473b380a4d5c Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sun, 2 Mar 2025 10:02:58 +0100 Subject: [PATCH 8/8] Update UnsetRuleTest.php --- .../PHPStan/Rules/Variables/UnsetRuleTest.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 885970edbb..0564fbe509 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -61,16 +61,18 @@ public function testBug2752(): void public function testBug4289(): void { - if (PHP_VERSION_ID < 80400) { - $this->analyse([__DIR__ . '/data/bug-4289.php'], []); - } else { - $this->analyse([__DIR__ . '/data/bug-4289.php'], [ + $errors = []; + + if (PHP_VERSION_ID >= 80400) { + $errors = [ [ - 'Cannot unset Bug4289\BaseClass::$fields property which might get hooked in subclass.', + '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 @@ -112,7 +114,7 @@ public function testBug12421(): void $errors = []; if (PHP_VERSION_ID >= 80400) { $errors[] = [ - 'Cannot unset Bug12421\RegularProperty::$y property which might get hooked in subclass.', + 'Cannot unset property Bug12421\RegularProperty::$y because it might have hooks in a subclass.', 7, ]; } @@ -171,7 +173,7 @@ public function testUnsetHookedProperty(): void 10, ], [ - 'Cannot unset UnsetHookedProperty\NonFinalClass::$publicProperty property which might get hooked in subclass.', + 'Cannot unset property UnsetHookedProperty\NonFinalClass::$publicProperty because it might have hooks in a subclass.', 13, ], ]);