Skip to content

Pin the boolean value of a whole &&/|| condition into its truthy/falsey scopes#5937

Draft
phpstan-bot wants to merge 3 commits into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-z99ru7s
Draft

Pin the boolean value of a whole &&/|| condition into its truthy/falsey scopes#5937
phpstan-bot wants to merge 3 commits into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-z99ru7s

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

PHPStan failed to carry information across two identical compound conditions. Given

function logicalOr(bool $cond, bool $f): void
{
	if ($cond || $f) {
		$x = 1;
	}

	if ($cond || $f) {
		echo $x; // Variable $x might not be defined.
	}
}

it reported $x might not be defined, even though the second if has the exact
same condition as the first and therefore guarantees $x was assigned.

The single-variable form (if ($cond) { $x = 1; } if ($cond) { echo $x; })
already worked. This change extends the same reasoning to compound &&/||
conditions.

Changes

  • src/Analyser/ExprHandler/Helper/ConditionalExpressionHolderHelper.php: new
    specifyWholeExpressionType() method that pins a compound condition's boolean
    value (true in the truthy branch, false in the falsey branch) into a scope
    and re-runs the conditional-expression resolution, without re-narrowing the
    operands.
  • src/Analyser/ExprHandler/BooleanOrHandler.php and
    src/Analyser/ExprHandler/BooleanAndHandler.php: wrap the truthyScopeCallback
    / falseyScopeCallback returned from processExpr() with that helper so the
    whole ||/&& expression's boolean value is stored in both branch scopes.

Root cause

The conditional-expression machinery records guards from variables whose type
differs between the two merged branches of an if. For if ($cond) { $x = 1; },
$cond is narrowed to true in the body and false afterwards, so it becomes a
guard and the merge records "when $cond is true, $x is 1". A later
if ($cond) specifies $cond to true, fires the guard, and $x is known
defined.

A compound condition like $cond || $f narrows no single variable to a definite
value (either operand could be the truthy one), so the merge had nothing to guard
on and produced no conditional expression at all. The fix stores the whole
condition expression's boolean value in the truthy and falsey scopes, giving the
merge a guard keyed on the entire condition. Because the guard's boolean value is
also re-specified through filterBySpecifiedTypes(), the resolution runs and the
later identical if re-derives both the variable definedness and any type
narrowing.

Storing the value via filterBySpecifiedTypes() on the already operand-narrowed
scope (instead of re-filtering the whole expression) is what keeps condition side
effects intact — e.g. if (!ctype_digit($foo) || ($foo = intval($foo)) < 1) must
still see $foo as int<1, max> afterwards.

The two analogous constructs that are not fixed here, and why:

  • Negated conditions (!($a || $b)): BooleanNot computes its scopes via
    specifyTypesInCondition on the whole negated expression rather than through
    the inner operator's processExpr callbacks, so the pin never runs.
  • Definedness across elseif chains: this depends on capturing the negation of
    prior branches as multi-guards through incremental branch merges.

Both already fail for the single-variable case, so they are pre-existing
limitations independent of compound conditions and out of scope for this change.

Test

tests/PHPStan/Analyser/nsrt/bug-14871.php asserts (with assertVariableCertainty
and assertType):

  • the reported || case and the && mirror;
  • word operators ($cond or $f);
  • nested compound conditions ($a && $b) || $c;
  • comparison operands $a > 0 || $b > 0;
  • carrying real type narrowing ('set') across the second identical if;
  • a side-effect guard (if (!ctype_digit($foo) || ($foo = intval($foo)) < 1)
    still narrows $foo to int<1, max>), proving the pin does not re-run the
    assignment.

The test fails before the fix (the variables are Maybe-defined) and passes after.
The full test suite, PHPStan self-analysis (make phpstan), and the
TypeSpecifierTest / NodeScopeResolverTest suites pass.

Fixes phpstan/phpstan#14871

…falsey scopes

- Add `ConditionalExpressionHolderHelper::specifyWholeExpressionType()`, which
  pins a compound condition's boolean value (`true` in the truthy branch,
  `false` in the falsey branch) into a scope and re-runs the conditional
  expression resolution, without re-narrowing the operands.
- Wrap the truthy/falsey scope callbacks of `BooleanOrHandler` and
  `BooleanAndHandler` with that helper so the whole `||`/`&&` expression is
  stored in both branch scopes.
