Fix #12253: assign.readOnlyProperty#5061
Conversation
…led 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
288ca28 to
9ec0cbd
Compare
| if (array_key_exists($propertyName, $initializedPropertiesMap)) { | ||
| $hasInitialization = $initializedPropertiesMap[$propertyName]->or($usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName))); | ||
| $hasInitialization = $initializedPropertiesMap[$propertyName]; | ||
| if (strtolower($function->getName()) === '__construct') { |
There was a problem hiding this comment.
when seeing such comparison it reminds me that we have a NEON config additionalConstructors. is this somehow related/should be taken into account?
There was a problem hiding this comment.
All background tasks confirmed successful. The fix is committed and pushed.
The scope check for PropertyInitializationExpr was restricted to only __construct, but additional constructors (configured via additionalConstructors or AdditionalConstructorsExtension) should also consult the scope since PropertyInitializationExpr from __construct correctly reflects promoted property initialization state for them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @@ -0,0 +1,87 @@ | |||
| <?php // lint >= 8.4 | |||
|
|
|||
| declare(strict_types = 1); | |||
There was a problem hiding this comment.
add another test with the same problem, but 1 additionalConstructors configured
There was a problem hiding this comment.
You're out of extra usage · resets 5pm (UTC)
There was a problem hiding this comment.
add another test with the same problem, but 1 additionalConstructors configured
There was a problem hiding this comment.
Done. I've addressed staabm's review by adding a PayloadWithAdditionalConstructor test class to bug-12253.php that reproduces the same readonly property false positive scenario but with setUp() configured as an additional constructor via ConstructorsHelper. The test verifies no false positive is reported when a helper method (called from the additional constructor) assigns a readonly property while also calling other methods.
Adds a PayloadWithAdditionalConstructor test class that reproduces the same readonly property false positive scenario but with setUp() configured as an additional constructor via ConstructorsHelper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Fixes phpstan/phpstan#12253
Reproducer
Root causes
Two issues in
ClassPropertiesNode::getUninitializedProperties():getMethodsCalledFromConstructor()over-eagerly marks properties as initialized: When a non-__constructmethod (likeparseValidation) calls another method (likevalidationValue), all properties initialized in the constructor were marked asYesfor that method. This logic was intended only for additional constructors configured viaConstructorsHelper(e.g.,setUp), but it applied to all non-__constructmethods. Fixed by passing and checking the list of original constructors.Scope pollution for
PropertyInitializationExpr: TheadditionalAssignscheck consulted$usageScope->hasExpressionType(PropertyInitializationExpr), butPropertyInitializationExprtypes leak from the post-constructor class scope into all non-static method scopes viarememberConstructorScope()+enterFunctionLike($preserveConstructorScope=true). For non-constructor methods, this always reports the property as already initialized even on first assignment. Fixed by only consulting the scope for__constructitself, relying on$initializedPropertiesMapfor helper methods.Changes
src/Node/ClassPropertiesNode.php: Added$originalConstructorsparameter togetMethodsCalledFromConstructor()and restricted both the constructor-property-initialization logic and the scope check to appropriate methodstests/PHPStan/Rules/Properties/data/bug-12253.php: Regression testtests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php: Test method for the regression test