From 4154f0a29357d56a68e3d35048025f36dffb0016 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Thu, 6 Mar 2025 14:38:29 +0100 Subject: [PATCH] Fix unsetting array item triggers unset.possiblyHookedProperty --- src/Rules/Variables/UnsetRule.php | 118 +++++++++--------- .../PHPStan/Rules/Variables/UnsetRuleTest.php | 61 +++++++-- .../Variables/data/unset-hooked-property.php | 84 +++++++++++++ 3 files changed, 194 insertions(+), 69 deletions(-) diff --git a/src/Rules/Variables/UnsetRule.php b/src/Rules/Variables/UnsetRule.php index a376744bc2..91e5260488 100644 --- a/src/Rules/Variables/UnsetRule.php +++ b/src/Rules/Variables/UnsetRule.php @@ -37,6 +37,67 @@ public function processNode(Node $node, Scope $scope): array $errors = []; foreach ($functionArguments as $argument) { + if ( + $argument instanceof Node\Expr\PropertyFetch + && $argument->name instanceof Node\Identifier + ) { + $foundPropertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($argument, $scope); + if ($foundPropertyReflection === null) { + continue; + } + + $propertyReflection = $foundPropertyReflection->getNativeReflection(); + if ($propertyReflection === null) { + continue; + } + + if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) { + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Cannot unset %s %s::$%s property.', + $propertyReflection->isReadOnly() ? 'readonly' : '@readonly', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($argument->getStartLine()) + ->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc') + ->build(); + continue; + } + + if ($propertyReflection->isHooked()) { + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Cannot unset hooked %s::$%s property.', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($argument->getStartLine()) + ->identifier('unset.hookedProperty') + ->build(); + continue; + } elseif ($this->phpVersion->supportsPropertyHooks()) { + if ( + !$propertyReflection->isPrivate() + && !$propertyReflection->isFinal()->yes() + && !$propertyReflection->getDeclaringClass()->isFinal() + ) { + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Cannot unset property %s::$%s because it might have hooks in a subclass.', + $propertyReflection->getDeclaringClass()->getDisplayName(), + $foundPropertyReflection->getName(), + ), + ) + ->line($argument->getStartLine()) + ->identifier('unset.possiblyHookedProperty') + ->build(); + continue; + } + } + } $error = $this->canBeUnset($argument, $scope); if ($error === null) { continue; @@ -78,63 +139,6 @@ 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 - ) { - $foundPropertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($node, $scope); - if ($foundPropertyReflection === null) { - return null; - } - - $propertyReflection = $foundPropertyReflection->getNativeReflection(); - if ($propertyReflection === null) { - return null; - } - - if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) { - return RuleErrorBuilder::message( - sprintf( - 'Cannot unset %s %s::$%s property.', - $propertyReflection->isReadOnly() ? 'readonly' : '@readonly', - $propertyReflection->getDeclaringClass()->getDisplayName(), - $foundPropertyReflection->getName(), - ), - ) - ->line($node->getStartLine()) - ->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 8f7081b679..df34c15d50 100644 --- a/tests/PHPStan/Rules/Variables/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/UnsetRuleTest.php @@ -61,18 +61,7 @@ public function testBug2752(): void public function testBug4289(): void { - $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); + $this->analyse([__DIR__ . '/data/bug-4289.php'], []); } public function testBug5223(): void @@ -180,6 +169,54 @@ public function testUnsetHookedProperty(): void 'Cannot unset property UnsetHookedProperty\NonFinalClass::$publicProperty because it might have hooks in a subclass.', 13, ], + [ + 'Cannot unset property UnsetHookedProperty\ContainerClass::$finalClass because it might have hooks in a subclass.', + 83, + ], + [ + 'Cannot unset property UnsetHookedProperty\ContainerClass::$nonFinalClass because it might have hooks in a subclass.', + 87, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\Foo::$iii property.', + 89, + ], + [ + 'Cannot unset property UnsetHookedProperty\ContainerClass::$foo because it might have hooks in a subclass.', + 90, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$name property.', + 92, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$fullName property.', + 93, + ], + [ + 'Cannot unset property UnsetHookedProperty\ContainerClass::$user because it might have hooks in a subclass.', + 94, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$name property.', + 96, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$name property.', + 97, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$fullName property.', + 98, + ], + [ + 'Cannot unset hooked UnsetHookedProperty\User::$fullName property.', + 99, + ], + [ + 'Cannot unset property UnsetHookedProperty\ContainerClass::$arrayOfUsers because it might have hooks in a subclass.', + 100, + ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php index 109b09b507..d98eed672a 100644 --- a/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php +++ b/tests/PHPStan/Rules/Variables/data/unset-hooked-property.php @@ -64,3 +64,87 @@ function doFoo() { unset($this->privateProperty); } } + +class ContainerClass { + public FinalClass $finalClass; + public FinalClass $nonFinalClass; + + public Foo $foo; + + public User $user; + + /** @var array */ + public array $arrayOfUsers; +} + +function dooNestedUnset(ContainerClass $containerClass) { + unset($containerClass->finalClass->publicFinalProperty); + unset($containerClass->finalClass->publicProperty); + unset($containerClass->finalClass); + + unset($containerClass->nonFinalClass->publicFinalProperty); + unset($containerClass->nonFinalClass->publicProperty); + unset($containerClass->nonFinalClass); + + unset($containerClass->foo->iii); + unset($containerClass->foo); + + unset($containerClass->user->name); + unset($containerClass->user->fullName); + unset($containerClass->user); + + unset($containerClass->arrayOfUsers[0]->name); + unset($containerClass->arrayOfUsers[0]->name); + unset($containerClass->arrayOfUsers['hans']->fullName); + unset($containerClass->arrayOfUsers['hans']->fullName); + unset($containerClass->arrayOfUsers); +} + +class Bug12695 +{ + /** @var int[] */ + public array $values = [1]; + public function test(): void + { + unset($this->values[0]); + } +} + +abstract class Bug12695_AbstractJsonView +{ + protected array $variables = []; + + public function render(): array + { + return $this->variables; + } +} + +class Bug12695_GetSeminarDateJsonView extends Bug12695_AbstractJsonView +{ + public function render(): array + { + unset($this->variables['settings']); + return parent::render(); + } +} + +class Bug12695_AddBookingsJsonView extends Bug12695_GetSeminarDateJsonView +{ + public function render(): array + { + unset($this->variables['seminarDate']); + return parent::render(); + } +} + +class UnsetReadonly +{ + /** @var int[][] */ + public readonly array $a; + + public function doFoo(): void + { + unset($this->a[5]); + } +}