Skip to content

Allow reinitialization of a readonly property in __clone since PHP8.3 #3330

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Allow reinitialization of a readonly property in __clone since PHP8.3 #3330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fc2d5b
Allow reinitialization of a readonly property in `__clone` since PHP8.3
takaram Aug 17, 2024
9dbcae6
Add more test for readonly property assignment
takaram Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Php/PhpVersion.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,4 +327,9 @@ public function hasDateTimeExceptions(): bool
return $this->versionId >= 80300;
}

public function supportsReadonlyPropertyReinitializationOnClone(): bool
{
return $this->versionId >= 80300;
}

}
8 changes: 6 additions & 2 deletions src/Rules/Properties/ReadOnlyPropertyAssignRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Node\PropertyAssignNode;
use PHPStan\Php\PhpVersion;
use PHPStan\Reflection\ConstructorsHelper;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Rules\Rule;
Expand All @@ -24,6 +25,7 @@ class ReadOnlyPropertyAssignRule implements Rule
public function __construct(
private PropertyReflectionFinder $propertyReflectionFinder,
private ConstructorsHelper $constructorsHelper,
private PhpVersion $phpVersion,
)
{
}
Expand Down Expand Up @@ -76,9 +78,11 @@ public function processNode(Node $node, Scope $scope): array
throw new ShouldNotHappenException();
}

$methodName = $scopeMethod->getName();
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'
|| ($this->phpVersion->supportsReadonlyPropertyReinitializationOnClone() && strtolower($methodName) === '__clone')
) {
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()))
Expand Down
31 changes: 31 additions & 0 deletions tests/PHPStan/Rules/Properties/ReadOnlyPropertyAssignRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace PHPStan\Rules\Properties;

use PHPStan\Php\PhpVersion;
use PHPStan\Reflection\ConstructorsHelper;
use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;
Expand All @@ -23,6 +24,7 @@ protected function getRule(): Rule
'ReadonlyPropertyAssign\\TestCase::setUp',
],
),
new PhpVersion(PHP_VERSION_ID),
);
}

Expand Down Expand Up @@ -154,4 +156,33 @@ public function testBug6773(): void
]);
}

public function testBug11495(): void
{
if (PHP_VERSION_ID < 80100) {
$this->markTestSkipped('Test requires PHP 8.1.');
}

if (PHP_VERSION_ID < 80300) {
$errors = [
[
'Readonly property Bug11495\HelloWorld::$foo is assigned outside of the constructor.',
17,
],
[
'Readonly property Bug11495\HelloWorld::$foo is assigned outside of the constructor.',
20,
],
];
} else {
$errors = [
[
'Readonly property Bug11495\HelloWorld::$foo is not assigned on $this.',
20,
],
];
}

$this->analyse([__DIR__ . '/data/bug-11495.php'], $errors);
}

}
27 changes: 27 additions & 0 deletions tests/PHPStan/Rules/Properties/data/bug-11495.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have more tests here:

  1. To verify double assignment is still detected as error (https://3v4l.org/oaLm9)
  2. To verify assignment to something else than $this is detected as error:
	public function __clone()
	{
		$s = new self();
		$s->foo = 'baz';
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for 2.

For 1., no error is reported on double assignment. This looks not so easy to implement.
I'll take a deeper look...


$s = new self();
$s->foo = 'baz';
}

public function getFoo(): string
{
return $this->foo;
}
}
Loading