Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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 %s::$%s property which might get hooked in subclass.',
$propertyReflection->getDeclaringClass()->getDisplayName(),
$foundPropertyReflection->getName(),
),
)
->line($node->getStartLine())
->identifier('unset.maybeHookedProperty')
->build();
}
}
}

return null;
Expand Down
61 changes: 58 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,16 @@ public function testBug2752(): void

public function testBug4289(): void
{
$this->analyse([__DIR__ . '/data/bug-4289.php'], []);
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
Expand Down Expand Up @@ -94,7 +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,
Expand All @@ -120,6 +143,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 UnsetHookedProperty\NonFinalClass::$publicProperty property which might get hooked in 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