Skip to content

Commit

Permalink
Fix false positives about uninitialized properties
Browse files Browse the repository at this point in the history
  • Loading branch information
ondrejmirtes committed Jul 3, 2023
1 parent 8fe4401 commit 348debc
Show file tree
Hide file tree
Showing 6 changed files with 178 additions and 17 deletions.
60 changes: 46 additions & 14 deletions src/Node/ClassPropertiesNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
use PHPStan\Rules\Properties\ReadWritePropertiesExtension;
use PHPStan\Rules\Properties\ReadWritePropertiesExtensionProvider;
use PHPStan\ShouldNotHappenException;
use PHPStan\TrinaryLogic;
use function array_key_exists;
use function array_keys;
use function count;
use function in_array;

/** @api */
Expand Down Expand Up @@ -100,6 +100,8 @@ public function getUninitializedProperties(

$properties = [];
$originalProperties = [];
$initialInitializedProperties = [];
$initializedProperties = [];
foreach ($this->getProperties() as $property) {
if ($property->isStatic()) {
continue;
Expand All @@ -111,6 +113,11 @@ public function getUninitializedProperties(
continue;
}
$originalProperties[$property->getName()] = $property;
$is = TrinaryLogic::createFromBoolean($property->isPromoted());
$initialInitializedProperties[$property->getName()] = $is;
foreach ($constructors as $constructor) {
$initializedProperties[$constructor][$property->getName()] = $is;
}
if ($property->isPromoted()) {
continue;
}
Expand Down Expand Up @@ -138,7 +145,8 @@ public function getUninitializedProperties(
if ($constructors === []) {
return [$properties, [], []];
}
$methodsCalledFromConstructor = $this->getMethodsCalledFromConstructor($classReflection, $this->methodCalls, $constructors);

$methodsCalledFromConstructor = $this->getMethodsCalledFromConstructor($classReflection, $this->methodCalls, $initialInitializedProperties, $initializedProperties, $constructors);
$prematureAccess = [];
$additionalAssigns = [];

Expand All @@ -158,10 +166,12 @@ public function getUninitializedProperties(
if ($function->getDeclaringClass()->getName() !== $classReflection->getName()) {
continue;
}
if (!in_array($function->getName(), $methodsCalledFromConstructor, true)) {
if (!array_key_exists($function->getName(), $methodsCalledFromConstructor)) {
continue;
}

$initializedPropertiesMap = $methodsCalledFromConstructor[$function->getName()];

if (!$fetch->name instanceof Identifier) {
continue;
}
Expand All @@ -181,18 +191,18 @@ public function getUninitializedProperties(
unset($properties[$propertyName]);
}

if (array_key_exists($propertyName, $originalProperties)) {
$hasInitialization = $usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName));
if (!$hasInitialization->no()) {
if (array_key_exists($propertyName, $initializedPropertiesMap)) {
$hasInitialization = $initializedPropertiesMap[$propertyName]->or($usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName)));
if (!$hasInitialization->no() && !$usage->isPromotedPropertyWrite()) {
$additionalAssigns[] = [
$propertyName,
$fetch->getLine(),
$originalProperties[$propertyName],
];
}
}
} elseif (array_key_exists($propertyName, $originalProperties)) {
$hasInitialization = $usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName));
} elseif (array_key_exists($propertyName, $initializedPropertiesMap)) {
$hasInitialization = $initializedPropertiesMap[$propertyName]->or($usageScope->hasExpressionType(new PropertyInitializationExpr($propertyName)));
if (!$hasInitialization->yes()) {
$prematureAccess[] = [
$propertyName,
Expand All @@ -213,15 +223,20 @@ public function getUninitializedProperties(
/**
* @param MethodCall[] $methodCalls
* @param string[] $methods
* @return string[]
* @param array<string, TrinaryLogic> $initialInitializedProperties
* @param array<string, array<string, TrinaryLogic>> $initializedProperties
* @return array<string, array<string, TrinaryLogic>>
*/
private function getMethodsCalledFromConstructor(
ClassReflection $classReflection,
array $methodCalls,
array $initialInitializedProperties,
array $initializedProperties,
array $methods,
): array
{
$originalCount = count($methods);
$originalMap = $initializedProperties;
$originalMethods = $methods;
foreach ($methodCalls as $methodCall) {
$methodCallNode = $methodCall->getNode();
if ($methodCallNode instanceof Array_) {
Expand All @@ -242,7 +257,10 @@ private function getMethodsCalledFromConstructor(
}

$methodName = $methodCallNode->name->toString();
if (in_array($methodName, $methods, true)) {
if (array_key_exists($methodName, $initializedProperties)) {
foreach ($this->getInitializedProperties($callScope, $initialInitializedProperties) as $propertyName => $isInitialized) {
$initializedProperties[$methodName][$propertyName] = $initializedProperties[$methodName][$propertyName]->and($isInitialized);
}
continue;
}
$methodReflection = $callScope->getMethodReflection($calledOnType, $methodName);
Expand All @@ -259,14 +277,28 @@ private function getMethodsCalledFromConstructor(
if (!in_array($inMethod->getName(), $methods, true)) {
continue;
}
$initializedProperties[$methodName] = $this->getInitializedProperties($callScope, $initialInitializedProperties);
$methods[] = $methodName;
}

if ($originalCount === count($methods)) {
return $methods;
if ($originalMap === $initializedProperties && $originalMethods === $methods) {
return $initializedProperties;
}

return $this->getMethodsCalledFromConstructor($classReflection, $methodCalls, $initialInitializedProperties, $initializedProperties, $methods);
}

/**
* @param array<string, TrinaryLogic> $initialInitializedProperties
* @return array<string, TrinaryLogic>
*/
private function getInitializedProperties(Scope $scope, array $initialInitializedProperties): array
{
foreach ($initialInitializedProperties as $propertyName => $isInitialized) {
$initialInitializedProperties[$propertyName] = $isInitialized->or($scope->hasExpressionType(new PropertyInitializationExpr($propertyName)));
}

return $this->getMethodsCalledFromConstructor($classReflection, $methodCalls, $methods);
return $initialInitializedProperties;
}

}
5 changes: 3 additions & 2 deletions src/Node/ClassStatementsGatherer.php
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ private function gatherNodes(Node $node, Scope $scope): void
$this->propertyUsages[] = new PropertyWrite(
new PropertyFetch(new Expr\Variable('this'), new Identifier($node->getName())),
$scope,
true,
);
}
return;
Expand Down Expand Up @@ -167,7 +168,7 @@ private function gatherNodes(Node $node, Scope $scope): void
return;
}
if ($node instanceof PropertyAssignNode) {
$this->propertyUsages[] = new PropertyWrite($node->getPropertyFetch(), $scope);
$this->propertyUsages[] = new PropertyWrite($node->getPropertyFetch(), $scope, false);
return;
}
if (!$node instanceof Expr) {
Expand All @@ -184,7 +185,7 @@ private function gatherNodes(Node $node, Scope $scope): void
}

$this->propertyUsages[] = new PropertyRead($node->expr, $scope);
$this->propertyUsages[] = new PropertyWrite($node->expr, $scope);
$this->propertyUsages[] = new PropertyWrite($node->expr, $scope, false);
return;
}
if ($node instanceof Node\Scalar\EncapsedStringPart) {
Expand Down
7 changes: 6 additions & 1 deletion src/Node/Property/PropertyWrite.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
class PropertyWrite
{

public function __construct(private PropertyFetch|StaticPropertyFetch $fetch, private Scope $scope)
public function __construct(private PropertyFetch|StaticPropertyFetch $fetch, private Scope $scope, private bool $promotedPropertyWrite)
{
}

Expand All @@ -27,4 +27,9 @@ public function getScope(): Scope
return $this->scope;
}

public function isPromotedPropertyWrite(): bool
{
return $this->promotedPropertyWrite;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,14 @@ public function testRule(): void
'Readonly property MissingReadOnlyPropertyAssign\AdditionalAssignOfReadonlyPromotedProperty::$x is already assigned.',
188,
],
[
'Access to an uninitialized readonly property MissingReadOnlyPropertyAssign\MethodCalledFromConstructorBeforeAssign::$foo.',
226,
],
[
'Access to an uninitialized readonly property MissingReadOnlyPropertyAssign\MethodCalledTwice::$foo.',
244,
],
]);
}

Expand Down Expand Up @@ -174,4 +182,13 @@ public function testBug6402(): void
]);
}

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

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

}
51 changes: 51 additions & 0 deletions tests/PHPStan/Rules/Properties/data/bug-7198.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

