From 32cae2140d877def74ab445028d017fc11de69cd Mon Sep 17 00:00:00 2001 From: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Date: Wed, 20 May 2026 17:20:00 +0000 Subject: [PATCH 1/2] Do not create conditional expression when holder and guard have the same type and guard overlaps with other branch When merging if/elseif scopes, createConditionalExpressions could create a spurious CE linking two variables that happened to have the same type in one branch (e.g. $aa = $R['aa'] in an elseif branch). This CE would later fire incorrectly when the guard variable was narrowed, falsely narrowing the holder variable's type too. The fix adds a third skip path: when the expression does not exist in the other scope, the holder type equals the guard type, and the guard type overlaps with the other scope's guard type (isSuperTypeOf is not No), the CE is redundant and should be skipped. Closes https://github.com/phpstan/phpstan/issues/14469 --- src/Analyser/MutatingScope.php | 5 ++ tests/PHPStan/Analyser/nsrt/bug-14469.php | 19 ++++ .../BooleanNotConstantConditionRuleTest.php | 6 ++ .../Rules/Comparison/data/bug-14469.php | 88 +++++++++++++++++++ 4 files changed, 118 insertions(+) create mode 100644 tests/PHPStan/Analyser/nsrt/bug-14469.php create mode 100644 tests/PHPStan/Rules/Comparison/data/bug-14469.php diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index ef55f3683f..4c04d00d94 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3776,6 +3776,11 @@ private function createConditionalExpressions( && !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no() ) || $guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->yes() + || ( + !array_key_exists($exprString, $theirExpressionTypes) + && $holder->getType()->equals($guardHolder->getType()) + && !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no() + ) ) ) { continue; diff --git a/tests/PHPStan/Analyser/nsrt/bug-14469.php b/tests/PHPStan/Analyser/nsrt/bug-14469.php new file mode 100644 index 0000000000..e5bed8909f --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-14469.php @@ -0,0 +1,19 @@ +id === 10 ? 2 : null; + } elseif ($R['aa']) { + $aa = $R['aa']; + } + + if ($aa) { + assertType('mixed', $R['aa']); + } +} diff --git a/tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php b/tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php index fb5c504a94..8c6aec02bb 100644 --- a/tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php @@ -245,4 +245,10 @@ public function testBug12852(): void ]); } + public function testBug14469(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-14469.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Comparison/data/bug-14469.php b/tests/PHPStan/Rules/Comparison/data/bug-14469.php new file mode 100644 index 0000000000..f7bb934c15 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-14469.php @@ -0,0 +1,88 @@ +id === 10 ? 2 : null; + } elseif ($R['aa']) { + $aa = $R['aa']; + } + + if ($aa) { + if (!$R['aa']) { + return []; + } + } + return $R; +} + +function testIfConstantCondition(array $R, bool $var1, object $user): void { + $aa = null; + + if ($var1) { + $aa = $user->id === 10 ? 2 : null; + } elseif ($R['aa']) { + $aa = $R['aa']; + } + + if ($aa) { + if ($R['aa']) { + // not always true + } + } +} + +function testFalseyBranch(array $R, bool $var1, object $user): void { + $bb = 'default'; + + if ($var1) { + $bb = $user->id === 10 ? null : 'active'; + } elseif (!$R['bb']) { + $bb = $R['bb']; + } + + if (!$bb) { + if ($R['bb']) { + return; + } + } +} + +function testWithElse(array $R, bool $var1, object $user): void { + $aa = null; + + if ($var1) { + $aa = $user->id === 10 ? 2 : null; + } elseif ($R['aa']) { + $aa = $R['aa']; + } else { + $aa = 0; + } + + if ($aa) { + if (!$R['aa']) { + return; + } + } +} + +function testMultipleElseif(array $R, bool $var1, bool $var2, object $user): void { + $aa = null; + + if ($var1) { + $aa = $user->id === 10 ? 2 : null; + } elseif ($var2) { + $aa = 42; + } elseif ($R['aa']) { + $aa = $R['aa']; + } + + if ($aa) { + if (!$R['aa']) { + return; + } + } +} From 418f59a0eddaf87a34e507db1b8c4e5e0a1b1b56 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Sat, 23 May 2026 13:03:38 +0200 Subject: [PATCH 2/2] reformat --- src/Analyser/MutatingScope.php | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 4c04d00d94..be806b0613 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3769,22 +3769,26 @@ private function createConditionalExpressions( if ( array_key_exists($guardExprString, $theirExpressionTypes) && $theirExpressionTypes[$guardExprString]->getCertainty()->yes() - && ( - ( + ) { + $guardIsSuperTypeOfTheirExpr = $guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType()); + + if ( + $guardIsSuperTypeOfTheirExpr->yes() + || ( array_key_exists($exprString, $theirExpressionTypes) && $theirExpressionTypes[$exprString]->getCertainty()->yes() - && !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no() + && !$guardIsSuperTypeOfTheirExpr->no() ) - || $guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->yes() || ( !array_key_exists($exprString, $theirExpressionTypes) && $holder->getType()->equals($guardHolder->getType()) - && !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no() + && !$guardIsSuperTypeOfTheirExpr->no() ) - ) - ) { - continue; + ) { + continue; + } } + $conditionalExpression = new ConditionalExpressionHolder([$guardExprString => $guardHolder], $holder); $conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression; }