Skip to content

Commit 4154f0a

Browse files
staabmondrejmirtes
staabm
authored and
ondrejmirtes
committed
Fix unsetting array item triggers unset.possiblyHookedProperty
1 parent ed4ea0a commit 4154f0a

File tree

3 files changed

+194
-69
lines changed

3 files changed

+194
-69
lines changed

‎src/Rules/Variables/UnsetRule.php‎

Lines changed: 61 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,67 @@ public function processNode(Node $node, Scope $scope): array
3737
$errors = [];
3838

3939
foreach ($functionArguments as $argument) {
40+
if (
41+
$argument instanceof Node\Expr\PropertyFetch
42+
&& $argument->name instanceof Node\Identifier
43+
) {
44+
$foundPropertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($argument, $scope);
45+
if ($foundPropertyReflection === null) {
46+
continue;
47+
}
48+
49+
$propertyReflection = $foundPropertyReflection->getNativeReflection();
50+
if ($propertyReflection === null) {
51+
continue;
52+
}
53+
54+
if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) {
55+
$errors[] = RuleErrorBuilder::message(
56+
sprintf(
57+
'Cannot unset %s %s::$%s property.',
58+
$propertyReflection->isReadOnly() ? 'readonly' : '@readonly',
59+
$propertyReflection->getDeclaringClass()->getDisplayName(),
60+
$foundPropertyReflection->getName(),
61+
),
62+
)
63+
->line($argument->getStartLine())
64+
->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc')
65+
->build();
66+
continue;
67+
}
68+
69+
if ($propertyReflection->isHooked()) {
70+
$errors[] = RuleErrorBuilder::message(
71+
sprintf(
72+
'Cannot unset hooked %s::$%s property.',
73+
$propertyReflection->getDeclaringClass()->getDisplayName(),
74+
$foundPropertyReflection->getName(),
75+
),
76+
)
77+
->line($argument->getStartLine())
78+
->identifier('unset.hookedProperty')
79+
->build();
80+
continue;
81+
} elseif ($this->phpVersion->supportsPropertyHooks()) {
82+
if (
83+
!$propertyReflection->isPrivate()
84+
&& !$propertyReflection->isFinal()->yes()
85+
&& !$propertyReflection->getDeclaringClass()->isFinal()
86+
) {
87+
$errors[] = RuleErrorBuilder::message(
88+
sprintf(
89+
'Cannot unset property %s::$%s because it might have hooks in a subclass.',
90+
$propertyReflection->getDeclaringClass()->getDisplayName(),
91+
$foundPropertyReflection->getName(),
92+
),
93+
)
94+
->line($argument->getStartLine())
95+
->identifier('unset.possiblyHookedProperty')
96+
->build();
97+
continue;
98+
}
99+
}
100+
}
40101
$error = $this->canBeUnset($argument, $scope);
41102
if ($error === null) {
42103
continue;
@@ -78,63 +139,6 @@ private function canBeUnset(Node $node, Scope $scope): ?IdentifierRuleError
78139
}
79140

80141
return $this->canBeUnset($node->var, $scope);
81-
} elseif (
82-
$node instanceof Node\Expr\PropertyFetch
83-
&& $node->name instanceof Node\Identifier
84-
) {
85-
$foundPropertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($node, $scope);
86-
if ($foundPropertyReflection === null) {
87-
return null;
88-
}
89-
90-
$propertyReflection = $foundPropertyReflection->getNativeReflection();
91-
if ($propertyReflection === null) {
92-
return null;
93-
}
94-
95-
if ($propertyReflection->isReadOnly() || $propertyReflection->isReadOnlyByPhpDoc()) {
96-
return RuleErrorBuilder::message(
97-
sprintf(
98-
'Cannot unset %s %s::$%s property.',
99-
$propertyReflection->isReadOnly() ? 'readonly' : '@readonly',
100-
$propertyReflection->getDeclaringClass()->getDisplayName(),
101-
$foundPropertyReflection->getName(),
102-
),
103-
)
104-
->line($node->getStartLine())
105-
->identifier($propertyReflection->isReadOnly() ? 'unset.readOnlyProperty' : 'unset.readOnlyPropertyByPhpDoc')
106-
->build();
107-
}
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-
}
138142
}
139143

140144
return null;

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

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -61,18 +61,7 @@ public function testBug2752(): void
6161

6262
public function testBug4289(): void
6363
{
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);
64+
$this->analyse([__DIR__ . '/data/bug-4289.php'], []);
7665
}
7766

