-
Notifications
You must be signed in to change notification settings - Fork 575
Allow reinitialization of a readonly property in __clone since PHP8.3 #5731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.1.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Node\Expr; | ||
|
|
||
| use Override; | ||
| use PhpParser\Node\Expr; | ||
| use PHPStan\Node\VirtualNode; | ||
|
|
||
| /** | ||
| * Tracks that a readonly property has been re-assigned within the current __clone() body. | ||
| * | ||
| * Distinct from PropertyInitializationExpr because PHP 8.3+ allows readonly properties | ||
| * to be re-initialized once inside __clone — but the property is already initialized at | ||
| * __clone's entry (carried over from the post-construction class scope), so the standard | ||
| * initialization tracker can't distinguish "first write inside __clone" from "no write yet | ||
| * inside __clone". This expression is only set when an assignment actually happens inside | ||
| * __clone, and is excluded from rememberConstructorExpressions() so it never leaks into | ||
| * __clone's entry scope. | ||
| */ | ||
| final class CloneReinitializationExpr extends Expr implements VirtualNode | ||
| { | ||
|
|
||
| public function __construct(private string $propertyName) | ||
| { | ||
| parent::__construct([]); | ||
| } | ||
|
|
||
| public function getPropertyName(): string | ||
| { | ||
| return $this->propertyName; | ||
| } | ||
|
|
||
| #[Override] | ||
| public function getType(): string | ||
| { | ||
| return 'PHPStan_Node_CloneReinitializationExpr'; | ||
| } | ||
|
|
||
| /** | ||
| * @return string[] | ||
| */ | ||
| #[Override] | ||
| public function getSubNodeNames(): array | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| use PhpParser\Node; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\DependencyInjection\RegisteredRule; | ||
| use PHPStan\Node\Expr\CloneReinitializationExpr; | ||
| use PHPStan\Node\Expr\SetOffsetValueTypeExpr; | ||
| use PHPStan\Node\Expr\UnsetOffsetExpr; | ||
| use PHPStan\Node\PropertyAssignNode; | ||
|
|
@@ -96,15 +97,26 @@ public function processNode(Node $node, Scope $scope): array | |
| throw new ShouldNotHappenException(); | ||
| } | ||
|
|
||
| $methodName = $scopeMethod->getName(); | ||
| $inClone = $this->phpVersion->supportsReadonlyPropertyReinitializationOnClone() && strtolower($methodName) === '__clone'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the fact that CloneReinitializationExpr is only added if supportsReadonlyPropertyReinitializationOnClone is true, we might don't need to check it here again ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems check is still needed for the 1st occurence, which does not react on |
||
| if ( | ||
| in_array($scopeMethod->getName(), $this->constructorsHelper->getConstructors($scopeClassReflection), true) | ||
| || strtolower($scopeMethod->getName()) === '__unserialize' | ||
| in_array($methodName, $this->constructorsHelper->getConstructors($scopeClassReflection), true) | ||
| || strtolower($methodName) === '__unserialize' | ||
| || $inClone | ||
| ) { | ||
| if (TypeUtils::findThisType($scope->getType($propertyFetch->var)) === null) { | ||
| $errors[] = RuleErrorBuilder::message(sprintf('Readonly property %s::$%s is not assigned on $this.', $declaringClass->getDisplayName(), $propertyReflection->getName())) | ||
| ->line($propertyFetch->name->getStartLine()) | ||
| ->identifier('property.readOnlyAssignNotOnThis') | ||
| ->build(); | ||
| } elseif ( | ||
| $inClone | ||
| && !$scope->hasExpressionType(new CloneReinitializationExpr($propertyReflection->getName()))->no() | ||
| ) { | ||
| $errors[] = RuleErrorBuilder::message(sprintf('Readonly property %s::$%s is already assigned.', $declaringClass->getDisplayName(), $propertyReflection->getName())) | ||
| ->line($propertyFetch->name->getStartLine()) | ||
| ->identifier('assign.readOnlyProperty') | ||
| ->build(); | ||
| } | ||
|
|
||
| continue; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| <?php // lint >= 8.1 | ||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug11495; | ||
|
|
||
| class HelloWorld | ||
| { | ||
| private readonly string $foo; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->foo = 'bar'; | ||
| } | ||
|
|
||
| public function __clone() | ||
| { | ||
| $this->foo = 'baz'; | ||
|
|
||
| $s = new self(); | ||
| $s->foo = 'baz'; | ||
| } | ||
|
|
||
| public function getFoo(): string | ||
| { | ||
| return $this->foo; | ||
| } | ||
| } | ||
|
|
||
| class DoubleAssign | ||
| { | ||
| private readonly string $foo; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->foo = 'bar'; | ||
| } | ||
|
|
||
| public function __clone() | ||
| { | ||
| $this->foo = 'baz'; | ||
| $this->foo = 'qux'; | ||
| } | ||
| } | ||
|
|
||
| class BranchedAssign | ||
| { | ||
| private readonly string $foo; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->foo = 'bar'; | ||
| } | ||
|
|
||
| public function __clone() | ||
| { | ||
| if (rand(0, 1)) { | ||
| $this->foo = 'a'; | ||
| } else { | ||
| $this->foo = 'b'; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class ConditionalThenAssign | ||
| { | ||
| private readonly string $foo; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->foo = 'bar'; | ||
| } | ||
|
|
||
| public function __clone() | ||
| { | ||
| if (rand(0, 1)) { | ||
| $this->foo = 'a'; | ||
| } | ||
| $this->foo = 'b'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
instanceof MethodReflectionreally needed given the fact there is a previous$this->isInClass()check and you're checking the method name ?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$function !== nulledit: Ohh I see now why it is done like that. we don't want to react on regular functions named
__cloneand we did similar checks across the codebase.so its fine to me.