Gather all conjuncts of a compound condition side into BooleanAnd/BooleanOr conditional holders

[phpstan-bot]

Summary

Two interacting early-return/throw guards could make PHPStan infer a type that is narrower than reality, dropping a reachable value. For the reported snippet, $auto was inferred as null inside if ($forAll) when it should have been int|null.

The root cause is in how BooleanAnd/BooleanOr decomposition builds conditional expression holders (the machinery introduced/extended by #5760). A holder learned from an && guard kept only one conjunct of its precondition and then chained that incomplete implication with a fact learned from the other guard, concluding a wrong type.

Changes

• src/Analyser/TypeSpecifier.php
  • processBooleanConditionalTypes() now builds the holder's condition from both the sure narrowings (negated via TypeCombinator::remove) and the sureNot narrowings (negated via TypeCombinator::intersect) of the condition side, gathering every conjunct of "this side is true" into a single condition set. Previously it read only one of the two lists per call (selected by the conditionsFromSureTypes flag), so a compound condition side lost a conjunct.
  • The conditionsFromSureTypes parameter is removed and the eight per-direction calls in each branch collapse to four.
  • New mergeConditionalHolders() replaces the outer array_merge() used to combine the per-call holder maps. array_merge() is keyed by the target expression string, so a later call's entry overwrote an earlier holder for the same expression — and the old $holders[$exprString] = [] pre-initialization leaked empty entries that overwrote real holders depending on call order. The new helper unions holders per target expression by their getKey().
  • Removed the eager $holders[$exprString] = [] pre-initialization; entries are created lazily only when a holder is actually added.
• Both the BooleanAnd (false context) and BooleanOr (true context) holder paths receive the same fix, since they share the helper.

Root cause

For a guard like if (!$forAll && $expiration === null && $auto !== null) throw;, the negated fact is !(!$forAll && $expiration === null && $auto !== null). The valid implication concluding $auto is !$forAll && $expiration === null ⟹ $auto === null — it needs both conjuncts. But the condition side !$forAll && $expiration === null is itself a BooleanAnd, and its narrowings are spread across the sure list ($forAll) and the sureNot list ($expiration). Because processBooleanConditionalTypes() only consumed one list per call, it produced single-conjunct holders ($auto === null WHEN $expiration === null and $auto === null WHEN $forAll === false). The first guard separately taught $forAll ⟹ $expiration === null, so entering if ($forAll) established $expiration === null, which fired the truncated holder and collapsed $auto to null.

A second, independent defect amplified this: even when the correct multi-conjunct holder was produced, the per-call results were combined with array_merge(), which is keyed by the target expression and dropped holders for the same expression depending on call order (this is what caused the bug-6202 style isset narrowing to regress in early iterations of the fix). mergeConditionalHolders() fixes that by unioning per target expression.

Test

• tests/PHPStan/Analyser/nsrt/bug-14780.php:
  • withBothGuards() — the verbatim reproducer from the issue playground; asserts int|null for $auto.
  • singleGuard() — the same guard in isolation, asserting the truncated holder does not over-narrow.
  • fourConditions() — an analogous case with four conjuncts across two guards (broken before the fix, null instead of int|null), exercising a deeper compound condition side.

I also probed the parallel BooleanOr truthy path and deeper/right-associative BooleanAnd nestings with throwaway reproducers; the BooleanOr path is fixed symmetrically and remains covered by the existing negated-boolean-and-conditional-holders.php, boolean-and-conditional-holders-mixed-context.php and conditional-holders-or-assert-isset.php tests, which continue to pass.

Fixes phpstan/phpstan#14780

[ondrejmirtes]

Please also add test for https://phpstan.org/r/64d44954-b95e-48f9-af7d-3ee8b42da962 and fix it in case it’s still failing.

[ondrejmirtes]

Retry

[phpstan-bot]

Done. Here's a summary of what I did to address the review.

What the reviewer asked

ondrejmirtes asked to add a test for the playground at 64d44954-… and fix it if still failing. I fetched the snippet via https://api.phpstan.org/sample?id=…:

function f(?int $a, ?object $b, bool $mock): void {
    if ($a === null && $b === null && !$mock) { throw new \LogicException(); }
    if ($mock) { return; }
    \PHPStan\dumpType($a); // wanted int|null, got int
    \PHPStan\dumpType($b); // wanted object|null, got object
}

It was still failing on the PR branch (int/object instead of int|null/object|null), so this was a genuine bug, not just a missing test.

Root cause (a sibling of the PR's original bug)

The PR fixed the case where the condition side of a BooleanAnd decomposition is a compound conjunction. This snippet exposes the mirror image on the holder side:

For ($a === null && $b === null) && !$mock in the false context, a true condition ($mock) implies the other side is false. That side is a plain conjunction, so its false narrowing is empty and the holders get re-derived by swapping the truthy sure/sureNot lists. The implied negation is $a !== null || $b !== null — a disjunction — but the code split it into two independent holders ($mock ⟹ $a !== null and $mock ⟹ $b !== null), each of which fired on its own and over-narrowed.

The fix

In processBooleanConditionalTypes(), suppress holder creation only for that swapped, sureNot-based, multi-target case (new $holderSideIsNegated parameter, gated together with !$holdersFromSureTypes).

The narrow gating matters: my first attempt skipped all multi-target holder sides and broke PHPStan's own foreach analysis (make phpstan reported 4 errors in NodeScopeResolver). That code relies on a genuine negated conjunction !($stmt->keyVar instanceof Variable && is_string(...)) which asserts every conjunct true through the sure list — sound to split. Scoping the suppression to the swapped sureNot list leaves that (and the BooleanOr true context) untouched.

Verification

• Added compoundHolderSide() to tests/PHPStan/Analyser/nsrt/bug-14780.php; confirmed it fails without the fix (int/object) and passes with it.
• make phpstan: clean (the broad first attempt regressed it; the scoped fix does not).
• make tests: all 12590 pass.

Committed as 8b11bfe47 and pushed to the PR branch.