Fix phpstan/phpstan#14411: Incorrect type narrowing with dependent types#5373
Conversation
- Fixed createConditionalExpressions() in MutatingScope creating incorrect conditional type guards when the guard type overlaps with the other branch's type for the same expression - Added check: skip creating conditional expression when the guard type is a supertype of the other branch's type (isSuperTypeOf is not no), meaning the guard cannot uniquely identify which branch was taken - New regression test in tests/PHPStan/Analyser/nsrt/bug-14411.php Fixes phpstan/phpstan#14411
VincentLanglet
left a comment
There was a problem hiding this comment.
No. Propose a fix without reverting the PR #5056
Restore preserveVacuousConditionals from PR phpstan#5056 and add a targeted guard overlap check in createConditionalExpressions() to prevent creating ambiguous conditional expressions when the guard type overlaps with the other branch's type for the guard variable. This preserves the improved type narrowing from phpstan#5056 (bug phpstan#4173, #12992, #14227) while also fixing the incorrect dependent type narrowing reported in #14411. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The commit restores PR #5056's
All 11,709 tests pass and |
VincentLanglet
left a comment
There was a problem hiding this comment.
You have a regression with
interface OrderInterface {}
class Event
{
/** @return mixed */
public function getSubject()
{
return new \stdClass();
}
}
function getOrder(Event|OrderInterface $event): OrderInterface
{
if ($event instanceof Event) {
$order = $event->getSubject();
assert($order instanceof OrderInterface);
}
if ($event instanceof OrderInterface) {
$order = $event;
}
\PHPStan\Testing\assertVariableCertainty(\PHPStan\TrinaryLogic::createYes(), $order);
return $order;
}
…sions() The previous guard overlap check was too aggressive - it blocked creating conditional expressions in both loops of createConditionalExpressions(), including cases needed for variable certainty tracking. The fix: 1. Only apply the guard overlap check in the first loop (type narrowing), not the second loop (variable certainty/existence) 2. Additionally require that the guarded expression exists with certainty Yes in the other branch before skipping - this ensures we only skip when both branches define the variable (ambiguous type case), not when a variable is only defined in one branch (certainty tracking case) Adds regression test for the case reported by VincentLanglet where $order should be certainly defined after two sequential if-blocks that together cover all cases of a union type parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The fix addresses VincentLanglet's regression by refining the guard overlap check in
All 11,710 tests pass and |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
|
NB: @ondrejmirtes phpstan/phpstan#10843 is not solved by this (or your WIP PR) |
|
Perfect, thank you! |
Summary
PHPStan incorrectly reported
Strict comparison using === between mixed and null will always evaluate to falsewhen checking if a variable was null inside a conditional block that tested a different, independently assigned variable for null.The bug occurred because
createConditionalExpressions()created conditional type dependencies between all variables that differ between if/else branches, even when the guard type was ambiguous (overlapping with the other branch's type for the guard variable).Changes
createConditionalExpressions()insrc/Analyser/MutatingScope.phpto skip creating conditional expressions when the guard type overlaps with the other branch's type for the guard expression$guardType->isSuperTypeOf($theirType)returnsnobefore creating the conditional, ensuring the guard uniquely identifies the branchtests/PHPStan/Analyser/nsrt/bug-14411.phpRoot cause
When merging scopes from if/else branches,
createConditionalExpressions()creates conditional expressions like "if$bistype_from_branch1, then$aistype_from_branch1". This fires when a condition narrows$bto match the guard type.In the reported case:
$a !== null):$a: mixed~null,$b: mixed~null(from$b = $a)$a: null,$b: int|null(from$b = get_optional_int())After merging, the guard "if
$bismixed~null" was created. When$b !== nullwas checked,$bbecamemixed~null, matching the guard and incorrectly narrowing$atomixed~null. But$bcould have gotten its non-null value fromget_optional_int()in branch 2 (where$aWAS null).The fix adds a check: if the guard type (
mixed~null) is a supertype of any values from the other branch's type for the guard variable (int|nullincludesintwhich is withinmixed~null), the guard is ambiguous and the conditional expression is not created.Test
The regression test reproduces the exact scenario from the issue: two branches assign
$bfrom different sources ($avsget_optional_int()), and then checks that$acan still be null insideif ($b !== null).Fixes phpstan/phpstan#14411
Fixes phpstan/phpstan#11328
Fixes phpstan/phpstan#10085
Fixes phpstan/phpstan#14211