- This makes the merge after an `if` create a conditional-expression guard keyed
  on the whole condition (the single-variable case `if ($cond) { $x = 1; }`
  already worked because the narrowed variable became the guard; a compound
  condition narrowed no single variable, so no guard existed). A later identical
  `if ($cond || $f)` then re-derives the same narrowing and variable definedness.
- The operand-narrowed scope is reused rather than re-filtering the whole
  expression, so side effects such as `($foo = intval($foo)) < 1` inside the
  condition are preserved.
- Covered analogous cases: `&&`, word operators (`and`/`or`), nested compound
  conditions `($a && $b) || $c`, comparison operands `$a > 0 || $b > 0`, and
  carrying actual type narrowing (not just definedness) across the second `if`.
- Negated conditions (`!($a || $b)`) and elseif-chain definedness are left as
  separate pre-existing limitations: they route through `BooleanNot` /
  incremental branch merges rather than the operator's `processExpr` callbacks,
  and they already fail for single-variable conditions.

@staabm staabm left a comment

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.

the snippet https://phpstan.org/r/aab74e73-2bfe-432b-8bcd-f9b939d2eaab still produces errors with this PR, while it should not contain any "Variable ... might not be defined." error

Repeating an identical `if`/`elseif`/`else` chain only re-derived definedness
for the first branch; later branches (`elseif`, `else`) still reported the
variables assigned there as possibly undefined. This held for both compound
(`$a || $b`) and single-variable conditions.

A non-first branch is selected by a path condition spanning several conditions
(`!prevCond && thisCond`). The scope merge only recorded single-guard
conditional holders, and a single guard of that path also matches a sibling
branch, so it was dropped when that sibling merged in. Record a multi-guard
holder keyed on the whole path condition (kept alive by the vacuous-conditional
preservation because one of its guards contradicts the sibling branch), and let
such a holder take guards that are already definite in scope but not re-specified
by the condition currently being applied, as long as one guard is actively
specified. Single-guard holders keep their existing behaviour.

Multi-guard targets are restricted to plain variables whose definedness the
branch establishes, so condition variables and the narrowing recorded for
compound boolean operands are left untouched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@phpstan-bot

Copy link
Copy Markdown
Collaborator Author

Done. Here's a summary of how I addressed the review.

The review

staabb pointed out that the issue's "another example" (playground snippet) still produced Variable ... might not be defined. errors. That snippet is an if/elseif/else chain repeated with identical compound conditions:

if ($rel || $overwrite) { $vvv = 1; }
elseif ($document)       { $aaa = 2; }
else                     { $eee = 3; }

if ($rel || $overwrite) { var_dump($vvv); }  // worked already (the original PR)
elseif ($document)       { var_dump($aaa); }  // $aaa "might not be defined"
else                     { var_dump($eee); }  // $eee "might not be defined"

Root cause

The original PR fixed only the first branch. I traced the failure: a non-first branch is selected by a path condition (!prevCond && thisCond). Scope merging only recorded single-guard conditional holders, and a single guard of that path also matches a sibling branch, so it was discarded when that sibling merged in (intersectConditionalExpressions + non-vacuous preserveVacuousConditionalExpressions). This affected single-variable conditions too — not a compound-only limitation.

The fix (src/Analyser/MutatingScope.php)

  1. Creation — record a multi-guard conditional holder keyed on the whole path condition. It survives sibling merges because one of its guards contradicts the sibling branch. Restricted to plain-variable targets whose definedness the branch establishes, so condition variables and the narrowing of compound boolean operands are left untouched.
  2. Firing — a multi-guard holder may take guards already definite in scope but not re-specified by the current condition (e.g. the !prevCond part), provided ≥1 guard is actively specified. Single-guard holders keep their exact existing behaviour.

Verification

  • Reviewer's snippet + single-variable variant + the existing PR cases all pass (added to tests/PHPStan/Analyser/nsrt/bug-14871.php; confirmed failing before, passing after).
  • Soundness checked manually: reassigning a guard invalidates the carry; using a variable in the wrong branch still errors.
  • During development I caught and fixed a regression (it broke $bothUnsealed-style narrowing in TypeCombinator.php), which drove the variable-target restriction.
  • Full suite green: make tests (17479 tests), make phpstan (clean), make cs, make lint, plus NodeScopeResolverTest/TypeSpecifierTest.

Committed as fcdc6729d and pushed to the PR branch.

@staabm staabm marked this pull request as draft June 26, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

improve conditional types

2 participants