Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Reflection/Php/PhpPropertyReflection.php
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down
32 changes: 32 additions & 0 deletions src/Rules/Variables/UnsetRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -20,6 +21,7 @@ final class UnsetRule implements Rule

public function __construct(
private PropertyReflectionFinder $propertyReflectionFinder,
private PhpVersion $phpVersion,
)
{
}
Expand Down Expand Up @@ -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()
) {
Comment on lines +121 to +125
Copy link
Contributor Author

@staabm staabm Mar 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also though about adding

public function isHookedInSubclass(): TrinaryLogic {
		if (
			!$this->isPrivate()
			&& !$this->isFinal()->yes()
			&& !$this->getDeclaringClass()->isFinal()
		) {
			return TrinaryLogic::createMaybe();
		}

		return TrinaryLogic::createNo();
	}

to PhpPropertyReflection. wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed right now :) I'd have to this about that more.

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;
Expand Down
63 changes: 60 additions & 3 deletions tests/PHPStan/Rules/Variables/UnsetRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<UnsetRule>
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
],
]);
}

}
66 changes: 66 additions & 0 deletions tests/PHPStan/Rules/Variables/data/unset-hooked-property.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<?php // lint >= 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);
}
}
Loading