Skip to content

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

Open
phpstan-bot wants to merge 4 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-hn7ahpk
Open

Do not create conditional expressions for non-Variable expressions absent from the other branch#5465
phpstan-bot wants to merge 4 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-hn7ahpk

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

False positive "Negated boolean expression is always false" when checking an array dim fetch inside an if block that tests a variable set from different sources in if/elseif branches. PHPStan incorrectly concluded that $R['aa'] was always truthy inside if ($aa), even though $aa could be truthy from the if branch (where $R['aa'] was never accessed).

Changes

  • Added a guard in MutatingScope::createConditionalExpressions() (src/Analyser/MutatingScope.php) that prevents creating conditional expressions for non-Variable expressions when:

    1. The expression is not tracked in the other branch's expression types
    2. The expression is not a Variable node
    3. The guard expression exists in the other branch with certainty Yes
    4. The guard type overlaps with the other branch's guard type (isSuperTypeOf is not no)
  • Added regression test data at tests/PHPStan/Rules/Comparison/data/bug-14469.php covering:

    • Original reported case (array dim fetch)
    • Property fetch variant
    • Nested array dim fetch variant
    • Multiple elseif branches variant
  • Added NSRT test at tests/PHPStan/Analyser/nsrt/bug-14469.php verifying the correct type inference

Root cause

When MutatingScope::mergeWith() merges two branch scopes and one branch tracks an expression (e.g. $R['aa'] narrowed by an elseif condition) while the other does not, createConditionalExpressions() would create a conditional expression linking the guard variable ($aa) to the untracked expression ($R['aa']). When the guard later fires (e.g. if ($aa) narrows $aa to truthy), the conditional expression would incorrectly narrow $R['aa'] to its branch-specific type, even though the guard could have been satisfied by a value from the branch where $R['aa'] was never tracked.

The fix adds a Variable exception because Variables can be defined/undefined across branches (their certainty is meaningful for conditional expressions), while non-Variable expressions like array dim fetches and property fetches are always queryable and should not have their type artificially narrowed by guard expressions from branches where they weren't referenced.

Analogous cases probed and found already correct:

  • Variable certainty tracking (bug-14411-regression test continues to pass)
  • The existing guard-overlap check at the same location handles the case where the expression IS tracked in both branches

Test

  • Rule test testBug14469 in BooleanNotConstantConditionRuleTest with test data covering the original array dim fetch case plus property fetch, nested array fetch, and multiple elseif variants
  • NSRT test bug-14469.php verifying $R['aa'] is correctly inferred as mixed inside if ($aa)

Fixes phpstan/phpstan#14469

…sent from the other branch

- In `MutatingScope::createConditionalExpressions()`, skip creating a
  conditional expression when the target expression (e.g. an array dim
  fetch like `$R['aa']`) is not tracked in the other branch, the
  expression is not a Variable, and the guard type overlaps with the
  other branch's guard type.
- This prevents incorrect type narrowing where an array dim fetch or
  property fetch from an `elseif` condition leaks into a subsequent
  `if` block that checks a variable set in multiple branches.
- The Variable exception preserves the existing behavior for variable
  certainty tracking (e.g. a variable defined in only one branch
  correctly gets Yes certainty via conditional expressions).
- Tested with array dim fetch, nested array dim fetch, property fetch,
  and multiple elseif variants.
}
if (
!array_key_exists($exprString, $theirExpressionTypes)
&& !$holder->getExpr() instanceof Variable
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.

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

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.

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

… test cases

Address review comment by VincentLanglet asking why the guard is restricted
to non-Variable expressions.

The Variable exception is needed because:
- Non-Variable expressions (array dim fetches, property fetches) are always
  queryable; absence from a branch means "not narrowed", not "doesn't exist"
- Variables are different: absence means genuinely "not defined", and
  conditional expressions for newly-defined variables are essential for
  variable certainty tracking (e.g. bug-14411-regression where $order must
  be recognised as always-defined when two conditions cover all cases)
- Pre-defined variables (present in both branches) are already handled by
  the existing check that tests array_key_exists($exprString, $theirExpressionTypes)

Added variable-equivalent test cases to both the rule test data and NSRT
test to demonstrate that pre-defined variables work correctly with the
existing guard-overlap check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Remove the comments.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

In tests files added, use @param mixed rather than native type in order to pass test on PHP 7.4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Here's what was addressed:

  1. Removed comments from MutatingScope.php — the 12-line block comment above the if condition was removed per VincentLanglet's request.

  2. Used @param mixed instead of native mixed type in both test files for PHP 7.4 compatibility:

    • tests/PHPStan/Analyser/nsrt/bug-14469.php — changed mixed $input to $input with @param mixed $input PHPDoc
    • tests/PHPStan/Rules/Comparison/data/bug-14469.php — same change, also removed the verbose multi-line doc comment

Both make tests (11857 tests passing) and make phpstan (no errors) pass successfully.

…ove comments

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Here's what was changed:

  1. Test files: Replaced native mixed type with @param mixed PHPDoc annotation in both test data files for PHP 7.4 compatibility:

    • tests/PHPStan/Rules/Comparison/data/bug-14469.php: mixed $input@param mixed $input + untyped parameter
    • tests/PHPStan/Analyser/nsrt/bug-14469.php: mixed $input@param mixed $input + untyped parameter
  2. MutatingScope.php: Removed the large comment block (already done in the remote, confirmed merged correctly).

All tests pass (11857 tests, 79365 assertions) and make phpstan reports no errors.

VincentLanglet
VincentLanglet previously approved these changes Apr 15, 2026
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.

2 participants