Skip to content

Commit 288ca28

Browse files
phpstan-botphpstan-bot
committed
Fix false positive "readonly property already assigned" in method called from constructor
When a constructor calls a helper method that assigns a readonly property and also calls other methods, the property was incorrectly reported as "already assigned". Two root causes: 1. In getMethodsCalledFromConstructor(), non-__construct methods that called other methods had all constructor-initialized properties marked as Yes. This was intended only for additional constructors (like setUp) but incorrectly applied to regular helper methods. Fixed by tracking which methods are original constructors vs discovered helper methods. 2. The additionalAssigns check used scope's hasExpressionType() for PropertyInitializationExpr, but this expression leaks from the constructor scope into all non-static method scopes via rememberConstructorScope(). For non-constructor methods, the scope check always returns Yes even for the first assignment. Fixed by only consulting the scope for __construct itself, and relying on the initializedPropertiesMap for other methods. Fixes phpstan/phpstan#12253
1 parent d462113 commit 288ca28

File tree

3 files changed

+102
-5
lines changed

3 files changed

+102
-5
lines changed

‎src/Node/ClassPropertiesNode.php‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,9 @@ public function getUninitializedProperties(
166166
$initializedInConstructor = array_diff_key($uninitializedProperties, $this->collectUninitializedProperties([$classReflection->getConstructor()->getName()], $uninitializedProperties));
167167
}
168168

169-
$methodsCalledFromConstructor = $this->getMethodsCalledFromConstructor($classReflection, $initialInitializedProperties, $initializedProperties, $constructors, $initializedInConstructor);
169+
$methodsCalledFromConstructor = $this->getMethodsCalledFromConstructor($classReflection, $initialInitializedProperties, $initializedProperties, $constructors, $initializedInConstructor, $constructors);
170170
$prematureAccess = [];
171171
$additionalAssigns = [];
172-
173172
foreach ($this->getPropertyUsages() as $usage) {
174173
$fetch = $usage->getFetch();
175174
if (!$fetch instanceof PropertyFetch) {
@@ -211,7 +210,10 @@ public function getUninitializedProperties(
211210

212211
if ($usage instanceof PropertyWrite) {
213212
if (array_key_exists($propertyName, $initializedPropertiesMap)) {
214-
$hasInitialization = $initializedPropertiesMap[$propertyName]->or($usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName)));
213+
$hasInitialization = $initializedPropertiesMap[$propertyName];
214+
if (strtolower($function->getName()) === '__construct') {
215+
$hasInitialization = $hasInitialization->or($usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName)));
216+
}
215217
if (
216218
!$hasInitialization->no()
217219
&& !$usage->isPromotedPropertyWrite()
@@ -318,6 +320,7 @@ private function collectUninitializedProperties(array $constructors, array $unin
318320
* @param array<string, TrinaryLogic> $initialInitializedProperties
319321
* @param array<string, array<string, TrinaryLogic>> $initializedProperties
320322
* @param array<string, ClassPropertyNode> $initializedInConstructorProperties
323+
* @param string[] $originalConstructors
321324
*
322325
* @return array<string, array<string, TrinaryLogic>>
323326
*/
@@ -327,6 +330,7 @@ private function getMethodsCalledFromConstructor(
327330
array $initializedProperties,
328331
array $methods,
329332
array $initializedInConstructorProperties,
333+
array $originalConstructors,
330334
): array
331335
{
332336
$originalMap = $initializedProperties;
@@ -363,7 +367,7 @@ private function getMethodsCalledFromConstructor(
363367
continue;
364368
}
365369

366-
if ($inMethod->getName() !== '__construct') {
370+
if ($inMethod->getName() !== '__construct' && in_array($inMethod->getName(), $originalConstructors, true)) {
367371
foreach (array_keys($initializedInConstructorProperties) as $propertyName) {
368372
$initializedProperties[$inMethod->getName()][$propertyName] = TrinaryLogic::createYes();
369373
}
@@ -391,7 +395,7 @@ private function getMethodsCalledFromConstructor(
391395
return $initializedProperties;
392396
}
393397

394-
return $this->getMethodsCalledFromConstructor($classReflection, $initialInitializedProperties, $initializedProperties, $methods, $initializedInConstructorProperties);
398+
return $this->getMethodsCalledFromConstructor($classReflection, $initialInitializedProperties, $initializedProperties, $methods, $initializedInConstructorProperties, $originalConstructors);
395399
}
396400

397401
/**

‎tests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,4 +342,10 @@ public function testBug11828(): void
342342
$this->analyse([__DIR__ . '/data/bug-11828.php'], []);
343343
}
344344

345+
#[RequiresPhp('>= 8.4')]
346+
public function testBug12253(): void
347+
{
348+
$this->analyse([__DIR__ . '/data/bug-12253.php'], []);
349+
}
350+
345351
}

‎tests/PHPStan/Rules/Properties/data/bug-12253.php‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
<?php // lint >= 8.4
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug12253;
6+
7+
use stdClass;
8+
9+
class Payload
10+
{
11+
/** @var array<array<string, mixed>> */
12+
private(set) readonly array $validation;
13+
14+
/** @var array<string, string> */
15+
private array $ids = [];
16+
17+
public function __construct(private readonly stdClass $payload)
18+
{
19+
$this->parseValidation();
20+
}
21+
22+
private function parseValidation(): void
23+
{
24+
$validations = [];
25+
26+
foreach ($this->payload->validation as $key => $validation) {
27+
$validations[] = [
28+
'id' => $key,
29+
'field_id' => $this->ids[$validation->field_id],
30+
'rule' => $validation->rule,
31+
'value' => $this->validationValue($validation->value),
32+
'message' => $validation->message,
33+
];
34+
}
35+
36+
$this->validation = $validations;
37+
}
38+
39+
private function validationValue(mixed $value): mixed
40+
{
41+
if (is_null($value)) {
42+
return null;
43+
}
44+
45+
return $this->ids[$value] ?? $value;
46+
}
47+
}
48+
49+
class PayloadWithoutAsymmetricVisibility
50+
{
51+
/** @var array<array<string, mixed>> */
52+
private readonly array $validation;
53+
54+
/** @var array<string, string> */
55+
private array $ids = [];
56+
57+
public function __construct(private readonly stdClass $payload)
58+
{
59+
$this->parseValidation();
60+
}
61+
62+
private function parseValidation(): void
63+
{
64+
$validations = [];
65+
66+
foreach ($this->payload->validation as $key => $validation) {
67+
$validations[] = [
68+
'id' => $key,
69+
'field_id' => $this->ids[$validation->field_id],
70+
'rule' => $validation->rule,
71+
'value' => $this->validationValue($validation->value),
72+
'message' => $validation->message,
73+
];
74+
}
75+
76+
$this->validation = $validations;
77+
}
78+
79+
private function validationValue(mixed $value): mixed
80+
{
81+
if (is_null($value)) {
82+
return null;
83+
}
84+
85+
return $this->ids[$value] ?? $value;
86+
}
87+
}

0 commit comments

Comments
 (0)