Augment BooleanAnd falsey and BooleanOr truthy type narrowing when left and right conditions narrow different expression keys#5595
Conversation
…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).
| if ($context->true()) { | ||
| $types = $leftTypes->unionWith($rightTypes); | ||
| } else { | ||
| $leftNorm = $leftTypes->normalize($scope); |
There was a problem hiding this comment.
Rename $leftNorm and rightNorm to leftNormalized and rightNormalized
There was a problem hiding this comment.
All review feedback addressed and pushed:
- Renamed
$leftNorm/$rightNormto$leftNormalized/$rightNormalizedat both call sites and in the method parameters. - Extracted
$types->getSureTypes()into$existingSureTypesvariable before the foreach loop. - Renamed
$origTypeto$originalType. - Early-continue on left narrowing — split the combined
if (!$leftNarrowed || !$rightNarrowed)into two separate checks so we skip computing$rightTypenarrowing when left isn't narrowed.
The reviewer's question about whether overriding $candidateExprs entries from the right loop is an issue: it isn't, because we only use the Expr node for scope lookups (not the type from sureTypes), and both loops map the same $exprString key to equivalent expression nodes. The actual type narrowing is computed from the filtered scopes, not from the sureTypes directly.
| $candidateExprs[$exprString] = $exprNode; | ||
| } | ||
| foreach ($rightNorm->getSureTypes() as $exprString => [$exprNode, $type]) { | ||
| $candidateExprs[$exprString] = $exprNode; |
There was a problem hiding this comment.
Isn't an issue to risk to override a sureType for the leftNormalized foreach loop ?
| } | ||
|
|
||
| foreach ($candidateExprs as $exprString => $targetExpr) { | ||
| if (isset($types->getSureTypes()[$exprString])) { |
There was a problem hiding this comment.
Create a variable for $types->getSureTypes() before the foreach loop.
| continue; | ||
| } | ||
|
|
||
| $origType = $scope->getType($targetExpr); |
There was a problem hiding this comment.
Rename this originalType
|
|
||
| $origType = $scope->getType($targetExpr); | ||
| $leftType = $leftFilteredScope->getType($targetExpr); | ||
| $rightType = $rightFilteredScope->getType($targetExpr); |
There was a problem hiding this comment.
You could already continue if !$leftNarrowed without having to compue rightNarrowed
- 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>
|
|
||
| assertType('string|null', $foo); | ||
| assertType('string', $foo); | ||
| assertType('int|null', $bar); |
There was a problem hiding this comment.
Why $foo is refined but not $bar ?
There was a problem hiding this comment.
Pushed. Here's the answer to the reviewer's question:
Why $foo is refined but not $bar:
In A && B, when the overall condition is false, there are two disjunction paths:
- Left-false:
assertString($foo)was evaluated and returned false - Right-false (given left truthy):
assertString($foo)returned true, thenassertInt($bar)returned false
The key asymmetry is that assertString($foo) (left operand) is always evaluated regardless of which path is taken, while assertInt($bar) (right operand) is only evaluated in path 2 due to short-circuit evaluation.
Since the test extension fires in both truthy and falsey contexts (nullContext: false → !$context->null()):
$foois narrowed tostringin both paths → union =string$baris narrowed tointonly in path 2; in path 1 it staysint|null(assertInt was never called) →augmentDisjunctionTypescorrectly skips it because$leftType->equals($originalType)
I added inline comments to both test files explaining this asymmetry for future readers.
ebad75c to
d2744e3
Compare
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