7867
public function testBug5223(): void
@@ -180,6 +169,54 @@ public function testUnsetHookedProperty(): void
180169
'Cannot unset property UnsetHookedProperty\NonFinalClass::$publicProperty because it might have hooks in a subclass.',
181170
13,
182171
],
172+
[
173+
'Cannot unset property UnsetHookedProperty\ContainerClass::$finalClass because it might have hooks in a subclass.',
174+
83,
175+
],
176+
[
177+
'Cannot unset property UnsetHookedProperty\ContainerClass::$nonFinalClass because it might have hooks in a subclass.',
178+
87,
179+
],
180+
[
181+
'Cannot unset hooked UnsetHookedProperty\Foo::$iii property.',
182+
89,
183+
],
184+
[
185+
'Cannot unset property UnsetHookedProperty\ContainerClass::$foo because it might have hooks in a subclass.',
186+
90,
187+
],
188+
[
189+
'Cannot unset hooked UnsetHookedProperty\User::$name property.',
190+
92,
191+
],
192+
[
193+
'Cannot unset hooked UnsetHookedProperty\User::$fullName property.',
194+
93,
195+
],
196+
[
197+
'Cannot unset property UnsetHookedProperty\ContainerClass::$user because it might have hooks in a subclass.',
198+
94,
199+
],
200+
[
201+
'Cannot unset hooked UnsetHookedProperty\User::$name property.',
202+
96,
203+
],
204+
[
205+
'Cannot unset hooked UnsetHookedProperty\User::$name property.',
206+
97,
207+
],
208+
[
209+
'Cannot unset hooked UnsetHookedProperty\User::$fullName property.',
210+
98,
211+
],
212+
[
213+
'Cannot unset hooked UnsetHookedProperty\User::$fullName property.',
214+
99,
215+
],
216+
[
217+
'Cannot unset property UnsetHookedProperty\ContainerClass::$arrayOfUsers because it might have hooks in a subclass.',
218+
100,
219+
],
183220
]);
184221
}
185222

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

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,87 @@ function doFoo() {
6464
unset($this->privateProperty);
6565
}
6666
}
67+
68+
class ContainerClass {
69+
public FinalClass $finalClass;
70+
public FinalClass $nonFinalClass;
71+
72+
public Foo $foo;
73+
74+
public User $user;
75+
76+
/** @var array<User> */
77+
public array $arrayOfUsers;
78+
}
79+
80+
function dooNestedUnset(ContainerClass $containerClass) {
81+
unset($containerClass->finalClass->publicFinalProperty);
82+
unset($containerClass->finalClass->publicProperty);
83+
unset($containerClass->finalClass);
84+
85+
unset($containerClass->nonFinalClass->publicFinalProperty);
86+
unset($containerClass->nonFinalClass->publicProperty);
87+
unset($containerClass->nonFinalClass);
88+
89+
unset($containerClass->foo->iii);
90+
unset($containerClass->foo);
91+
92+
unset($containerClass->user->name);
93+
unset($containerClass->user->fullName);
94+
unset($containerClass->user);
95+
96+
unset($containerClass->arrayOfUsers[0]->name);
97+
unset($containerClass->arrayOfUsers[0]->name);
98+
unset($containerClass->arrayOfUsers['hans']->fullName);
99+
unset($containerClass->arrayOfUsers['hans']->fullName);
100+
unset($containerClass->arrayOfUsers);
101+
}
102+
103+
class Bug12695
104+
{
105+
/** @var int[] */
106+
public array $values = [1];
107+
public function test(): void
108+
{
109+
unset($this->values[0]);
110+
}
111+
}
112+
113+
abstract class Bug12695_AbstractJsonView
114+
{
115+
protected array $variables = [];
116+
117+
public function render(): array
118+
{
119+
return $this->variables;
120+
}
121+
}
122+
123+
class Bug12695_GetSeminarDateJsonView extends Bug12695_AbstractJsonView
124+
{
125+
public function render(): array
126+
{
127+
unset($this->variables['settings']);
128+
return parent::render();
129+
}
130+
}
131+
132+
class Bug12695_AddBookingsJsonView extends Bug12695_GetSeminarDateJsonView
133+
{
134+
public function render(): array
135+
{
136+
unset($this->variables['seminarDate']);
137+
return parent::render();
138+
}
139+
}
140+
141+
class UnsetReadonly
142+
{
143+
/** @var int[][] */
144+
public readonly array $a;
145+
146+
public function doFoo(): void
147+
{
148+
unset($this->a[5]);
149+
}
150+
}

0 commit comments

Comments
 (0)