Skip to content

Skip degenerate no-op antecedents when building boolean conditional expression holders #5951

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

Merged
staabm merged 2 commits into from
Jun 29, 2026
+107 −1
Merged

Skip degenerate no-op antecedents when building boolean conditional expression holders #5951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e12a826
Skip degenerate no-op antecedents when building boolean conditional e…
phpstan-bot Jun 29, 2026
cf817da
Guard bug-14878 against rule errors from BooleanAnd and StrictCompari…
phpstan-bot Jun 29, 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
8 changes: 7 additions & 1 deletion src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,15 @@ public function processBooleanConditionalTypes(Scope $scope, SpecifiedTypes $con
continue;
}

$scopeType = $scope->getType($expr);
$conditionType = TypeCombinator::intersect($scopeType, $type);
if ($scopeType->equals($conditionType)) {
continue;
}

$conditionExpressionTypes[$exprString] = ExpressionTypeHolder::createYes(
$expr,
TypeCombinator::intersect($scope->getType($expr), $type),
$conditionType,
);
}

Expand Down
89 changes: 89 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-14878.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
<?php declare(strict_types = 1);

namespace Bug14878;

@staabm staabm Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should reference this file from *RuleTest classes to make sure no emited error like

Result of && is always false/true.

or

Strict comparison using === between ... and ... will always evaluate to ....

@phpstan-bot phpstan-bot Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. I addressed staabm's review.

What the reviewer asked

Reference bug-14878.php from *RuleTest classes so a regression is caught by the actual rules that emitted the wrong errors — "Result of && is always false/true" and "Strict comparison using === … will always evaluate to …".

What I changed

  1. BooleanAndConstantConditionRuleTest and StrictComparisonOfDifferentTypesRuleTest — added testBug14878() analysing nsrt/bug-14878.php and expecting no errors.

  2. nsrt/bug-14878.php — the original data file's branches only contained assertType() calls, so referencing it from the rule tests would have passed even with the fix reverted (no comparison in the branch ⇒ no rule error). I added the statements that actually regressed, inside each no-narrowing branch:

    $b === true;                   // StrictComparison guard
$result = $b === true && $cond; // BooleanAnd guard

    I had to pick operands carefully so the guards aren't constant for the wrong reason: $a === 3 becomes 3 === 3 once $a is narrowed in the strict elseif, and the right operand of && sees $b narrowed to true from the left — so I used a separate mixed $cond parameter for the right side.

Verification

I reconstructed the reproducer locally (the playground wasn't reachable) to confirm the exact errors, then proved the tests are real guards by reverting the source fix:

  • With the fix: all three tests pass; full BooleanAndConstantConditionRuleTest + StrictComparisonOfDifferentTypesRuleTest (112 tests) pass; make phpstan clean.
  • With the fix reverted: the nsrt type test fails ($b is mixed~true), StrictComparison reports "will always evaluate to false", and BooleanAnd reports "Result of && is always false" — exactly the errors the reviewer wanted guarded. (The testPlainElse case correctly stays clean even with the bug, matching the PR's note that the plain-else variant was already correct.)

Committed as cf817daa8 and pushed.


use function PHPStan\Testing\assertType;

// The `$b === true` and `$b === true && $cond` statements inside the branches
// are what regressed: with the bug $b was narrowed to `mixed~true`, so they
// emitted "Strict comparison ... will always evaluate to false" and "Result of
// && is always false". They are referenced from BooleanAndConstantConditionRuleTest
// and StrictComparisonOfDifferentTypesRuleTest as a regression guard.

function test($a, $b, $cond): void
{
if (
in_array($a, [1, 2])
&& $b === true)
{

} elseif (
$a == 3) {
assertType('mixed', $b);

$b === true;
$result = $b === true && $cond;
}
}

function testStrictElseIf($a, $b, $cond): void
{
if (
in_array($a, [1, 2])
&& $b === true)
{

} elseif (
$a === 3) {
assertType('mixed', $b);

$b === true;
$result = $b === true && $cond;
}
}

function testPlainElse($a, $b, $cond): void
{
if (
in_array($a, [1, 2])
&& $b === true)
{

} else {
assertType('mixed', $b);

$b === true;
$result = $b === true && $cond;
}
}

// Same degenerate-condition pattern without in_array: a loose `==` whose
// falsey narrowing of $a is a no-op on a `mixed` type.
function testLooseEqual($a, $b, $cond): void
{
if (
($a == 1 || $a == 2)
&& $b === true)
{

} elseif ($a == 3) {
assertType('mixed', $b);

$b === true;
$result = $b === true && $cond;
}
}

// A genuinely conditional holder must still fire: $a === 1 makes the strict
// in_array() antecedent true, so $b === true must have been false.
function testMeaningfulHolderStillFires($a, $b): void
{
if (
in_array($a, [1, 2], true)
&& $b === true)
{

} elseif ($a === 1) {
assertType('mixed~true', $b);
}
}
6 changes: 6 additions & 0 deletions tests/PHPStan/Rules/Comparison/BooleanAndConstantConditionRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,12 @@ public function testBug14807(): void
$this->analyse([__DIR__ . '/data/bug-14807.php'], []);
}

public function testBug14878(): void
{
$this->treatPhpDocTypesAsCertain = true;
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14878.php'], []);
}

public function testInTrait(): void
{
$this->treatPhpDocTypesAsCertain = true;
Expand Down
5 changes: 5 additions & 0 deletions tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,11 @@ public function testBug14791(): void
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14791.php'], []);
}

public function testBug14878(): void
{
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14878.php'], []);
}

public function testBug14847(): void
{
$this->analyse([__DIR__ . '/data/bug-14847.php'], [
Expand Down
Loading