Skip to content

Commit b143449

Browse files
staabmstaabm
authored
Hooked properties cannot be unset() 
1 parent f87b890 commit b143449

File tree

4 files changed

+163
-3
lines changed

4 files changed

+163
-3
lines changed

‎src/Reflection/Php/PhpPropertyReflection.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,11 @@ public function hasHook(string $hookType): bool
256256
return $this->setHook !== null;
257257
}
258258

259+
public function isHooked(): bool
260+
{
261+
return $this->getHook !== null || $this->setHook !== null;
262+
}
263+
259264
public function getHook(string $hookType): ExtendedMethodReflection
260265
{
261266
if ($hookType === 'get') {

‎src/Rules/Variables/UnsetRule.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use PhpParser\Node;
66
use PHPStan\Analyser\Scope;
7+
use PHPStan\Php\PhpVersion;
78
use PHPStan\Rules\IdentifierRuleError;
89
use PHPStan\Rules\Properties\PropertyReflectionFinder;
910
use PHPStan\Rules\Rule;
@@ -20,6 +21,7 @@ final class UnsetRule implements Rule
2021

2122
public function __construct(
2223
private PropertyReflectionFinder $propertyReflectionFinder,
24+
private PhpVersion $phpVersion,
2325
)
2426
{
2527
}
@@ -103,6 +105,36 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError
103105
->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc')
104106
->build();
105107
}
108+
109+
if ($propertyReflection->isHooked()) {
110+
return RuleErrorBuilder::message(
111+
sprintf(
112+
'Cannot unset hooked %s::$%s property.',
113+
$propertyReflection->getDeclaringClass()->getDisplayName(),
114+
$foundPropertyReflection->getName(),
115+
),
116+
)
117+
->line($node->getStartLine())
118+
->identifier('unset.hookedProperty')
119+
->build();
120+
} elseif ($this->phpVersion->supportsPropertyHooks()) {
121+
if (
122+
!$propertyReflection->isPrivate()
123+
&& !$propertyReflection->isFinal()->yes()
124+
&& !$propertyReflection->getDeclaringClass()->isFinal()
125+
) {
126+
return RuleErrorBuilder::message(
127+
sprintf(
128+
'Cannot unset property %s::$%s because it might have hooks in a subclass.',
129+
$propertyReflection->getDeclaringClass()->getDisplayName(),
130+
$foundPropertyReflection->getName(),
131+
),
132+
)
133+
->line($node->getStartLine())
134+
->identifier('unset.possiblyHookedProperty')
135+
->build();
136+
}
137+
}
106138
}
107139

108140
return null;

‎tests/PHPStan/Rules/Variables/UnsetRuleTest.php

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@
22

33
namespace PHPStan\Rules\Variables;
44

5+
use PHPStan\Php\PhpVersion;
56
use PHPStan\Rules\Properties\PropertyReflectionFinder;
67
use PHPStan\Rules\Rule;
78
use PHPStan\Testing\RuleTestCase;
9+
use function array_merge;
10+
use const PHP_VERSION_ID;
811

