From 677ca44fe6c0da039daf9e9e46636c5b89d99191 Mon Sep 17 00:00:00 2001 From: Vincent Langlet Date: Mon, 30 Mar 2026 13:07:11 +0200 Subject: [PATCH 1/2] Preserve vacuously true conditional expressions during scope merging --- src/Analyser/MutatingScope.php | 58 ++++++++++++++++++- src/Analyser/NodeScopeResolver.php | 6 +- 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 ++-- 7 files changed, 148 insertions(+), 50 deletions(-) create mode 100644 tests/PHPStan/Rules/Variables/data/bug-12992.php create mode 100644 tests/PHPStan/Rules/Variables/data/bug-14227.php diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 4ce247488f..404097352b 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3230,7 +3230,7 @@ public function filterBySpecifiedTypes(SpecifiedTypes $specifiedTypes): self } else { $scope = $scope->removeTypeFromExpression($expr, $type); } - $specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getType($expr)); + $specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getScopeType($expr)); } $conditions = []; @@ -3362,7 +3362,7 @@ public function isInFirstLevelStatement(): bool return $this->inFirstLevelStatement; } - public function mergeWith(?self $otherScope): self + public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false): self { if ($otherScope === null || $this === $otherScope) { return $this; @@ -3372,6 +3372,18 @@ public function mergeWith(?self $otherScope): self $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, @@ -3477,6 +3489,48 @@ 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 diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 560a994af4..001c8b5b36 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); + $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true); $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); + $finalScope = $scope->mergeWith($finalScope, true); $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); + $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true); $alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating(); if (count($branchScopeStatementResult->getEndStatements()) > 0) { $endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements()); diff --git a/tests/PHPStan/Analyser/nsrt/bug-5051.php b/tests/PHPStan/Analyser/nsrt/bug-5051.php index fff7211d7a..6c3e80dce1 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|3|10', $data); + assertType('1|2', $data); assertType('bool', $update); } @@ -81,7 +81,7 @@ public function testWithBooleans($data): void if ($data === 3) { assertType('bool', $update); - assertType('bool', $foo); + assertType('true', $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('bool', $foo); + assertType('false', $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 a05ca24efd..b6161df233 100644 --- a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php +++ b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php @@ -911,12 +911,7 @@ public function testBug4173(): void $this->polluteScopeWithLoopInitialAssignments = false; $this->checkMaybeUndefinedVariables = true; $this->polluteScopeWithAlwaysIterableForeach = true; - $this->analyse([__DIR__ . '/data/bug-4173.php'], [ - [ - 'Variable $value might not be defined.', // could be fixed - 30, - ], - ]); + $this->analyse([__DIR__ . '/data/bug-4173.php'], []); } public function testBug5805(): void @@ -1119,29 +1114,13 @@ public function testDynamicAccess(): void 18, ], [ - 'Variable $foo might not be defined.', - 36, - ], - [ - 'Variable $foo might not be defined.', - 37, - ], - [ - 'Variable $bar might not be defined.', + 'Undefined variable: $bar', 38, ], [ - 'Variable $bar might not be defined.', - 40, - ], - [ - 'Variable $foo might not be defined.', + 'Undefined variable: $foo', 41, ], - [ - 'Variable $bar might not be defined.', - 42, - ], [ 'Undefined variable: $buz', 44, @@ -1158,14 +1137,6 @@ 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, @@ -1487,6 +1458,24 @@ 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; @@ -1495,10 +1484,6 @@ 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 new file mode 100644 index 0000000000..e022b6c8aa --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-12992.php @@ -0,0 +1,40 @@ + Date: Mon, 30 Mar 2026 13:07:20 +0200 Subject: [PATCH 2/2] Remove useless checks --- src/Analyser/ExprHandler/MethodCallHandler.php | 12 +++++------- src/Analyser/ExprHandler/StaticCallHandler.php | 3 +-- src/Analyser/TypeSpecifier.php | 2 +- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/Analyser/ExprHandler/MethodCallHandler.php b/src/Analyser/ExprHandler/MethodCallHandler.php index 826d757a20..56af96b2ed 100644 --- a/src/Analyser/ExprHandler/MethodCallHandler.php +++ b/src/Analyser/ExprHandler/MethodCallHandler.php @@ -153,24 +153,22 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $argsResult->getScope(); if ($methodReflection !== null) { - if ($parametersAcceptor !== null) { - $methodThrowPoint = $this->getMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope); - if ($methodThrowPoint !== null) { - $throwPoints[] = $methodThrowPoint; - } + $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() && $parametersAcceptor !== null) { + } elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin()) { $scope = $scope->assignExpression( new PossiblyImpureCallExpr($normalizedExpr, $normalizedExpr->var, sprintf('%s::%s()', $methodReflection->getDeclaringClass()->getDisplayName(), $methodReflection->getName())), $parametersAcceptor->getReturnType(), new MixedType(), ); } - if ($parametersAcceptor !== null && !$methodReflection->isStatic()) { + if (!$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 2c06152b2c..4e34e0991d 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 && $parametersAcceptor !== null) { + if ($methodReflection !== null) { $methodThrowPoint = $this->getStaticMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope); if ($methodThrowPoint !== null) { $throwPoints[] = $methodThrowPoint; @@ -221,7 +221,6 @@ 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/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 9d06e8d6fb..4f3e7f9c43 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() === [] || $parametersAcceptor === null) { + if ($assertions === null || $assertions->getAll() === []) { return null; }