Skip degenerate no-op antecedents when building boolean conditional expression holders#5951
Conversation
…xpression holders
- `ConditionalExpressionHolderHelper::processBooleanConditionalTypes()` builds
conditional holders ("if antecedent narrows to X then consequent narrows to Y")
that project a boolean operand's narrowing into later scopes.
- The `sureNotTypes` branch added a condition even when the antecedent narrowing
was a no-op (`intersect(scopeType, $type)` equal to the current scope type, e.g.
`$a = mixed`). A trivially-true antecedent makes the implication fire
unconditionally, so re-specifying the antecedent variable later (e.g. in an
`elseif ($a == 3)`) wrongly applied the consequent and narrowed an unrelated
variable (`$b` to `mixed~true`).
- Mirror the guard already present in the `sureTypes` branch: skip the condition
when the computed condition type equals the existing scope type.
- This covers both `&&` and `||` since `BooleanAndHandler` and `BooleanOrHandler`
share the helper. The fix is not specific to `in_array()`: the same degenerate
holder was produced by plain loose comparisons such as `($a == 1 || $a == 2)`.
- Legitimate conditional holders (e.g. strict `in_array($a, [1, 2], true)` whose
antecedent genuinely narrows `$a`) still fire as before.
| @@ -0,0 +1,71 @@ | |||
| <?php declare(strict_types = 1); | |||
|
|
|||
| namespace Bug14878; | |||
There was a problem hiding this comment.
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 ....
There was a problem hiding this comment.
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
-
BooleanAndConstantConditionRuleTestandStrictComparisonOfDifferentTypesRuleTest— addedtestBug14878()analysingnsrt/bug-14878.phpand expecting no errors. -
nsrt/bug-14878.php— the original data file's branches only containedassertType()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 === 3becomes3 === 3once$ais narrowed in the strict elseif, and the right operand of&&sees$bnarrowed totruefrom the left — so I used a separatemixed$condparameter 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 phpstanclean. - With the fix reverted: the nsrt type test fails (
$bismixed~true),StrictComparisonreports "will always evaluate to false", andBooleanAndreports "Result of && is always false" — exactly the errors the reviewer wanted guarded. (ThetestPlainElsecase correctly stays clean even with the bug, matching the PR's note that the plain-elsevariant was already correct.)
Committed as cf817daa8 and pushed.
…son rules Reference the regression test from BooleanAndConstantConditionRuleTest and StrictComparisonOfDifferentTypesRuleTest, and add the `$b === true` / `$b === true && $cond` statements inside the branches that previously emitted "Result of && is always false" and "Strict comparison ... will always evaluate to false" so those rule tests genuinely guard the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
When a non-strict
in_array()(or any condition that cannot actually narrow itsargument) was the left operand of
&&in anif, reaching the followingelseifwhose condition touched that same argument made PHPStan wrongly narrowan unrelated variable. In the reported example:
$bshould staymixedin theelseif. The fix stops PHPStan from creating adegenerate, trivially-true conditional implication that fired the consequent
narrowing unconditionally.
Changes
src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php: inprocessBooleanConditionalTypes(), thesureNotTypesloop now skips anantecedent whose condition type (
intersect(scopeType, $type)) equals thecurrent scope type — i.e. a no-op narrowing. This mirrors the guard the
sureTypesloop already had (if ($scopeType->equals($conditionType)) continue;).The helper is shared by
BooleanAndHandlerandBooleanOrHandler, so both&&and
||are covered by the single change.Root cause
processBooleanConditionalTypes()builds "conditional expression holders" thatencode implications like if the antecedent narrows expression A to type X, then
narrow the consequent expression B to type Y. These holders are stored in the
scope and fired later (in
MutatingScope) whenever A gets re-specified.For
!(in_array($a, [1, 2]) && $b === true)the intended holder isif
in_array($a, [1, 2])is true then$b !== true. A non-strictin_arrayon a
mixedneedle cannot narrow$aat all, so its falsey specificationproduced a
sureNotTypefor$awhose intersection with the scope type was justmixedagain. ThesureNotTypesbranch stored that as the antecedentcondition
$a = mixed, which is trivially satisfied. The resulting holder waseffectively (always) then
$b !== true, so as soon as theelseif ($a == 3)re-specified
$a, the consequent fired and narrowed$btomixed~true.The pattern is "a boolean operand that does not narrow its argument still
contributes a condition". The
sureTypesbranch already avoided it; thesureNotTypesbranch did not. The fix applies the same equality guard there.This is not specific to
in_array()— any loose comparison with the sameno-narrowing shape triggers it, e.g.
($a == 1 || $a == 2) && $b === true.Test
tests/PHPStan/Analyser/nsrt/bug-14878.phpcovers:elseif ($a == 3)—$bismixed;===elseif variant —$bismixed;elsevariant (was already correct) —$bismixed;in_array,($a == 1 || $a == 2) && $b === true—$bismixed;in_array($a, [1, 2], true)andelseif ($a === 1),$bis correctlynarrowed to
mixed~true.The first, second and loose-equality cases produced
mixed~truebefore the fixand
mixedafter it.Fixes phpstan/phpstan#14878