Skip to content

Do not create conditional expressions for non-Variable expressions absent from the other branch #5465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
phpstan-bot wants to merge 4 commits into
base: 2.1.x
Choose a base branch
from
+141 −0
Open

Do not create conditional expressions for non-Variable expressions absent from the other branch #5465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32c94b9
Do not create conditional expressions for non-Variable expressions ab…
VincentLanglet Apr 14, 2026
5857b59
Add comment explaining Variable exception and add variable-equivalent…
phpstan-bot Apr 15, 2026
96cbec6
Remove comments and use @param mixed for PHP 7.4 compatibility
phpstan-bot Apr 15, 2026
8f158b1
Use @param mixed instead of native mixed type for PHP 7.4 compat, rem…
phpstan-bot Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/Analyser/MutatingScope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3649,6 +3649,15 @@
) {
continue;
}
if (
!array_key_exists($exprString, $theirExpressionTypes)
&& !$holder->getExpr() instanceof Variable
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it restricted for !$holder->getExpr() instanceof Variable ?
Can't the same bug occurs in that case ?

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes have been pushed. Here's a summary of what was done to address VincentLanglet's review comment:

The reviewer's question: Why is the check restricted to !$holder->getExpr() instanceof Variable? Can't the same bug occur for Variables?

Investigation and answer:

I tested the Variable case empirically:

  1. Pre-defined Variables (defined before the if/elseif, present in both branches' expression types): These are handled correctly by the existing check at lines 3644-3651 which tests array_key_exists($exprString, $theirExpressionTypes). I verified this with a variable-equivalent test case ($bb = $input before the if/elseif) - no false positive occurs.

  2. Newly-defined Variables (only defined in one branch, NOT in $theirExpressionTypes): Removing the Variable exception causes a regression in bug-14411-regression.php where $order must be recognized as always-defined when two conditions ($event instanceof Event and $event instanceof OrderInterface) together cover all cases. Conditional expressions for newly-defined variables are essential for this certainty tracking.

The fundamental difference is that non-Variable expressions are "always queryable" (absence from a branch means "not narrowed"), while Variables have genuine defined/undefined semantics (absence means "not defined").

Changes made:

  • Added a code comment explaining the Variable exception and why it's needed
  • Added variable-equivalent test cases to both the rule test data and NSRT test to demonstrate correctness

&& array_key_exists($guardExprString, $theirExpressionTypes)
&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()

Check warning on line 3656 in src/Analyser/MutatingScope.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.3, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator":

@@ @@
 					!array_key_exists($exprString, $theirExpressionTypes)
 					&& !$holder->getExpr() instanceof Variable
 					&& array_key_exists($guardExprString, $theirExpressionTypes)
-					&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()
+					&& !$theirExpressionTypes[$guardExprString]->getCertainty()->no()
 					&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()
 				) {
 					continue;

Check warning on line 3656 in src/Analyser/MutatingScope.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.4, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\TrinaryLogicMutator":

@@ @@
 					!array_key_exists($exprString, $theirExpressionTypes)
 					&& !$holder->getExpr() instanceof Variable
 					&& array_key_exists($guardExprString, $theirExpressionTypes)
-					&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()
+					&& !$theirExpressionTypes[$guardExprString]->getCertainty()->no()
 					&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()
 				) {
 					continue;
&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()

Check warning on line 3657 in src/Analyser/MutatingScope.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.3, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\IsSuperTypeOfCalleeAndArgumentMutator":

@@ @@
 					&& !$holder->getExpr() instanceof Variable
 					&& array_key_exists($guardExprString, $theirExpressionTypes)
 					&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()
-					&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()
+					&& !$theirExpressionTypes[$guardExprString]->getType()->isSuperTypeOf($guardHolder->getType())->no()
 				) {
 					continue;
 				}

Check warning on line 3657 in src/Analyser/MutatingScope.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.4, ubuntu-latest) 

Escaped Mutant for Mutator "PHPStan\Infection\IsSuperTypeOfCalleeAndArgumentMutator":

@@ @@
 					&& !$holder->getExpr() instanceof Variable
 					&& array_key_exists($guardExprString, $theirExpressionTypes)
 					&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()
-					&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()
+					&& !$theirExpressionTypes[$guardExprString]->getType()->isSuperTypeOf($guardHolder->getType())->no()
 				) {
 					continue;
 				}
) {
continue;
}
$conditionalExpression = new ConditionalExpressionHolder([$guardExprString => $guardHolder], $holder);
$conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression;
}
Expand Down
35 changes: 35 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-14469.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

namespace Bug14469Nsrt;

use function PHPStan\Testing\assertType;

function t(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) {
assertType('mixed', $R['aa']);
}
}

/** @param mixed $input */
function variableEquivalent($input, bool $var1, object $user): void {
$aa = null;
$bb = $input;

if ($var1) {
$aa = $user->id === 10 ? 2 : null;
} elseif ($bb) {
$aa = $bb;
}

if ($aa) {
assertType('mixed', $bb);
}
}
6 changes: 6 additions & 0 deletions tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,4 +236,10 @@ public function testBug6702(): void
$this->analyse([__DIR__ . '/data/bug-6702.php'], []);
}

public function testBug14469(): void
{
$this->treatPhpDocTypesAsCertain = true;
$this->analyse([__DIR__ . '/data/bug-14469.php'], []);
}

}
91 changes: 91 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/bug-14469.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php

namespace Bug14469;

function t(array $R, bool $var1, object $user, bool $is): array {
$aa = null;

if ($var1) {
$aa = $user->id === 10 ? 2 : null;
} elseif ($R['aa']) {
$aa = $R['aa'];
}

if ($aa) {
if (!$R['aa']) {
return [];
}
}
return $R;
}

/** Property fetch variant */
function propertyFetch(object $obj, bool $var1, object $user): void {
$aa = null;

if ($var1) {
$aa = $user->id === 10 ? 2 : null;
} elseif ($obj->prop) {
$aa = $obj->prop;
}

if ($aa) {
if (!$obj->prop) {
return;
}
}
}

/** Nested array fetch variant */
function nestedArrayFetch(array $R, bool $var1, object $user): void {
$aa = null;

if ($var1) {
$aa = $user->id === 10 ? 2 : null;
} elseif ($R['a']['b']) {
$aa = $R['a']['b'];
}

if ($aa) {
if (!$R['a']['b']) {
return;
}
}
}

/** Multiple elseif branches */
function multipleElseif(array $R, bool $var1, bool $var2, object $user): void {
$aa = null;

if ($var1) {
$aa = $user->id === 10 ? 2 : null;
} elseif ($var2) {
$aa = 5;
} elseif ($R['aa']) {
$aa = $R['aa'];
}

if ($aa) {
if (!$R['aa']) {
return;
}
}
}

/** @param mixed $input */
function variableEquivalent(bool $var1, object $user, $input): void {
$aa = null;
$bb = $input;

if ($var1) {
$aa = $user->id === 10 ? 2 : null;
} elseif ($bb) {
$aa = $bb;
}

if ($aa) {
if (!$bb) {
return;
}
}
}
Loading