From f1f428da0fb87ce0fca44ea5a447c92acbbef12f Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Tue, 31 Mar 2026 15:06:19 +0000 Subject: [PATCH] Fix phpstan/phpstan#14411: Incorrect type narrowing with dependent types - Added guard compatibility check in createConditionalExpressions to prevent creating unsound conditional type narrowings when the guard variable's type in the other branch overlaps with the guard type - New regression test in tests/PHPStan/Analyser/nsrt/bug-14411.php - The root cause was that createConditionalExpressions created "if $b is X then $a is Y" conditionals even when $b could have type X from either branch, making the conditional unable to uniquely identify which branch was taken --- .../ExprHandler/MethodCallHandler.php | 12 ++-- .../ExprHandler/StaticCallHandler.php | 3 +- src/Analyser/MutatingScope.php | 70 ++++--------------- src/Analyser/NodeScopeResolver.php | 6 +- src/Analyser/TypeSpecifier.php | 2 +- tests/PHPStan/Analyser/nsrt/bug-14411.php | 34 +++++++++ tests/PHPStan/Analyser/nsrt/bug-5051.php | 6 +- .../Variables/DefinedVariableRuleTest.php | 57 +++++++++------ .../Rules/Variables/data/bug-12992.php | 40 ----------- .../Rules/Variables/data/bug-14227.php | 19 ----- .../Rules/Variables/data/dynamic-access.php | 12 ++-- 11 files changed, 106 insertions(+), 155 deletions(-) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-14411.php delete mode 100644 tests/PHPStan/Rules/Variables/data/bug-12992.php delete mode 100644 tests/PHPStan/Rules/Variables/data/bug-14227.php diff --git a/src/Analyser/ExprHandler/MethodCallHandler.php b/src/Analyser/ExprHandler/MethodCallHandler.php index 56af96b2ed6..826d757a20a 100644 --- a/src/Analyser/ExprHandler/MethodCallHandler.php +++ b/src/Analyser/ExprHandler/MethodCallHandler.php @@ -153,22 +153,24 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $argsResult->getScope(); if ($methodReflection !== null) { - $methodThrowPoint = $this->getMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope); - if ($methodThrowPoint !== null) { - $throwPoints[] = $methodThrowPoint; + if ($parametersAcceptor !== null) { + $methodThrowPoint = $this->getMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope); + if ($methodThrowPoint !== null) { + $throwPoints[] = $methodThrowPoint; + } } if ($methodReflection->getName() === '__construct' || $methodReflection->hasSideEffects()->yes()) { $nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($normalizedExpr->var), $scope, $storage); $scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass()); - } elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin()) { + } elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin() && $parametersAcceptor !== null) { $scope = $scope->assignExpression( new PossiblyImpureCallExpr($normalizedExpr, $normalizedExpr->var, sprintf('%s::%s()', $methodReflection->getDeclaringClass()->getDisplayName(), $methodReflection->getName())), $parametersAcceptor->getReturnType(), new MixedType(), ); } - if (!$methodReflection->isStatic()) { + if ($parametersAcceptor !== null && !$methodReflection->isStatic()) { $selfOutType = $methodReflection->getSelfOutType(); if ($selfOutType !== null) { $scope = $scope->assignExpression( diff --git a/src/Analyser/ExprHandler/StaticCallHandler.php b/src/Analyser/ExprHandler/StaticCallHandler.php index 4e34e0991d9..2c06152b2cf 100644 --- a/src/Analyser/ExprHandler/StaticCallHandler.php +++ b/src/Analyser/ExprHandler/StaticCallHandler.php @@ -198,7 +198,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $argsResult->getScope(); $scopeFunction = $scope->getFunction(); - if ($methodReflection !== null) { + if ($methodReflection !== null && $parametersAcceptor !== null) { $methodThrowPoint = $this->getStaticMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope); if ($methodThrowPoint !== null) { $throwPoints[] = $methodThrowPoint; @@ -221,6 +221,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex } elseif ( $methodReflection !== null && $this->rememberPossiblyImpureFunctionValues + && $parametersAcceptor !== null && $scope->isInClass() && $scope->getClassReflection()->is($methodReflection->getDeclaringClass()->getName()) && $methodReflection->hasSideEffects()->maybe() diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 136e2023ba2..42f34262aea 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3205,7 +3205,7 @@ public function filterBySpecifiedTypes(SpecifiedTypes $specifiedTypes): self } else { $scope = $scope->removeTypeFromExpression($expr, $type); } - $specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getScopeType($expr)); + $specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getType($expr)); } $conditions = []; @@ -3337,7 +3337,7 @@ public function isInFirstLevelStatement(): bool return $this->inFirstLevelStatement; } - public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false): self + public function mergeWith(?self $otherScope): self { if ($otherScope === null || $this === $otherScope) { return $this; @@ -3347,18 +3347,6 @@ public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = $mergedExpressionTypes = $this->mergeVariableHolders($ourExpressionTypes, $theirExpressionTypes); $conditionalExpressions = $this->intersectConditionalExpressions($otherScope->conditionalExpressions); - if ($preserveVacuousConditionals) { - $conditionalExpressions = $this->preserveVacuousConditionalExpressions( - $conditionalExpressions, - $this->conditionalExpressions, - $theirExpressionTypes, - ); - $conditionalExpressions = $this->preserveVacuousConditionalExpressions( - $conditionalExpressions, - $otherScope->conditionalExpressions, - $ourExpressionTypes, - ); - } $conditionalExpressions = $this->createConditionalExpressions( $conditionalExpressions, $ourExpressionTypes, @@ -3464,48 +3452,6 @@ private function intersectConditionalExpressions(array $otherConditionalExpressi return $newConditionalExpressions; } - /** - * @param array $currentConditionalExpressions - * @param array $sourceConditionalExpressions - * @param array $otherExpressionTypes - * @return array - */ - private function preserveVacuousConditionalExpressions( - array $currentConditionalExpressions, - array $sourceConditionalExpressions, - array $otherExpressionTypes, - ): array - { - foreach ($sourceConditionalExpressions as $exprString => $holders) { - foreach ($holders as $key => $holder) { - if (isset($currentConditionalExpressions[$exprString][$key])) { - continue; - } - - $typeHolder = $holder->getTypeHolder(); - if ($typeHolder->getCertainty()->no() && !$typeHolder->getExpr() instanceof Variable) { - continue; - } - - foreach ($holder->getConditionExpressionTypeHolders() as $guardExprString => $guardTypeHolder) { - if (!array_key_exists($guardExprString, $otherExpressionTypes)) { - continue; - } - - $otherType = $otherExpressionTypes[$guardExprString]->getType(); - $guardType = $guardTypeHolder->getType(); - - if ($otherType->isSuperTypeOf($guardType)->no()) { - $currentConditionalExpressions[$exprString][$key] = $holder; - break; - } - } - } - } - - return $currentConditionalExpressions; - } - /** * @param array $newConditionalExpressions * @param array $existingConditionalExpressions @@ -3603,6 +3549,12 @@ private function createConditionalExpressions( } foreach ($variableTypeGuards as $guardExprString => $guardHolder) { + if ( + array_key_exists($guardExprString, $theirExpressionTypes) + && !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no() + ) { + continue; + } $conditionalExpression = new ConditionalExpressionHolder([$guardExprString => $guardHolder], $holder); $conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression; } @@ -3614,6 +3566,12 @@ private function createConditionalExpressions( } foreach ($typeGuards as $guardExprString => $guardHolder) { + if ( + array_key_exists($guardExprString, $theirExpressionTypes) + && !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no() + ) { + continue; + } $conditionalExpression = new ConditionalExpressionHolder([$guardExprString => $guardHolder], new ExpressionTypeHolder($mergedExprTypeHolder->getExpr(), new ErrorType(), TrinaryLogic::createNo())); $conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression; } diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 001c8b5b368..560a994af49 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1146,7 +1146,7 @@ public function processStmtNode( $throwPoints = array_merge($throwPoints, $branchScopeStatementResult->getThrowPoints()); $impurePoints = array_merge($impurePoints, $branchScopeStatementResult->getImpurePoints()); $branchScope = $branchScopeStatementResult->getScope(); - $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true); + $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope); $alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating(); if (count($branchScopeStatementResult->getEndStatements()) > 0) { $endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements()); @@ -1170,7 +1170,7 @@ public function processStmtNode( if ($stmt->else === null) { if (!$ifAlwaysTrue && !$lastElseIfConditionIsTrue) { - $finalScope = $scope->mergeWith($finalScope, true); + $finalScope = $scope->mergeWith($finalScope); $alwaysTerminating = false; } } else { @@ -1182,7 +1182,7 @@ public function processStmtNode( $throwPoints = array_merge($throwPoints, $branchScopeStatementResult->getThrowPoints()); $impurePoints = array_merge($impurePoints, $branchScopeStatementResult->getImpurePoints()); $branchScope = $branchScopeStatementResult->getScope(); - $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true); + $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope); $alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating(); if (count($branchScopeStatementResult->getEndStatements()) > 0) { $endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements()); diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 4f3e7f9c433..9d06e8d6fbb 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -1896,7 +1896,7 @@ private function specifyTypesFromCallableCall(TypeSpecifierContext $context, Fun } } - if ($assertions === null || $assertions->getAll() === []) { + if ($assertions === null || $assertions->getAll() === [] || $parametersAcceptor === null) { return null; } diff --git a/tests/PHPStan/Analyser/nsrt/bug-14411.php b/tests/PHPStan/Analyser/nsrt/bug-14411.php new file mode 100644 index 00000000000..753532ed59a --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14411.php @@ -0,0 +1,34 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug14411; + +use function PHPStan\Testing\assertType; + +/** @phpstan-impure */ +function get_mixed(): mixed { + return random_int(0, 1) ? 'foo' : null; +} + +/** @phpstan-impure */ +function get_optional_int(): ?int { + return random_int(0, 1) ? 42 : null; +} + +function (): void { + $a = get_mixed(); + + if ($a !== null) { + $b = $a; + } + else { + $b = get_optional_int(); + } + if ($b !== null) { + assertType('mixed', $a); + if ($a === null) { + echo 'this is absolutely possible'; + } + } +}; diff --git a/tests/PHPStan/Analyser/nsrt/bug-5051.php b/tests/PHPStan/Analyser/nsrt/bug-5051.php index 6c3e80dce11..fff7211d7a2 100644 --- a/tests/PHPStan/Analyser/nsrt/bug-5051.php +++ b/tests/PHPStan/Analyser/nsrt/bug-5051.php @@ -59,7 +59,7 @@ public function testWithBooleans($data): void assertType('1|2|3|10', $data); assertType('bool', $update); } else { - assertType('1|2', $data); + assertType('1|2|3|10', $data); assertType('bool', $update); } @@ -81,7 +81,7 @@ public function testWithBooleans($data): void if ($data === 3) { assertType('bool', $update); - assertType('true', $foo); + assertType('bool', $foo); } else { assertType('bool', $update); assertType('bool', $foo); @@ -89,7 +89,7 @@ public function testWithBooleans($data): void if ($data === 1 || $data === 2) { assertType('bool', $update); - assertType('false', $foo); + assertType('bool', $foo); } else { assertType('bool', $update); assertType('bool', $foo); diff --git a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php index b6161df2333..a05ca24efd7 100644 --- a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php +++ b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php @@ -911,7 +911,12 @@ public function testBug4173(): void $this->polluteScopeWithLoopInitialAssignments = false; $this->checkMaybeUndefinedVariables = true; $this->polluteScopeWithAlwaysIterableForeach = true; - $this->analyse([__DIR__ . '/data/bug-4173.php'], []); + $this->analyse([__DIR__ . '/data/bug-4173.php'], [ + [ + 'Variable $value might not be defined.', // could be fixed + 30, + ], + ]); } public function testBug5805(): void @@ -1114,13 +1119,29 @@ public function testDynamicAccess(): void 18, ], [ - 'Undefined variable: $bar', + 'Variable $foo might not be defined.', + 36, + ], + [ + 'Variable $foo might not be defined.', + 37, + ], + [ + 'Variable $bar might not be defined.', 38, ], [ - 'Undefined variable: $foo', + 'Variable $bar might not be defined.', + 40, + ], + [ + 'Variable $foo might not be defined.', 41, ], + [ + 'Variable $bar might not be defined.', + 42, + ], [ 'Undefined variable: $buz', 44, @@ -1137,6 +1158,14 @@ public function testDynamicAccess(): void 'Undefined variable: $buz', 49, ], + [ + 'Variable $bar might not be defined.', + 49, + ], + [ + 'Variable $foo might not be defined.', + 49, + ], [ 'Variable $foo might not be defined.', 50, @@ -1458,24 +1487,6 @@ public function testBug13920(): void $this->analyse([__DIR__ . '/data/bug-13920.php'], []); } - public function testBug12992(): void - { - $this->cliArgumentsVariablesRegistered = true; - $this->polluteScopeWithLoopInitialAssignments = false; - $this->checkMaybeUndefinedVariables = true; - $this->polluteScopeWithAlwaysIterableForeach = true; - $this->analyse([__DIR__ . '/data/bug-12992.php'], []); - } - - public function testBug14227(): void - { - $this->cliArgumentsVariablesRegistered = true; - $this->polluteScopeWithLoopInitialAssignments = false; - $this->checkMaybeUndefinedVariables = true; - $this->polluteScopeWithAlwaysIterableForeach = true; - $this->analyse([__DIR__ . '/data/bug-14227.php'], []); - } - public function testBug14117(): void { $this->cliArgumentsVariablesRegistered = true; @@ -1484,6 +1495,10 @@ public function testBug14117(): void $this->polluteScopeWithAlwaysIterableForeach = true; $this->analyse([__DIR__ . '/data/bug-14117.php'], [ + [ + 'Variable $value might not be defined.', + 33, + ], [ 'Variable $value might not be defined.', 49, diff --git a/tests/PHPStan/Rules/Variables/data/bug-12992.php b/tests/PHPStan/Rules/Variables/data/bug-12992.php deleted file mode 100644 index e022b6c8aa6..00000000000 --- a/tests/PHPStan/Rules/Variables/data/bug-12992.php +++ /dev/null @@ -1,40 +0,0 @@ -