Skip to content

Commit 23bc628

Browse files
staabmphpstan-bot
authored andcommitted
Fix readonly property modification through clone() not reported outside allowed scope
- ReadOnlyPropertyAssignRule and ReadOnlyByPhpDocPropertyAssignRule unconditionally skipped all checks when inCloneWith was true, but should still enforce the "assigned outside declaring class" check - Moved the inCloneWith early-exit inside the loop, after the declaring class check, so that only the constructor requirement is skipped for clone-with operations - Added regression test for phpstan/phpstan#14063 - Updated existing clone-with test expectations to include new error cases
1 parent c009cb0 commit 23bc628

7 files changed

Lines changed: 71 additions & 10 deletions

src/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRule.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,6 @@ public function processNode(Node $node, Scope $scope): array
4848
}
4949

5050
$inCloneWith = (bool) $propertyFetch->getAttribute('inCloneWith', false);
51-
if ($inCloneWith) {
52-
return [];
53-
}
5451

5552
$inFunction = $scope->getFunction();
5653
if (
@@ -97,6 +94,10 @@ public function processNode(Node $node, Scope $scope): array
9794
continue;
9895
}
9996

97+
if ($inCloneWith) {
98+
continue;
99+
}
100+
100101
$scopeMethod = $scope->getFunction();
101102
if (!$scopeMethod instanceof MethodReflection) {
102103
throw new ShouldNotHappenException();

src/Rules/Properties/ReadOnlyPropertyAssignRule.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,6 @@ public function processNode(Node $node, Scope $scope): array
4747
}
4848

4949
$inCloneWith = (bool) $propertyFetch->getAttribute('inCloneWith', false);
50-
if ($inCloneWith) {
51-
return [];
52-
}
5350

5451
$errors = [];
5552
$reflections = $this->propertyReflectionFinder->findPropertyReflectionsFromNode($propertyFetch, $scope);
@@ -84,6 +81,10 @@ public function processNode(Node $node, Scope $scope): array
8481
continue;
8582
}
8683

84+
if ($inCloneWith) {
85+
continue;
86+
}
87+
8788
$scopeMethod = $scope->getFunction();
8889
if (!$scopeMethod instanceof MethodReflection) {
8990
throw new ShouldNotHappenException();

tests/PHPStan/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRuleTest.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,12 @@ public function testPropertyHooks(): void
188188
#[RequiresPhp('>= 8.5')]
189189
public function testCloneWith(): void
190190
{
191-
$this->analyse([__DIR__ . '/data/readonly-phpdoc-property-assign-clone-with.php'], []);
191+
$this->analyse([__DIR__ . '/data/readonly-phpdoc-property-assign-clone-with.php'], [
192+
[
193+
'@readonly property ReadonlyPhpDocPropertyAssignCloneWith\Foo::$pub is assigned outside of its declaring class.',
194+
29,
195+
],
196+
]);
192197
}
193198

194199
}

tests/PHPStan/Rules/Properties/ReadOnlyPropertyAssignRuleTest.php

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,27 @@ public function testBug12537(): void
177177
#[RequiresPhp('>= 8.5')]
178178
public function testCloneWith(): void
179179
{
180-
$this->analyse([__DIR__ . '/data/readonly-property-assign-clone-with.php'], []);
180+
$this->analyse([__DIR__ . '/data/readonly-property-assign-clone-with.php'], [
181+
[
182+
'Readonly property ReadonlyPropertyAssignCloneWith\Foo::$pubSet is assigned outside of its declaring class.',
183+
28,
184+
],
185+
[
186+
'Readonly property ReadonlyPropertyAssignCloneWith\Bar::$value is assigned outside of its declaring class.',
187+
47,
188+
],
189+
]);
190+
}
191+
192+
#[RequiresPhp('>= 8.5')]
193+
public function testBug14063(): void
194+
{
195+
$this->analyse([__DIR__ . '/data/bug-14063.php'], [
196+
[
197+
'Readonly property Bug14063\Obj::$value is assigned outside of its declaring class.',
198+
18,
199+
],
200+
]);
181201
}
182202

183203
}
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php // lint >= 8.5
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug14063;
6+
7+
final readonly class Obj
8+
{
9+
public function __construct(public string $value) {}
10+
11+
public function withValue(string $value): self
12+
{
13+
return clone($this, ['value' => $value]); // OK - inside declaring class
14+
}
15+
}
16+
17+
$obj = new Obj('val');
18+
$newObj = clone($obj, ['value' => 'newVal']);

tests/PHPStan/Rules/Properties/data/readonly-phpdoc-property-assign-clone-with.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,6 @@ function (Foo $foo): void {
2929
clone ($foo, [
3030
'priv' => 1, // reported in AccessPropertiesInAssignRule
3131
'prot' => 1, // reported in AccessPropertiesInAssignRule
32-
'pub' => 1, // reported in AccessPropertiesInAssignRule
32+
'pub' => 1, // reported here - @readonly property assigned outside declaring class
3333
]);
3434
};

tests/PHPStan/Rules/Properties/data/readonly-property-assign-clone-with.php

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,22 @@ function (Foo $foo): void {
2929
'priv' => 1, // reported in AccessPropertiesInAssignRule
3030
'prot' => 1, // reported in AccessPropertiesInAssignRule
3131
'pub' => 1, // reported in AccessPropertiesInAssignRule
32-
'pubSet' => 1,
32+
'pubSet' => 1, // reported here - readonly property assigned outside declaring class
33+
]);
34+
};
35+
36+
final readonly class Bar
37+
{
38+
public function __construct(public string $value) {}
39+
40+
public function withValue(string $value): self
41+
{
42+
return clone($this, ['value' => $value]); // OK - inside declaring class
43+
}
44+
}
45+
46+
function (Bar $bar): void {
47+
clone ($bar, [
48+
'value' => 'newVal', // reported in AccessPropertiesInAssignRule (protected(set)) - and also here
3349
]);
3450
};

0 commit comments

Comments
 (0)