From f26ae2f46a5c15854d39bcb68ba73048dfb91267 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sat, 15 Feb 2025 17:30:51 +0100 Subject: [PATCH 1/6] Readonly properties cannot be unset() --- src/Rules/Variables/UnsetRule.php | 39 ++++++++ .../PHPStan/Rules/Variables/UnsetRuleTest.php | 26 +++++ .../Rules/Variables/data/bug-12421.php | 97 +++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 tests/PHPStan/Rules/Variables/data/bug-12421.php diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index dc75eb024e..e2b8c4e356 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -69,6 +69,45 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError } return $this->canBeUnset($node->var, $scope); + } elseif ( + $node instanceof Node\Expr\PropertyFetch + && $node->name instanceof Node\Identifier + ) { + $type = $scope->getType($node->var); + foreach ($type->getObjectClassReflections() as $classReflection) { + if ($classReflection->isReadOnly() || $classReflection->isImmutable()) { + return RuleErrorBuilder::message( + sprintf( + 'Cannot unset property %s of %s class %s.', + $node->name->name, + $classReflection->isReadOnly() ? 'readonly' : 'immutable', + $type->describe(VerbosityLevel::value()), + ), + ) + ->line($node->getStartLine()) + ->identifier('unset.readonlyClass') + ->build(); + } + + if (!$classReflection->hasProperty($node->name->name)) { + continue; + } + + $propertyReflection = $classReflection->getNativeProperty($node->name->name); + if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) { + return RuleErrorBuilder::message( + sprintf( + 'Cannot unset %s property %s of %s.', + $propertyReflection->isReadOnly() ? 'readonly' : '@readonly', + $node->name->name, + $type->describe(VerbosityLevel::value()), + ), + ) + ->line($node->getStartLine()) + ->identifier('unset.readonlyProperty') + ->build(); + } + } } return null; diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 036d09c974..9a970205e4 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -91,4 +91,30 @@ public function testBug4565(): void $this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-4565.php'], []); } + public function testBug12421(): void + { + $this->analyse([__DIR__ . '/data/bug-12421.php'], [ + [ + 'Cannot unset property y of readonly class Bug12421\NativeReadonlyClass.', + 78, + ], + [ + 'Cannot unset readonly property y of Bug12421\NativeReadonlyProperty.', + 82, + ], + [ + 'Cannot unset property y of immutable class Bug12421\PhpdocReadonlyClass.', + 86, + ], + [ + 'Cannot unset @readonly property y of Bug12421\PhpdocReadonlyProperty.', + 90, + ], + [ + 'Cannot unset property y of immutable class Bug12421\PhpdocImmutableClass.', + 94, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-12421.php b/tests/PHPStan/Rules/Variables/data/bug-12421.php new file mode 100644 index 0000000000..382bab59ea --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-12421.php @@ -0,0 +1,97 @@ += 8.2 + +namespace Bug12421; + +readonly class NativeReadonlyClass +{ + public Y $y; + + public function __construct() + { + $this->y = new Y(); + } +} + +class NativeReadonlyProperty +{ + public readonly Y $y; + + public function __construct() + { + $this->y = new Y(); + } +} + +/** @readonly */ +class PhpdocReadonlyClass +{ + public Y $y; + + public function __construct() + { + $this->y = new Y(); + } +} + +class PhpdocReadonlyProperty +{ + /** @readonly */ + public Y $y; + + public function __construct() + { + $this->y = new Y(); + } +} + +/** @immutable */ +class PhpdocImmutableClass +{ + public Y $y; + + public function __construct() + { + $this->y = new Y(); + } +} + +class RegularProperty +{ + public Y $y; + + public function __construct() + { + $this->y = new Y(); + } +} + +class Y +{ +} + +function doFoo() { + $x = new RegularProperty(); + unset($x->y); + var_dump($x->y); + + $x = new NativeReadonlyClass(); + unset($x->y); + var_dump($x->y); + + $x = new NativeReadonlyProperty(); + unset($x->y); + var_dump($x->y); + + $x = new PhpdocReadonlyClass(); + unset($x->y); + var_dump($x->y); + + $x = new PhpdocReadonlyProperty(); + unset($x->y); + var_dump($x->y); + + $x = new PhpdocImmutableClass(); + unset($x->y); + var_dump($x->y); +} + From c07127cee881f6bb4f43df83b557ee9eb8a01483 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sat, 15 Feb 2025 17:49:02 +0100 Subject: [PATCH 2/6] fix unset on stdClass --- src/Rules/Variables/UnsetRule.php | 2 +- tests/PHPStan/Rules/Variables/data/bug-12421.php | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index e2b8c4e356..73cff9513e 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -89,7 +89,7 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ->build(); } - if (!$classReflection->hasProperty($node->name->name)) { + if (!$classReflection->hasNativeProperty($node->name->name)) { continue; } diff --git a/tests/PHPStan/Rules/Variables/data/bug-12421.php b/tests/PHPStan/Rules/Variables/data/bug-12421.php index 382bab59ea..d5c342f30b 100644 --- a/tests/PHPStan/Rules/Variables/data/bug-12421.php +++ b/tests/PHPStan/Rules/Variables/data/bug-12421.php @@ -93,5 +93,9 @@ function doFoo() { $x = new PhpdocImmutableClass(); unset($x->y); var_dump($x->y); + + $x = new \stdClass(); + unset($x->y); + } From ca308ab072f9d0751396aea5a035718d1cb3df1a Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sun, 16 Feb 2025 08:53:02 +0100 Subject: [PATCH 3/6] test subclass --- .../PHPStan/Rules/Variables/UnsetRuleTest.php | 14 ++-- .../Rules/Variables/data/bug-12421.php | 65 ++++++++++--------- 2 files changed, 45 insertions(+), 34 deletions(-) diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 9a970205e4..6beb6e337a 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -96,23 +96,27 @@ public function testBug12421(): void $this->analyse([__DIR__ . '/data/bug-12421.php'], [ [ 'Cannot unset property y of readonly class Bug12421\NativeReadonlyClass.', - 78, + 11, ], [ 'Cannot unset readonly property y of Bug12421\NativeReadonlyProperty.', - 82, + 15, ], [ 'Cannot unset property y of immutable class Bug12421\PhpdocReadonlyClass.', - 86, + 19, ], [ 'Cannot unset @readonly property y of Bug12421\PhpdocReadonlyProperty.', - 90, + 23, ], [ 'Cannot unset property y of immutable class Bug12421\PhpdocImmutableClass.', - 94, + 27, + ], + [ + 'Cannot unset readonly property y of Bug12421\NativeReadonlyPropertySubClass.', + 34, ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/bug-12421.php b/tests/PHPStan/Rules/Variables/data/bug-12421.php index d5c342f30b..eb0c53f755 100644 --- a/tests/PHPStan/Rules/Variables/data/bug-12421.php +++ b/tests/PHPStan/Rules/Variables/data/bug-12421.php @@ -2,6 +2,39 @@ namespace Bug12421; +function doFoo() { + $x = new RegularProperty(); + unset($x->y); + var_dump($x->y); + + $x = new NativeReadonlyClass(); + unset($x->y); + var_dump($x->y); + + $x = new NativeReadonlyProperty(); + unset($x->y); + var_dump($x->y); + + $x = new PhpdocReadonlyClass(); + unset($x->y); + var_dump($x->y); + + $x = new PhpdocReadonlyProperty(); + unset($x->y); + var_dump($x->y); + + $x = new PhpdocImmutableClass(); + unset($x->y); + var_dump($x->y); + + $x = new \stdClass(); + unset($x->y); + + $x = new NativeReadonlyPropertySubClass(); + unset($x->y); + var_dump($x->y); +} + readonly class NativeReadonlyClass { public Y $y; @@ -65,37 +98,11 @@ public function __construct() } } -class Y +class NativeReadonlyPropertySubClass extends NativeReadonlyProperty { } -function doFoo() { - $x = new RegularProperty(); - unset($x->y); - var_dump($x->y); - - $x = new NativeReadonlyClass(); - unset($x->y); - var_dump($x->y); - - $x = new NativeReadonlyProperty(); - unset($x->y); - var_dump($x->y); - - $x = new PhpdocReadonlyClass(); - unset($x->y); - var_dump($x->y); - - $x = new PhpdocReadonlyProperty(); - unset($x->y); - var_dump($x->y); - - $x = new PhpdocImmutableClass(); - unset($x->y); - var_dump($x->y); - - $x = new \stdClass(); - unset($x->y); - +class Y +{ } From aa238e20a53c38d93b24b999d7e6f247662486b5 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Thu, 27 Feb 2025 20:03:26 +0100 Subject: [PATCH 4/6] fix --- src/Rules/Variables/UnsetRule.php | 16 +--------------- tests/PHPStan/Rules/Variables/UnsetRuleTest.php | 6 +++--- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 73cff9513e..dee9fcaa11 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -75,20 +75,6 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ) { $type = $scope->getType($node->var); foreach ($type->getObjectClassReflections() as $classReflection) { - if ($classReflection->isReadOnly() || $classReflection->isImmutable()) { - return RuleErrorBuilder::message( - sprintf( - 'Cannot unset property %s of %s class %s.', - $node->name->name, - $classReflection->isReadOnly() ? 'readonly' : 'immutable', - $type->describe(VerbosityLevel::value()), - ), - ) - ->line($node->getStartLine()) - ->identifier('unset.readonlyClass') - ->build(); - } - if (!$classReflection->hasNativeProperty($node->name->name)) { continue; } @@ -104,7 +90,7 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError ), ) ->line($node->getStartLine()) - ->identifier('unset.readonlyProperty') + ->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc') ->build(); } } diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 6beb6e337a..901121e578 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -95,7 +95,7 @@ public function testBug12421(): void { $this->analyse([__DIR__ . '/data/bug-12421.php'], [ [ - 'Cannot unset property y of readonly class Bug12421\NativeReadonlyClass.', + 'Cannot unset readonly property y of Bug12421\NativeReadonlyClass.', 11, ], [ @@ -103,7 +103,7 @@ public function testBug12421(): void 15, ], [ - 'Cannot unset property y of immutable class Bug12421\PhpdocReadonlyClass.', + 'Cannot unset @readonly property y of Bug12421\PhpdocReadonlyClass.', 19, ], [ @@ -111,7 +111,7 @@ public function testBug12421(): void 23, ], [ - 'Cannot unset property y of immutable class Bug12421\PhpdocImmutableClass.', + 'Cannot unset @readonly property y of Bug12421\PhpdocImmutableClass.', 27, ], [ From c4c7b8c7eb4fb3b913c2f4fc641a0d5f75e98630 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 28 Feb 2025 07:27:02 +0100 Subject: [PATCH 5/6] Utilize PropertyReflectionFinder --- src/Rules/Variables/UnsetRule.php | 51 +++++++++++-------- .../PHPStan/Rules/Variables/UnsetRuleTest.php | 3 +- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index dee9fcaa11..1a963045fd 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -5,6 +5,7 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\VerbosityLevel; @@ -17,6 +18,12 @@ final class UnsetRule implements Rule { + public function __construct( + private PropertyReflectionFinder $propertyReflectionFinder, + ) + { + } + public function getNodeType(): string { return Node\Stmt\Unset_::class; @@ -73,26 +80,30 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError $node instanceof Node\Expr\PropertyFetch && $node->name instanceof Node\Identifier ) { - $type = $scope->getType($node->var); - foreach ($type->getObjectClassReflections() as $classReflection) { - if (!$classReflection->hasNativeProperty($node->name->name)) { - continue; - } - - $propertyReflection = $classReflection->getNativeProperty($node->name->name); - if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) { - return RuleErrorBuilder::message( - sprintf( - 'Cannot unset %s property %s of %s.', - $propertyReflection->isReadOnly() ? 'readonly' : '@readonly', - $node->name->name, - $type->describe(VerbosityLevel::value()), - ), - ) - ->line($node->getStartLine()) - ->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc') - ->build(); - } + $foundPropertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($node, $scope); + if ($foundPropertyReflection === null) { + return null; + } + + $propertyReflection = $foundPropertyReflection->getNativeReflection(); + if ($propertyReflection === null) { + return null; + } + + if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) { + $type = $scope->getType($node->var); + + return RuleErrorBuilder::message( + sprintf( + 'Cannot unset %s property %s of %s.', + $propertyReflection->isReadOnly() ? 'readonly' : '@readonly', + $node->name->name, + $type->describe(VerbosityLevel::value()), + ), + ) + ->line($node->getStartLine()) + ->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc') + ->build(); } } diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 901121e578..2e9418cbe8 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\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; @@ -13,7 +14,7 @@ class UnsetRuleTest extends RuleTestCase protected function getRule(): Rule { - return new UnsetRule(); + return new UnsetRule(self::getContainer()->getByType(PropertyReflectionFinder::class)); } public function testUnsetRule(): void From 71ff1f6509243326ccae6f1fb48a815a8372e34b Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 28 Feb 2025 07:52:07 +0100 Subject: [PATCH 6/6] Yennefer of Vengerberg ;) --- src/Rules/Variables/UnsetRule.php | 8 +++----- tests/PHPStan/Rules/Variables/UnsetRuleTest.php | 12 ++++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index 1a963045fd..8240e35e95 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -91,14 +91,12 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError } if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) { - $type = $scope->getType($node->var); - return RuleErrorBuilder::message( sprintf( - 'Cannot unset %s property %s of %s.', + 'Cannot unset %s %s::$%s property.', $propertyReflection->isReadOnly() ? 'readonly' : '@readonly', - $node->name->name, - $type->describe(VerbosityLevel::value()), + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), ), ) ->line($node->getStartLine()) diff --git a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php index 2e9418cbe8..0b5861c1c0 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -96,27 +96,27 @@ public function testBug12421(): void { $this->analyse([__DIR__ . '/data/bug-12421.php'], [ [ - 'Cannot unset readonly property y of Bug12421\NativeReadonlyClass.', + 'Cannot unset readonly Bug12421\NativeReadonlyClass::$y property.', 11, ], [ - 'Cannot unset readonly property y of Bug12421\NativeReadonlyProperty.', + 'Cannot unset readonly Bug12421\NativeReadonlyProperty::$y property.', 15, ], [ - 'Cannot unset @readonly property y of Bug12421\PhpdocReadonlyClass.', + 'Cannot unset @readonly Bug12421\PhpdocReadonlyClass::$y property.', 19, ], [ - 'Cannot unset @readonly property y of Bug12421\PhpdocReadonlyProperty.', + 'Cannot unset @readonly Bug12421\PhpdocReadonlyProperty::$y property.', 23, ], [ - 'Cannot unset @readonly property y of Bug12421\PhpdocImmutableClass.', + 'Cannot unset @readonly Bug12421\PhpdocImmutableClass::$y property.', 27, ], [ - 'Cannot unset readonly property y of Bug12421\NativeReadonlyPropertySubClass.', + 'Cannot unset readonly Bug12421\NativeReadonlyProperty::$y property.', 34, ], ]);