Skip to content

Fix #12253: assign.readOnlyProperty#5061

Open
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-fvdtxi4
Open

Fix #12253: assign.readOnlyProperty#5061
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-fvdtxi4

Conversation

@phpstan-bot
Copy link
Collaborator

This PR fixes phpstan/phpstan#12253 - a false positive assign.readOnlyProperty error when a readonly property is assigned in a method called from the constructor that also calls other methods.

Reproducer

class Payload
{
    private(set) readonly array $validation;

    public function __construct(private readonly stdClass $payload)
    {
        $this->parseValidation();
    }

    private function parseValidation(): void
    {
        // ... builds $validations array, calls $this->validationValue() ...
        $this->validation = $validations; // FALSE POSITIVE: "already assigned"
    }
}

Root causes

Two issues in ClassPropertiesNode::getUninitializedProperties():

  1. getMethodsCalledFromConstructor() over-eagerly marks properties as initialized: When a non-__construct method (like parseValidation) calls another method (like validationValue), all properties initialized in the constructor were marked as Yes for that method. This logic was intended only for additional constructors configured via ConstructorsHelper (e.g., setUp), but it applied to all non-__construct methods. Fixed by passing and checking the list of original constructors.

  2. Scope pollution for PropertyInitializationExpr: The additionalAssigns check consulted $usageScope->hasExpressionType(PropertyInitializationExpr), but PropertyInitializationExpr types leak from the post-constructor class scope into all non-static method scopes via rememberConstructorScope() + 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 __construct itself, relying on $initializedPropertiesMap for helper methods.

Changes

  • src/Node/ClassPropertiesNode.php: Added $originalConstructors parameter to getMethodsCalledFromConstructor() and restricted both the constructor-property-initialization logic and the scope check to appropriate methods
  • tests/PHPStan/Rules/Properties/data/bug-12253.php: Regression test
  • tests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php: Test method for the regression test

…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant