Skip to content

Create conditional expression holders for mixed sureType/sureNotType sides in BooleanAnd falsey and BooleanOr truthy#5763

Open
phpstan-bot wants to merge 1 commit into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-0355y1q
Open

Create conditional expression holders for mixed sureType/sureNotType sides in BooleanAnd falsey and BooleanOr truthy#5763
phpstan-bot wants to merge 1 commit into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-0355y1q

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

When multiple if (A && B) { return; } guards eliminate null combinations for two variables, PHPStan failed to narrow their types at the continuation point. For example:

function foo(?Greeter $a, ?Greeter $b): bool {
    if ($a === null && $b !== null) { return true; }
    if ($a !== null && $b === null) { return true; }
    if ($a === null && $b === null) { return false; }
    // $a and $b were still Greeter|null here, now correctly narrowed to Greeter
    return $a->isEqualTo($b);
}

The root cause was in the conditional expression holder mechanism in TypeSpecifier. When processing the falsey branch of BooleanAnd (which is a disjunction by De Morgan's law), holders that track relationships between variables were only created when both sides of the && produced the same kind of type specification (both sureTypes or both sureNotTypes). The "cross case" — where one side produces sureTypes and the other sureNotTypes (e.g., $a === null produces sureNotTypes while $b !== null produces sureTypes in the falsey context) — created no holders at all.

Changes

  • src/Analyser/TypeSpecifier.php:
    • In the BooleanAnd falsey handler: additionally call processBooleanSureConditionalTypes with normalized types (which convert sureNotTypes to sureTypes), covering the mixed sureType/sureNotType case
    • In the BooleanOr truthy handler: apply the same fix (this is the dual disjunction case)
    • Add mergeHolders() private method to properly merge holder arrays without array_merge overwriting entries that share the same expression string key
  • tests/PHPStan/Analyser/nsrt/bug-3385.php: regression test covering the three-guard pattern, reversed operand order, ||-combined guards, two-guard variant, nested ifs (control), and BooleanOr truthy conditional holder cases

Root cause

processBooleanSureConditionalTypes iterates over getSureTypes() from both the left and right side of the &&/|| condition. processBooleanNotSureConditionalTypes iterates over getSureNotTypes(). Neither method handled the case where one side had sureTypes and the other had sureNotTypes.

By normalizing the types (which calls TypeCombinator::remove(scopeType, sureNotType) to convert sureNotTypes into equivalent sureTypes), the existing processBooleanSureConditionalTypes logic creates the correct holders for all combinations.

The BooleanOr truthy context has the same disjunction structure and was fixed with the same approach.

Test

  • tests/PHPStan/Analyser/nsrt/bug-3385.php with assertType calls verifying:
    • Three sequential if (A && B) { return; } guards narrow both nullable parameters to non-null
    • Same pattern with reversed operand order
    • Guards combined with ||
    • Two-guard variant where one guard uses && and the next is a simple null check
    • Nested ifs (control case, already worked)
    • BooleanOr truthy conditional holders for the analogous case

Fixes phpstan/phpstan#3385

…sides in `BooleanAnd` falsey and `BooleanOr` truthy

- When processing the falsey branch of `BooleanAnd` (or truthy branch of
  `BooleanOr`), conditional expression holders track relationships between
  variables in a disjunction (e.g. "if $a is null then $b is null").
- `processBooleanSureConditionalTypes` only handled the case where both
  sides had sureTypes, and `processBooleanNotSureConditionalTypes` only
  handled both sides having sureNotTypes. When one side had sureTypes and
  the other sureNotTypes (e.g. `$a === null && $b !== null`), no holders
  were created.
- Fix: also call `processBooleanSureConditionalTypes` with normalized
  types, which converts sureNotTypes to sureTypes, covering the cross case.
- Add `mergeHolders()` helper to properly combine holder arrays without
  `array_merge` overwriting string keys.
- Applied the same fix to `BooleanOr` truthy context (the dual disjunction).
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