912
/**
1013
* @extends RuleTestCase<UnsetRule>
@@ -14,7 +17,10 @@ class UnsetRuleTest extends RuleTestCase
1417

1518
protected function getRule(): Rule
1619
{
17-
return new UnsetRule(self::getContainer()->getByType(PropertyReflectionFinder::class));
20+
return new UnsetRule(
21+
self::getContainer()->getByType(PropertyReflectionFinder::class),
22+
self::getContainer()->getByType(PhpVersion::class),
23+
);
1824
}
1925

2026
public function testUnsetRule(): void
@@ -55,7 +61,18 @@ public function testBug2752(): void
5561

5662
public function testBug4289(): void
5763
{
58-
$this->analyse([__DIR__ . '/data/bug-4289.php'], []);
64+
$errors = [];
65+
66+
if (PHP_VERSION_ID >= 80400) {
67+
$errors = [
68+
[
69+
'Cannot unset property Bug4289\BaseClass::$fields because it might have hooks in a subclass.',
70+
25,
71+
],
72+
];
73+
}
74+
75+
$this->analyse([__DIR__ . '/data/bug-4289.php'], $errors);
5976
}
6077

6178
public function testBug5223(): void
@@ -94,7 +111,15 @@ public function testBug4565(): void
94111

95112
public function testBug12421(): void
96113
{
97-
$this->analyse([__DIR__ . '/data/bug-12421.php'], [
114+
$errors = [];
115+
if (PHP_VERSION_ID >= 80400) {
116+
$errors[] = [
117+
'Cannot unset property Bug12421\RegularProperty::$y because it might have hooks in a subclass.',
118+
7,
119+
];
120+
}
121+
122+
$errors = array_merge($errors, [
98123
[
99124
'Cannot unset readonly Bug12421\NativeReadonlyClass::$y property.',
100125
11,
@@ -120,6 +145,38 @@ public function testBug12421(): void
120145
34,
121146
],
122147
]);
148+
149+
$this->analyse([__DIR__ . '/data/bug-12421.php'], $errors);
150+
}
151+
152+
public function testUnsetHookedProperty(): void
153+
{
154+
if (PHP_VERSION_ID < 80400) {
155+
$this->markTestSkipped('Test requires PHP 8.4 or later.');
156+
}
157+
158+
$this->analyse([__DIR__ . '/data/unset-hooked-property.php'], [
159+
[
160+
'Cannot unset hooked UnsetHookedProperty\User::$name property.',
161+
6,
162+
],
163+
[
164+
'Cannot unset hooked UnsetHookedProperty\User::$fullName property.',
165+
7,
166+
],
167+
[
168+
'Cannot unset hooked UnsetHookedProperty\Foo::$ii property.',
169+
9,
170+
],
171+
[
172+
'Cannot unset hooked UnsetHookedProperty\Foo::$iii property.',
173+
10,
174+
],
175+
[
176+
'Cannot unset property UnsetHookedProperty\NonFinalClass::$publicProperty because it might have hooks in a subclass.',
177+
13,
178+
],
179+
]);
123180
}
124181

125182
}

‎tests/PHPStan/Rules/Variables/data/unset-hooked-property.php

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
<?php // lint >= 8.4
2+
3+
namespace UnsetHookedProperty;
4+
5+
function doUnset(Foo $foo, User $user, NonFinalClass $nonFinalClass, FinalClass $finalClass): void {
6+
unset($user->name);
7+
unset($user->fullName);
8+
9+
unset($foo->ii);
10+
unset($foo->iii);
11+
12+
unset($nonFinalClass->publicFinalProperty);
13+
unset($nonFinalClass->publicProperty);
14+
15+
unset($finalClass->publicFinalProperty);
16+
unset($finalClass->publicProperty);
17+
}
18+
19+
class User
20+
{
21+
public string $name {
22+
set {
23+
if (strlen($value) === 0) {
24+
throw new \ValueError("Name must be non-empty");
25+
}
26+
$this->name = $value;
27+
}
28+
}
29+
30+
public string $fullName {
31+
get {
32+
return "Yennefer of Vengerberg";
33+
}
34+
}
35+
36+
public function __construct(string $name) {
37+
$this->name = $name;
38+
}
39+
}
40+
41+
abstract class Foo
42+
{
43+
abstract protected int $ii { get; }
44+
45+
abstract public int $iii { get; }
46+
}
47+
48+
class NonFinalClass {
49+
private string $privateProperty;
50+
public string $publicProperty;
51+
final public string $publicFinalProperty;
52+
53+
function doFoo() {
54+
unset($this->privateProperty);
55+
}
56+
}
57+
58+
final class FinalClass {
59+
private string $privateProperty;
60+
public string $publicProperty;
61+
final public string $publicFinalProperty;
62+
63+
function doFoo() {
64+
unset($this->privateProperty);
65+
}
66+
}

0 commit comments

Comments
 (0)