Skip to content

Augment BooleanAnd falsey and BooleanOr truthy type narrowing when left and right conditions narrow different expression keys#5595

Merged
ondrejmirtes merged 7 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-58oa332
May 12, 2026
Merged

Augment BooleanAnd falsey and BooleanOr truthy type narrowing when left and right conditions narrow different expression keys#5595
ondrejmirtes merged 7 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-58oa332

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

@phpstan-bot phpstan-bot commented May 3, 2026

Summary

When isset($test['hi']) && is_string($test['hi']) was used in a combined && condition with an early return, PHPStan failed to narrow the array union type in the falsey branch. Splitting the condition into nested if blocks worked correctly. The fix recovers the lost type narrowing for both BooleanAnd falsey and the De Morgan dual BooleanOr truthy.

Changes

  • Added augmentDisjunctionTypes() method to src/Analyser/TypeSpecifier.php that computes both disjunction-path filtered scopes and checks if candidate expressions are narrowed in both paths. If so, it creates a sureType with the union of the narrowed types.
  • Called this method in the BooleanAnd falsey branch (after the existing intersectWith) using left-falsey and right-falsey-given-left-truthy scopes.
  • Called this method in the BooleanOr truthy branch (after the existing intersectWith and augmentBooleanOrTruthyWithConditionalHolders) using left-truthy and right-truthy-given-left-falsey scopes.
  • Updated tests/PHPStan/Analyser/data/type-specifying-extensions-2-false.php and type-specifying-extensions-2-null.php: $foo is now correctly string instead of string|null because the test extension fires in all contexts and both disjunction paths narrow it.
  • Updated tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php (testBug3632): after progressive elimination by early returns, $a is now correctly narrowed to null and $b to NiceClass, producing an additional true-positive instanceof error at line 36.
  • Updated tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php (testBug11903): PHPDoc tip removed from line 21's error because the narrowing is now from control flow rather than PHPDoc types.

Root cause

In TypeSpecifier::specifyTypesInCondition, the falsey branch of BooleanAnd (and truthy branch of BooleanOr) uses SpecifiedTypes::intersectWith() to combine type assertions from the two operands. intersectWith only keeps expression keys present in both operand sets. When the left condition narrows $test (the array variable) and the right condition narrows $test['hi'] (an array offset), they have different expression keys, so intersectWith drops both — resulting in no narrowing at all.

The fix adds a post-intersection augmentation step that computes the two actual filtered scopes (one for each disjunction path) and checks if both paths narrow the same expression. If so, the union of the narrowed types is added as a sureType.

Analogous cases probed

  • BooleanOr truthy (De Morgan dual): same information loss pattern, fixed with the same augmentDisjunctionTypes method using truthy scopes instead of falsey scopes. Test added.
  • Other type-check functions: is_array, is_int, is_float, is_bool all tested in combination with isset — all pass with the fix.
  • array_key_exists: tested as an alternative to isset — passes.
  • Conditional expression holders path (via augmentBooleanOrTruthyWithConditionalHolders): already existed for BooleanOr truthy but only handles candidates from pre-existing conditional holders in the scope, not from the current condition's sureTypes. The new augmentation complements it.

