diff --git a/src/Analyser/ExprHandler/AssignHandler.php b/src/Analyser/ExprHandler/AssignHandler.php index 51995ac6657..fa2986274d2 100644 --- a/src/Analyser/ExprHandler/AssignHandler.php +++ b/src/Analyser/ExprHandler/AssignHandler.php @@ -39,6 +39,7 @@ use PHPStan\Node\Expr\GetOffsetValueTypeExpr; use PHPStan\Node\Expr\IntertwinedVariableByReferenceWithExpr; use PHPStan\Node\Expr\OriginalPropertyTypeExpr; +use PHPStan\Node\Expr\PropertyInitializationExpr; use PHPStan\Node\Expr\SetExistingOffsetValueTypeExpr; use PHPStan\Node\Expr\SetOffsetValueTypeExpr; use PHPStan\Node\Expr\TypeExpr; @@ -73,6 +74,8 @@ use function in_array; use function is_int; use function is_string; +use function sprintf; +use function strtolower; /** * @implements ExprHandler @@ -611,6 +614,26 @@ public function processAssignVar( } if ($this->phpVersion->supportsPropertyHooks()) { $throwPoints = array_merge($throwPoints, $nodeScopeResolver->getThrowPointsFromPropertyHook($scope, $var, $nativeProperty, 'set')); + if ( + $nativeProperty->hasHook('set') + && $scope->isInClass() + && !$scope->isInAnonymousFunction() + && $scope->getFunctionName() !== null + && strtolower($scope->getFunctionName()) === '__construct' + && TypeUtils::findThisType($propertyHolderType) !== null + ) { + $hookStackName = sprintf('%s::$%s::set', $declaringClass->getName(), $propertyName); + $uninitializedProperties = []; + foreach ($scope->getClassReflection()->getNativeReflection()->getProperties() as $refProperty) { + if ($refProperty->hasDefaultValue() || $refProperty->isStatic()) { + continue; + } + if (!$scope->hasExpressionType(new PropertyInitializationExpr($refProperty->getName()))->yes()) { + $uninitializedProperties[$refProperty->getName()] = true; + } + } + $nodeScopeResolver->registerCalledMethodUninitializedProperties($hookStackName, $uninitializedProperties); + } } if ($enterExpressionAssign) { $scope = $scope->assignInitializedProperty($propertyHolderType, $propertyName); diff --git a/src/Analyser/ExprHandler/MethodCallHandler.php b/src/Analyser/ExprHandler/MethodCallHandler.php index 28e0181c331..ffef401791d 100644 --- a/src/Analyser/ExprHandler/MethodCallHandler.php +++ b/src/Analyser/ExprHandler/MethodCallHandler.php @@ -22,6 +22,7 @@ use PHPStan\Analyser\NodeScopeResolver; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Node\Expr\PropertyInitializationExpr; use PHPStan\Node\Expr\PossiblyImpureCallExpr; use PHPStan\Node\InvalidateExprNode; use PHPStan\Reflection\Callables\SimpleImpurePoint; @@ -204,10 +205,23 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex } if ( $scope->isInClass() + && !$scope->isInAnonymousFunction() && $scope->getClassReflection()->getName() === $methodReflection->getDeclaringClass()->getName() && ($scope->getFunctionName() !== null && strtolower($scope->getFunctionName()) === '__construct') && TypeUtils::findThisType($calledOnType) !== null ) { + $stackName = sprintf('%s::%s', $methodReflection->getDeclaringClass()->getName(), $methodReflection->getName()); + $uninitializedProperties = []; + foreach ($scope->getClassReflection()->getNativeReflection()->getProperties() as $nativeProperty) { + if ($nativeProperty->hasDefaultValue() || $nativeProperty->isStatic()) { + continue; + } + if (!$scope->hasExpressionType(new PropertyInitializationExpr($nativeProperty->getName()))->yes()) { + $uninitializedProperties[$nativeProperty->getName()] = true; + } + } + $nodeScopeResolver->registerCalledMethodUninitializedProperties($stackName, $uninitializedProperties); + $calledMethodScope = $nodeScopeResolver->processCalledMethod($methodReflection); if ($calledMethodScope !== null) { $scope = $scope->mergeInitializedProperties($calledMethodScope); diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 001c8b5b368..bad7bff4908 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -199,6 +199,9 @@ class NodeScopeResolver /** @var array */ private array $calledMethodResults = []; + /** @var array> */ + private array $calledMethodUninitializedProperties = []; + /** * @param string[][] $earlyTerminatingMethodCalls className(string) => methods(string[]) * @param array $earlyTerminatingFunctionCalls @@ -751,6 +754,13 @@ public function processStmtNode( ); $methodScope = $methodScope->assignExpression(new PropertyInitializationExpr($param->var->name), new MixedType(), new MixedType()); } + } elseif (!$stmt->isStatic()) { + $stackName = sprintf('%s::%s', $classReflection->getName(), $stmt->name->toString()); + if (array_key_exists($stackName, $this->calledMethodUninitializedProperties)) { + foreach ($this->calledMethodUninitializedProperties[$stackName] as $propertyName => $_) { + $methodScope = $methodScope->invalidateExpression(new PropertyInitializationExpr($propertyName)); + } + } } if ($stmt->getAttribute('virtual', false) === false) { @@ -1017,6 +1027,7 @@ public function processStmtNode( $this->callNodeCallback($nodeCallback, new ClassConstantsNode($stmt, $classStatementsGatherer->getConstants(), $classStatementsGatherer->getConstantFetches(), $classReflection), $classScope, $storage); $classReflection->evictPrivateSymbols(); $this->calledMethodResults = []; + $this->calledMethodUninitializedProperties = []; } elseif ($stmt instanceof Node\Stmt\Property) { $hasYield = false; $throwPoints = []; @@ -3085,6 +3096,24 @@ private function processPropertyHooks( $phpDocComment, $resolvedPhpDoc, ); + + $hookName = $hook->name->toLowerString(); + if ($hookName === 'set') { + $hookStackName = sprintf('%s::$%s::set', $classReflection->getName(), $propertyName); + if (array_key_exists($hookStackName, $this->calledMethodUninitializedProperties)) { + foreach ($this->calledMethodUninitializedProperties[$hookStackName] as $propName => $_) { + $hookScope = $hookScope->invalidateExpression(new PropertyInitializationExpr($propName)); + } + } else { + foreach ($classReflection->getNativeReflection()->getProperties() as $nativeProperty) { + if ($nativeProperty->hasDefaultValue() || $nativeProperty->isStatic()) { + continue; + } + $hookScope = $hookScope->invalidateExpression(new PropertyInitializationExpr($nativeProperty->getName())); + } + } + } + $hookReflection = $hookScope->getFunction(); if (!$hookReflection instanceof PhpMethodFromParserNodeReflection) { throw new ShouldNotHappenException(); @@ -4045,6 +4074,19 @@ private function processNodesForTraitUse($node, ClassReflection $traitReflection } } + /** + * @param array $uninitializedProperties + */ + public function registerCalledMethodUninitializedProperties(string $stackName, array $uninitializedProperties): void + { + if (!array_key_exists($stackName, $this->calledMethodUninitializedProperties)) { + $this->calledMethodUninitializedProperties[$stackName] = $uninitializedProperties; + return; + } + + $this->calledMethodUninitializedProperties[$stackName] = $this->calledMethodUninitializedProperties[$stackName] + $uninitializedProperties; + } + public function processCalledMethod(MethodReflection $methodReflection): ?MutatingScope { $declaringClass = $methodReflection->getDeclaringClass(); diff --git a/tests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php b/tests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php index 2fc14f808da..e8269e5c3eb 100644 --- a/tests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php +++ b/tests/PHPStan/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php @@ -137,14 +137,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, - ],*/ + ], [ 'Class MissingReadOnlyPropertyAssign\PropertyAssignedOnDifferentObjectUninitialized has an uninitialized readonly property $foo. Assign it in the constructor.', 264, diff --git a/tests/PHPStan/Rules/Variables/IssetRuleTest.php b/tests/PHPStan/Rules/Variables/IssetRuleTest.php index fd841e49b16..e690cdec768 100644 --- a/tests/PHPStan/Rules/Variables/IssetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/IssetRuleTest.php @@ -526,6 +526,51 @@ public function testBug9503(): void $this->analyse([__DIR__ . '/data/bug-9503.php'], []); } + #[RequiresPhp('>= 8.4')] + public function testBug13473(): void + { + $this->treatPhpDocTypesAsCertain = true; + + $this->analyse([__DIR__ . '/data/bug-13473.php'], [ + [ + 'Property Bug13473\Bar::$bar in isset() is not nullable nor uninitialized.', + 30, + ], + [ + 'Property Bug13473\Baz::$foo (int) in isset() is not nullable.', + 67, + ], + [ + 'Property Bug13473\PropertyInitializedBeforeHookedAssignment::$foo in isset() is not nullable nor uninitialized.', + 88, + ], + ]); + } + + public function testIssetMethodCalledFromConstructor(): void + { + $this->treatPhpDocTypesAsCertain = true; + + $this->analyse([__DIR__ . '/data/isset-method-called-from-constructor.php'], [ + [ + 'Property IssetMethodCalledFromConstructor\MethodCalledFromConstructorWithDefault::$bar in isset() is not nullable nor uninitialized.', + 34, + ], + [ + 'Property IssetMethodCalledFromConstructor\MethodNotCalledFromConstructor::$bar in isset() is not nullable nor uninitialized.', + 51, + ], + [ + 'Property IssetMethodCalledFromConstructor\MultipleProperties::$bar in isset() is not nullable nor uninitialized.', + 72, + ], + [ + 'Property IssetMethodCalledFromConstructor\PropertyInitializedBeforeMethodCall::$foo in isset() is not nullable nor uninitialized.', + 91, + ], + ]); + } + public function testBug14393(): void { $this->treatPhpDocTypesAsCertain = true; diff --git a/tests/PHPStan/Rules/Variables/data/bug-13473.php b/tests/PHPStan/Rules/Variables/data/bug-13473.php new file mode 100644 index 00000000000..2a028894c38 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-13473.php @@ -0,0 +1,100 @@ += 8.4 + +declare(strict_types = 1); + +namespace Bug13473; + +class Foo { + private(set) int $bar { + get => $this->bar; + set(int $bar) { + if (isset($this->bar)) { + throw new \Exception('bar is set'); + } + $this->bar = $bar; + } + } + + public function __construct(int $bar) + { + $this->bar = $bar; + } +} + +$foo = new Foo(10); + +class Bar { + private(set) int $bar = 1 { + get => $this->bar; + set(int $bar) { + if (isset($this->bar)) { + throw new \Exception('bar is set'); + } + $this->bar = $bar; + } + } + + public function __construct(int $bar) + { + $this->bar = $bar; + } +} + +class Qux { + private(set) int $foo; + private(set) int $bar { + get => $this->bar; + set(int $bar) { + if (isset($this->foo)) { // $foo has no default, could be uninitialized - no error + throw new \Exception('foo is set'); + } + $this->bar = $bar; + } + } + + public function __construct(int $bar) + { + $this->bar = $bar; + $this->foo = 42; + } +} + +class Baz { + private(set) int $foo = 5; + private(set) int $bar { + get => $this->bar; + set(int $bar) { + if (isset($this->foo)) { // $foo has default value, always initialized - should error + echo 'foo is set'; + } + if (isset($this->bar)) { // $bar has no default, could be uninitialized - no error + throw new \Exception('bar is set'); + } + $this->bar = $bar; + } + } + + public function __construct(int $bar) + { + $this->bar = $bar; + } +} + +class PropertyInitializedBeforeHookedAssignment { + private(set) int $foo; + private(set) int $bar { + get => $this->bar; + set(int $bar) { + if (isset($this->foo)) { // $foo was initialized before $this->bar = ... in constructor - should error + echo 'foo is set'; + } + $this->bar = $bar; + } + } + + public function __construct(int $bar) + { + $this->foo = 42; + $this->bar = $bar; + } +} diff --git a/tests/PHPStan/Rules/Variables/data/isset-method-called-from-constructor.php b/tests/PHPStan/Rules/Variables/data/isset-method-called-from-constructor.php new file mode 100644 index 00000000000..5b76becf6ac --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/isset-method-called-from-constructor.php @@ -0,0 +1,99 @@ +setBar($bar); + } + + private function setBar(int $bar): void + { + if (isset($this->bar)) { // $bar has no default, could be uninitialized when called from constructor - no error + throw new \Exception('bar is set'); + } + $this->bar = $bar; + } +} + +final class MethodCalledFromConstructorWithDefault { + private int $bar = 1; + + public function __construct(int $bar) + { + $this->setBar($bar); + } + + private function setBar(int $bar): void + { + if (isset($this->bar)) { // $bar has default value, always initialized - should error + throw new \Exception('bar is set'); + } + $this->bar = $bar; + } +} + +final class MethodNotCalledFromConstructor { + private int $bar; + + public function __construct(int $bar) + { + $this->bar = $bar; + } + + private function checkBar(): void + { + if (isset($this->bar)) { // Not called from constructor, property is initialized after construction - should error + echo 'bar is set'; + } + } +} + +final class MultipleProperties { + private int $foo; + private int $bar = 5; + + public function __construct(int $bar) + { + $this->init($bar); + $this->foo = 42; + } + + private function init(int $bar): void + { + if (isset($this->foo)) { // $foo has no default, could be uninitialized - no error + throw new \Exception('foo is set'); + } + if (isset($this->bar)) { // $bar has default value, always initialized - should error + echo 'bar is set'; + } + $this->bar = $bar; + } +} + +final class PropertyInitializedBeforeMethodCall { + private int $foo; + private int $bar; + + public function __construct() + { + $this->foo = 1; + $this->setBar(); + } + + private function setBar(): void + { + if (isset($this->foo)) { // $foo was initialized before the call - should error + echo 'foo is set'; + } + if (isset($this->bar)) { // $bar has no default, not yet initialized - no error + echo 'bar is set'; + } + $this->bar = 2; + } +}