From 23bc628b1ae75df1d3f51e09b33c1698ddfcb982 Mon Sep 17 00:00:00 2001 From: staabm <120441+staabm@users.noreply.github.com> Date: Sat, 21 Mar 2026 08:17:43 +0000 Subject: [PATCH] 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 --- .../ReadOnlyByPhpDocPropertyAssignRule.php | 7 +++--- .../Properties/ReadOnlyPropertyAssignRule.php | 7 +++--- ...ReadOnlyByPhpDocPropertyAssignRuleTest.php | 7 +++++- .../ReadOnlyPropertyAssignRuleTest.php | 22 ++++++++++++++++++- .../Rules/Properties/data/bug-14063.php | 18 +++++++++++++++ ...only-phpdoc-property-assign-clone-with.php | 2 +- .../readonly-property-assign-clone-with.php | 18 ++++++++++++++- 7 files changed, 71 insertions(+), 10 deletions(-) create mode 100644 tests/PHPStan/Rules/Properties/data/bug-14063.php diff --git a/src/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRule.php b/src/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRule.php index e61a9a2426..696b37e1f9 100644 --- a/src/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRule.php +++ b/src/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRule.php @@ -48,9 +48,6 @@ public function processNode(Node $node, Scope $scope): array } $inCloneWith = (bool) $propertyFetch->getAttribute('inCloneWith', false); - if ($inCloneWith) { - return []; - } $inFunction = $scope->getFunction(); if ( @@ -97,6 +94,10 @@ public function processNode(Node $node, Scope $scope): array continue; } + if ($inCloneWith) { + continue; + } + $scopeMethod = $scope->getFunction(); if (!$scopeMethod instanceof MethodReflection) { throw new ShouldNotHappenException(); diff --git a/src/Rules/Properties/ReadOnlyPropertyAssignRule.php b/src/Rules/Properties/ReadOnlyPropertyAssignRule.php index 987e000256..74cb5025a2 100644 --- a/src/Rules/Properties/ReadOnlyPropertyAssignRule.php +++ b/src/Rules/Properties/ReadOnlyPropertyAssignRule.php @@ -47,9 +47,6 @@ public function processNode(Node $node, Scope $scope): array } $inCloneWith = (bool) $propertyFetch->getAttribute('inCloneWith', false); - if ($inCloneWith) { - return []; - } $errors = []; $reflections = $this->propertyReflectionFinder->findPropertyReflectionsFromNode($propertyFetch, $scope); @@ -84,6 +81,10 @@ public function processNode(Node $node, Scope $scope): array continue; } + if ($inCloneWith) { + continue; + } + $scopeMethod = $scope->getFunction(); if (!$scopeMethod instanceof MethodReflection) { throw new ShouldNotHappenException(); diff --git a/tests/PHPStan/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRuleTest.php b/tests/PHPStan/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRuleTest.php index 849e6f6938..6e5c5dcb46 100644 --- a/tests/PHPStan/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRuleTest.php +++ b/tests/PHPStan/Rules/Properties/ReadOnlyByPhpDocPropertyAssignRuleTest.php @@ -188,7 +188,12 @@ public function testPropertyHooks(): void #[RequiresPhp('>= 8.5')] public function testCloneWith(): void { - $this->analyse([__DIR__ . '/data/readonly-phpdoc-property-assign-clone-with.php'], []); + $this->analyse([__DIR__ . '/data/readonly-phpdoc-property-assign-clone-with.php'], [ + [ + '@readonly property ReadonlyPhpDocPropertyAssignCloneWith\Foo::$pub is assigned outside of its declaring class.', + 29, + ], + ]); } } diff --git a/tests/PHPStan/Rules/Properties/ReadOnlyPropertyAssignRuleTest.php b/tests/PHPStan/Rules/Properties/ReadOnlyPropertyAssignRuleTest.php index 40bbb2d5c3..c461bcca8a 100644 --- a/tests/PHPStan/Rules/Properties/ReadOnlyPropertyAssignRuleTest.php +++ b/tests/PHPStan/Rules/Properties/ReadOnlyPropertyAssignRuleTest.php @@ -177,7 +177,27 @@ public function testBug12537(): void #[RequiresPhp('>= 8.5')] public function testCloneWith(): void { - $this->analyse([__DIR__ . '/data/readonly-property-assign-clone-with.php'], []); + $this->analyse([__DIR__ . '/data/readonly-property-assign-clone-with.php'], [ + [ + 'Readonly property ReadonlyPropertyAssignCloneWith\Foo::$pubSet is assigned outside of its declaring class.', + 28, + ], + [ + 'Readonly property ReadonlyPropertyAssignCloneWith\Bar::$value is assigned outside of its declaring class.', + 47, + ], + ]); + } + + #[RequiresPhp('>= 8.5')] + public function testBug14063(): void + { + $this->analyse([__DIR__ . '/data/bug-14063.php'], [ + [ + 'Readonly property Bug14063\Obj::$value is assigned outside of its declaring class.', + 18, + ], + ]); } } diff --git a/tests/PHPStan/Rules/Properties/data/bug-14063.php b/tests/PHPStan/Rules/Properties/data/bug-14063.php new file mode 100644 index 0000000000..906db6e2cf --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/bug-14063.php @@ -0,0 +1,18 @@ += 8.5 + +declare(strict_types = 1); + +namespace Bug14063; + +final readonly class Obj +{ + public function __construct(public string $value) {} + + public function withValue(string $value): self + { + return clone($this, ['value' => $value]); // OK - inside declaring class + } +} + +$obj = new Obj('val'); +$newObj = clone($obj, ['value' => 'newVal']); diff --git a/tests/PHPStan/Rules/Properties/data/readonly-phpdoc-property-assign-clone-with.php b/tests/PHPStan/Rules/Properties/data/readonly-phpdoc-property-assign-clone-with.php index 2a120a4c73..cb85233f99 100644 --- a/tests/PHPStan/Rules/Properties/data/readonly-phpdoc-property-assign-clone-with.php +++ b/tests/PHPStan/Rules/Properties/data/readonly-phpdoc-property-assign-clone-with.php @@ -29,6 +29,6 @@ function (Foo $foo): void { clone ($foo, [ 'priv' => 1, // reported in AccessPropertiesInAssignRule 'prot' => 1, // reported in AccessPropertiesInAssignRule - 'pub' => 1, // reported in AccessPropertiesInAssignRule + 'pub' => 1, // reported here - @readonly property assigned outside declaring class ]); }; diff --git a/tests/PHPStan/Rules/Properties/data/readonly-property-assign-clone-with.php b/tests/PHPStan/Rules/Properties/data/readonly-property-assign-clone-with.php index a7c96d1b98..0fd5d44f7d 100644 --- a/tests/PHPStan/Rules/Properties/data/readonly-property-assign-clone-with.php +++ b/tests/PHPStan/Rules/Properties/data/readonly-property-assign-clone-with.php @@ -29,6 +29,22 @@ function (Foo $foo): void { 'priv' => 1, // reported in AccessPropertiesInAssignRule 'prot' => 1, // reported in AccessPropertiesInAssignRule 'pub' => 1, // reported in AccessPropertiesInAssignRule - 'pubSet' => 1, + 'pubSet' => 1, // reported here - readonly property assigned outside declaring class + ]); +}; + +final readonly class Bar +{ + public function __construct(public string $value) {} + + public function withValue(string $value): self + { + return clone($this, ['value' => $value]); // OK - inside declaring class + } +} + +function (Bar $bar): void { + clone ($bar, [ + 'value' => 'newVal', // reported in AccessPropertiesInAssignRule (protected(set)) - and also here ]); };