Test

  • tests/PHPStan/Analyser/nsrt/bug-14566.php: regression test for the reported bug with isset && is_string on array{}|array{hi: 'hello'}|array{hi: array{0: 42, 1?: 42}}, plus:
    • Nested-if baseline (already worked)
    • Combined && with early return (the reported bug)
    • Combined && with offset assignment (from the issue's playground)
    • || dual case (!isset || !is_string)
    • is_array, is_int, is_float, is_bool variants
    • array_key_exists variant

Fixes phpstan/phpstan#14566
Fixes phpstan/phpstan#13061
Fixes phpstan/phpstan#7259

Comment thread src/Analyser/TypeSpecifier.php Outdated
Comment thread src/Analyser/TypeSpecifier.php
Comment thread src/Analyser/TypeSpecifier.php Outdated
Comment thread src/Analyser/TypeSpecifier.php Outdated
Comment thread src/Analyser/TypeSpecifier.php
Comment thread tests/PHPStan/Analyser/data/type-specifying-extensions-2-false.php
@VincentLanglet VincentLanglet self-assigned this May 5, 2026
@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-58oa332 branch 2 times, most recently from d2744e3 to f178bf5 Compare May 10, 2026 09:29
@VincentLanglet VincentLanglet requested a review from staabm May 10, 2026 10:03
@VincentLanglet
Copy link
Copy Markdown
Contributor

To me the integration test errors are correct

@ondrejmirtes
Copy link
Copy Markdown
Member

Please look at issue-bot results and decide whether anything else is fixed too

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.

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Added two regression test files:

  • tests/PHPStan/Analyser/nsrt/bug-13061.php — tests that isset($scenarioHash['type']) && $scenarioHash['type'] === 'outline' correctly narrows out the TOutlineHash type in the else branch, so loadScenarioHash() receives only TScenarioHash.
  • tests/PHPStan/Analyser/nsrt/bug-7259.php — tests both the nullable DateTimeImmutable variant and the string|int variant where || at the outer level with nested && conditions and assignments guarantees both variables are non-null / string by the time they're used.

Both make tests (12,038 tests) and make phpstan pass with no errors.

@ondrejmirtes ondrejmirtes force-pushed the create-pull-request/patch-58oa332 branch from 7ea3d4d to f93512d Compare May 11, 2026 16:20
@ondrejmirtes
Copy link
Copy Markdown
Member

I just want to see if it's slow and if it is, make the bot optimize it, then I'd merge it.

@VincentLanglet
Copy link
Copy Markdown
Contributor

I just want to see if it's slow and if it is, make the bot optimize it, then I'd merge it.

Maybe you can help me on this @staabm ?

  • I don't really understand the benchmark test result
  • I never measured perf on phpstan project. I assume you could run test with and without this fix with your usual tools ?

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

there's a lot of files these changes make slower, as the phpbench workflow run shows: https://github.com/phpstan/phpstan-src/actions/runs/25682660203/job/75398052005?pr=5595

Looks like all of them, or at least some of them are related to BooleanAnd and BooleanOr which tracks.

Please try to optimize these cases so they run as fast as on base 2.1.x branch, while still making the regression tests added in this PR work.

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. The optimization defers the expensive filterByFalseyValue/filterByTruthyValue scope computations inside augmentDisjunctionTypes until after we've verified there are actual candidates worth checking.

The key insight: intersectWith drops expression keys only when the left and right sides of &&/|| narrow different expression keys. In the common case (both sides narrow the same expressions), all candidates are already in the intersection result, viableCandidates is empty, and we return immediately — no filtered scopes computed. This also eliminates the duplicate scope computation that was happening for BooleanOr truthy (where augmentBooleanOrTruthyWithConditionalHolders already computes the same truthy scopes).

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

All of these files are still slow:

https://github.com/phpstan/phpstan-src/actions/runs/25692504674/job/75432317706?pr=5595

See:


17 variants failed:

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-5081.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-7581.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-7903.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-8146b.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-8215.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-9690.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-10538.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-10772.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-10979.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-11263.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-11913.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-12159.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-12800.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-13352.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-14207.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # bug-14590.php

  ✘ \PHPStan\Benchmark\RegressionBench::benchRunAnalyse # wordpress-user.php


try to run them with hyperfine on 2.1.x and then on this branch, you'll see the slowdown. Copy them outside of tests/bench/data if needed.

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's a summary of what I did:

The optimization adds three layers to augmentDisjunctionTypes that make it zero-cost for the common case:

  1. Count-based fast path (O(1)): If intersectWith preserved all expression keys from both sides, return immediately. This handles $x > 0 && $x < 10 patterns where both sides narrow the same variables.

  2. Lazy left scope: Only compute filterByFalseyValue(leftExpr) first — always cheap since leftExpr is a simple expression in recursive &&/|| processing.

  3. Lazy right scope: Only compute the expensive rightScope.filterByFalseyValue(rightExpr) when at least one candidate passes the left narrowing check. For long && chains like bug-12800.php (41 comparisons of different expressions), the left check fails for all candidates because a simple comparison being false doesn't narrow unrelated expressions. This avoids the O(n²) recursive processing of compound right expressions.

A/B benchmark results (same machine, augmentDisjunctionTypes enabled vs disabled):

  • All 17 flagged files show <1% difference, confirming zero measurable overhead
  • 13 of the 17 files have zero &&/|| operators — the CI failures for those files are purely due to the benchmark VM being slower than the baseline VM

@ondrejmirtes
Copy link
Copy Markdown
Member

@staabm Please take over this and test if these files are experiencing real slowdown on your computer on 2.1.x vs. this branch.

@staabm staabm self-assigned this May 12, 2026
@ondrejmirtes
Copy link
Copy Markdown
Member

Sorry, one more try from me. I thought I pushed a baseline update yesterday but I did not.

VincentLanglet and others added 5 commits May 12, 2026 09:38
…n left and right conditions narrow different expression keys

- In TypeSpecifier::specifyTypesInCondition for BooleanAnd falsey context,
  the intersection of normalized left and right SpecifiedTypes drops all
  narrowing when the two sides operate on different expression keys
  (e.g. $test vs $test['hi']). Add augmentDisjunctionTypes() to recover
  narrowing by computing both disjunction-path scopes and unioning the
  narrowed types for any expression narrowed in both paths.
- Apply the same augmentation for BooleanOr truthy context, which is
  the De Morgan dual and has the same information loss.
- Update four existing tests whose assertions reflected the old
  incomplete narrowing: type-specifying-extensions-2-{false,null}.php
  (extension fires in all contexts, both paths now narrow correctly),
  bug-3632 (additional true-positive instanceof error detected),
  bug-11903 (PHPDoc tip removed because narrowing is now from control
  flow).
- Add regression tests covering the reported case (isset && is_string
  on array union), the || dual, and parallel type checks (is_array,
  is_int, is_float, is_bool, array_key_exists).
- Rename $leftNorm/$rightNorm to $leftNormalized/$rightNormalized
- Rename $origType to $originalType
- Extract $types->getSureTypes() into $existingSureTypes before the loop
- Early-continue on left narrowing check before computing right narrowing

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…didates exist

The filterByFalseyValue/filterByTruthyValue calls are expensive. Previously
they were computed eagerly at every BooleanAnd falsey / BooleanOr truthy
branch, even when intersectWith already captured all expression keys and
there was nothing to augment.

Now augmentDisjunctionTypes first collects viable candidates (expression
keys not already in the intersection result) using only cheap checks
against the original scope. Only when at least one viable candidate
exists are the filtered scopes actually computed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…right scope

Three optimizations to make augmentDisjunctionTypes zero-cost for the common case:

1. Count-based fast path: if intersectWith preserved all expression keys from
   both sides (checked via O(1) count comparison), no keys were dropped and
   augmentation is unnecessary. This handles the common case where both sides
   of &&/|| narrow the same expression keys.

2. Lazy left scope: compute only the left filtered scope first. For
   A && B && C && ..., leftExpr is always a simple expression, so
   filterByFalseyValue(leftExpr) is cheap.

3. Lazy right scope: compute the expensive rightScope.filterByFalseyValue(rightExpr)
   only when at least one candidate passes the left narrowing check. For long
   && chains where conditions test unrelated expressions (e.g. $a === $b && $c === $d),
   the left check fails for all candidates and the right scope is never computed.
   This avoids O(n²) recursive processing of compound right expressions.

A/B benchmark on the same machine confirms zero measurable overhead: all 17
previously-flagged benchmark files show <1% difference with vs without
augmentDisjunctionTypes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ondrejmirtes ondrejmirtes force-pushed the create-pull-request/patch-58oa332 branch from b6178bf to 9d8330b Compare May 12, 2026 07:38
@staabm
Copy link
Copy Markdown
Contributor

staabm commented May 12, 2026

I can only have a look in a few hours. feel free to play with it meanwhile.

@ondrejmirtes
Copy link
Copy Markdown
Member

It's good now. I'll try to revert the latest fix to see if the benchmark is still ok.

@ondrejmirtes
Copy link
Copy Markdown
Member

Alright, it's green, let's go

@ondrejmirtes ondrejmirtes merged commit b072257 into phpstan:2.1.x May 12, 2026
113 checks passed
@ondrejmirtes ondrejmirtes deleted the create-pull-request/patch-58oa332 branch May 12, 2026 08:04
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.

4 participants