Augment BooleanAnd falsey and BooleanOr truthy type narrowing when left and right conditions narrow different expression keys#5595
Conversation
d2744e3 to
f178bf5
Compare
|
To me the integration test errors are correct |
|
Please look at issue-bot results and decide whether anything else is fixed too |
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression for phpstan/phpstan#13061
(with the code snippet https://phpstan.org/r/923d55d8-296e-42ea-897b-0dffb23e4ef5)
Add non regression for phpstan/phpstan#7259
(with both the code snippet https://phpstan.org/r/dca4f949-ab14-41d0-b02d-09adc7ff011e and https://phpstan.org/r/ee55e085-aff1-4c4e-9ef6-96ceceec9e83)
|
Pushed. Added two regression test files:
Both |
7ea3d4d to
f93512d
Compare
|
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 ?
|
ondrejmirtes
left a comment
There was a problem hiding this comment.
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.
|
Pushed. The optimization defers the expensive The key insight: |
ondrejmirtes
left a comment
There was a problem hiding this comment.
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.
|
Pushed. Here's a summary of what I did: The optimization adds three layers to
A/B benchmark results (same machine, augmentDisjunctionTypes enabled vs disabled):
|
|
@staabm Please take over this and test if these files are experiencing real slowdown on your computer on 2.1.x vs. this branch. |
|
Sorry, one more try from me. I thought I pushed a baseline update yesterday but I did not. |
…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>
b6178bf to
9d8330b
Compare
|
I can only have a look in a few hours. feel free to play with it meanwhile. |
|
It's good now. I'll try to revert the latest fix to see if the benchmark is still ok. |
…nd lazy right scope" This reverts commit 9d8330b.
|
Alright, it's green, let's go |
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 nestedifblocks worked correctly. The fix recovers the lost type narrowing for bothBooleanAndfalsey and the De Morgan dualBooleanOrtruthy.Changes
augmentDisjunctionTypes()method tosrc/Analyser/TypeSpecifier.phpthat 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.BooleanAndfalsey branch (after the existingintersectWith) using left-falsey and right-falsey-given-left-truthy scopes.BooleanOrtruthy branch (after the existingintersectWithandaugmentBooleanOrTruthyWithConditionalHolders) using left-truthy and right-truthy-given-left-falsey scopes.tests/PHPStan/Analyser/data/type-specifying-extensions-2-false.phpandtype-specifying-extensions-2-null.php:$foois now correctlystringinstead ofstring|nullbecause the test extension fires in all contexts and both disjunction paths narrow it.tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php(testBug3632): after progressive elimination by early returns,$ais now correctly narrowed tonulland$btoNiceClass, producing an additional true-positiveinstanceoferror at line 36.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 ofBooleanAnd(and truthy branch ofBooleanOr) usesSpecifiedTypes::intersectWith()to combine type assertions from the two operands.intersectWithonly 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, sointersectWithdrops 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
augmentDisjunctionTypesmethod using truthy scopes instead of falsey scopes. Test added.is_array,is_int,is_float,is_boolall tested in combination withisset— all pass with the fix.array_key_exists: tested as an alternative toisset— passes.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 withisset && is_stringonarray{}|array{hi: 'hello'}|array{hi: array{0: 42, 1?: 42}}, plus:&&with early return (the reported bug)&&with offset assignment (from the issue's playground)||dual case (!isset || !is_string)is_array,is_int,is_float,is_boolvariantsarray_key_existsvariantFixes phpstan/phpstan#14566
Fixes phpstan/phpstan#13061
Fixes phpstan/phpstan#7259