namespace Bug7198;

trait TestTrait {
public function foo(): void
{
$this->callee->foo();
}
}

class TestCallee {
public function foo(): void
{
echo "FOO\n";
}
}

class TestCaller {
use TestTrait;

public function __construct(private readonly TestCallee $callee)
{
$this->foo();
}
}

class TestCaller2 {
public function foo(): void
{
$this->callee->foo();
}

public function __construct(private readonly TestCallee $callee)
{
$this->foo();
}
}

class TestCaller3 {

public function __construct(private readonly TestCallee $callee)
{
$this->foo();
}

public function foo(): void
{
$this->callee->foo();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -189,3 +189,58 @@ public function __construct(private readonly int $x)
}

}

class MethodCalledFromConstructorAfterAssign
{


private readonly int $foo;

public function __construct()
{
$this->foo = 1;
$this->doFoo();
}

public function doFoo(): void
{
echo $this->foo;
}

}

class MethodCalledFromConstructorBeforeAssign
{


private readonly int $foo;

public function __construct()
{
$this->doFoo();
$this->foo = 1;
}

public function doFoo(): void
{
echo $this->foo;
}

}

class MethodCalledTwice
{
private readonly int $foo;

public function __construct()
{
$this->doFoo();
$this->foo = 1;
$this->doFoo();
}

public function doFoo(): void
{
echo $this->foo;
}
}

0 comments on commit 348debc

Please sign in to comment.