Skip to content

Commit f6ed6bd

Browse files
VincentLangletphpstan-bot
VincentLanglet
authored and
phpstan-bot
committed
Fix incorrect type narrowing with dependent types in scope merging
- 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
1 parent b70fb0f commit f6ed6bd

11 files changed

Lines changed: 108 additions & 155 deletions

File tree

‎src/Analyser/ExprHandler/MethodCallHandler.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,22 +153,24 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
153153
$scope = $argsResult->getScope();
154154

155155
if ($methodReflection !== null) {
156-
$methodThrowPoint = $this->getMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope);
157-
if ($methodThrowPoint !== null) {
158-
$throwPoints[] = $methodThrowPoint;
156+
if ($parametersAcceptor !== null) {
157+
$methodThrowPoint = $this->getMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope);
158+
if ($methodThrowPoint !== null) {
159+
$throwPoints[] = $methodThrowPoint;
160+
}
159161
}
160162

161163
if ($methodReflection->getName() === '__construct' || $methodReflection->hasSideEffects()->yes()) {
162164
$nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($normalizedExpr->var), $scope, $storage);
163165
$scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass());
164-
} elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin()) {
166+
} elseif ($this->rememberPossiblyImpureFunctionValues && $methodReflection->hasSideEffects()->maybe() && !$methodReflection->getDeclaringClass()->isBuiltin() && $parametersAcceptor !== null) {
165167
$scope = $scope->assignExpression(
166168
new PossiblyImpureCallExpr($normalizedExpr, $normalizedExpr->var, sprintf('%s::%s()', $methodReflection->getDeclaringClass()->getDisplayName(), $methodReflection->getName())),
167169
$parametersAcceptor->getReturnType(),
168170
new MixedType(),
169171
);
170172
}
171-
if (!$methodReflection->isStatic()) {
173+
if ($parametersAcceptor !== null && !$methodReflection->isStatic()) {
172174
$selfOutType = $methodReflection->getSelfOutType();
173175
if ($selfOutType !== null) {
174176
$scope = $scope->assignExpression(

‎src/Analyser/ExprHandler/StaticCallHandler.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
198198
$scope = $argsResult->getScope();
199199
$scopeFunction = $scope->getFunction();
200200

201-
if ($methodReflection !== null) {
201+
if ($methodReflection !== null && $parametersAcceptor !== null) {
202202
$methodThrowPoint = $this->getStaticMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope);
203203
if ($methodThrowPoint !== null) {
204204
$throwPoints[] = $methodThrowPoint;
@@ -221,6 +221,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
221221
} elseif (
222222
$methodReflection !== null
223223
&& $this->rememberPossiblyImpureFunctionValues
224+
&& $parametersAcceptor !== null
224225
&& $scope->isInClass()
225226
&& $scope->getClassReflection()->is($methodReflection->getDeclaringClass()->getName())
226227
&& $methodReflection->hasSideEffects()->maybe()

‎src/Analyser/MutatingScope.php‎

Lines changed: 16 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -3205,7 +3205,7 @@ public function filterBySpecifiedTypes(SpecifiedTypes $specifiedTypes): self
32053205
} else {
32063206
$scope = $scope->removeTypeFromExpression($expr, $type);
32073207
}
3208-
$specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getScopeType($expr));
3208+
$specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getType($expr));
32093209
}
32103210

32113211
$conditions = [];
@@ -3337,7 +3337,7 @@ public function isInFirstLevelStatement(): bool
33373337
return $this->inFirstLevelStatement;
33383338
}
33393339

3340-
public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false): self
3340+
public function mergeWith(?self $otherScope): self
33413341
{
33423342
if ($otherScope === null || $this === $otherScope) {
33433343
return $this;
@@ -3347,18 +3347,6 @@ public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals =
33473347

33483348
$mergedExpressionTypes = $this->mergeVariableHolders($ourExpressionTypes, $theirExpressionTypes);
33493349
$conditionalExpressions = $this->intersectConditionalExpressions($otherScope->conditionalExpressions);
3350-
if ($preserveVacuousConditionals) {
3351-
$conditionalExpressions = $this->preserveVacuousConditionalExpressions(
3352-
$conditionalExpressions,
3353-
$this->conditionalExpressions,
3354-
$theirExpressionTypes,
3355-
);
3356-
$conditionalExpressions = $this->preserveVacuousConditionalExpressions(
3357-
$conditionalExpressions,
3358-
$otherScope->conditionalExpressions,
3359-
$ourExpressionTypes,
3360-
);
3361-
}
33623350
$conditionalExpressions = $this->createConditionalExpressions(
33633351
$conditionalExpressions,
33643352
$ourExpressionTypes,
@@ -3464,48 +3452,6 @@ private function intersectConditionalExpressions(array $otherConditionalExpressi
34643452
return $newConditionalExpressions;
34653453
}
34663454

3467-
/**
3468-
* @param array<string, ConditionalExpressionHolder[]> $currentConditionalExpressions
3469-
* @param array<string, ConditionalExpressionHolder[]> $sourceConditionalExpressions
3470-
* @param array<string, ExpressionTypeHolder> $otherExpressionTypes
3471-
* @return array<string, ConditionalExpressionHolder[]>
3472-
*/
3473-
private function preserveVacuousConditionalExpressions(
3474-
array $currentConditionalExpressions,
3475-
array $sourceConditionalExpressions,
3476-
array $otherExpressionTypes,
3477-
): array
3478-
{
3479-
foreach ($sourceConditionalExpressions as $exprString => $holders) {
3480-
foreach ($holders as $key => $holder) {
3481-
if (isset($currentConditionalExpressions[$exprString][$key])) {
3482-
continue;
3483-
}
3484-
3485-
$typeHolder = $holder->getTypeHolder();
3486-
if ($typeHolder->getCertainty()->no() && !$typeHolder->getExpr() instanceof Variable) {
3487-
continue;
3488-
}
3489-
3490-
foreach ($holder->getConditionExpressionTypeHolders() as $guardExprString => $guardTypeHolder) {
3491-
if (!array_key_exists($guardExprString, $otherExpressionTypes)) {
3492-
continue;
3493-
}
3494-
3495-
$otherType = $otherExpressionTypes[$guardExprString]->getType();
3496-
$guardType = $guardTypeHolder->getType();
3497-
3498-
if ($otherType->isSuperTypeOf($guardType)->no()) {
3499-
$currentConditionalExpressions[$exprString][$key] = $holder;
3500-
break;
3501-
}
3502-
}
3503-
}
3504-
}
3505-
3506-
return $currentConditionalExpressions;
3507-
}
3508-
35093455
/**
35103456
* @param array<string, ConditionalExpressionHolder[]> $newConditionalExpressions
35113457
* @param array<string, ConditionalExpressionHolder[]> $existingConditionalExpressions
@@ -3603,6 +3549,13 @@ private function createConditionalExpressions(
36033549
}
36043550

36053551
foreach ($variableTypeGuards as $guardExprString => $guardHolder) {
3552+
if (
3553+
array_key_exists($guardExprString, $theirExpressionTypes)
3554+
&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()
3555+
&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()
3556+
) {
3557+
continue;
3558+
}
36063559
$conditionalExpression = new ConditionalExpressionHolder([$guardExprString => $guardHolder], $holder);
36073560
$conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression;
36083561
}
@@ -3614,6 +3567,13 @@ private function createConditionalExpressions(
36143567
}
36153568

36163569
foreach ($typeGuards as $guardExprString => $guardHolder) {
3570+
if (
3571+
array_key_exists($guardExprString, $theirExpressionTypes)
3572+
&& $theirExpressionTypes[$guardExprString]->getCertainty()->yes()
3573+
&& !$guardHolder->getType()->isSuperTypeOf($theirExpressionTypes[$guardExprString]->getType())->no()
3574+
) {
3575+
continue;
3576+
}
36173577
$conditionalExpression = new ConditionalExpressionHolder([$guardExprString => $guardHolder], new ExpressionTypeHolder($mergedExprTypeHolder->getExpr(), new ErrorType(), TrinaryLogic::createNo()));
36183578
$conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression;
36193579
}

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1146,7 +1146,7 @@ public function processStmtNode(
11461146
$throwPoints = array_merge($throwPoints, $branchScopeStatementResult->getThrowPoints());
11471147
$impurePoints = array_merge($impurePoints, $branchScopeStatementResult->getImpurePoints());
11481148
$branchScope = $branchScopeStatementResult->getScope();
1149-
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true);
1149+
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope);
11501150
$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
11511151
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
11521152
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
@@ -1170,7 +1170,7 @@ public function processStmtNode(
11701170

11711171
if ($stmt->else === null) {
11721172
if (!$ifAlwaysTrue && !$lastElseIfConditionIsTrue) {
1173-
$finalScope = $scope->mergeWith($finalScope, true);
1173+
$finalScope = $scope->mergeWith($finalScope);
11741174
$alwaysTerminating = false;
11751175
}
11761176
} else {
@@ -1182,7 +1182,7 @@ public function processStmtNode(
11821182
$throwPoints = array_merge($throwPoints, $branchScopeStatementResult->getThrowPoints());
11831183
$impurePoints = array_merge($impurePoints, $branchScopeStatementResult->getImpurePoints());
11841184
$branchScope = $branchScopeStatementResult->getScope();
1185-
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true);
1185+
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope);
11861186
$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
11871187
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
11881188
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());

‎src/Analyser/TypeSpecifier.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1896,7 +1896,7 @@ private function specifyTypesFromCallableCall(TypeSpecifierContext $context, Fun
18961896
}
18971897
}
18981898

1899-
if ($assertions === null || $assertions->getAll() === []) {
1899+
if ($assertions === null || $assertions->getAll() === [] || $parametersAcceptor === null) {
19001900
return null;
19011901
}
19021902

‎tests/PHPStan/Analyser/nsrt/bug-14411.php‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<?php // lint >= 8.0
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug14411;
6+
7+
use function PHPStan\Testing\assertType;
8+
9+
/** @phpstan-impure */
10+
function get_mixed(): mixed {
11+
return random_int(0, 1) ? 'foo' : null;
12+
}
13+
14+
/** @phpstan-impure */
15+
function get_optional_int(): ?int {
16+
return random_int(0, 1) ? 42 : null;
17+
}
18+
19+
function (): void {
20+
$a = get_mixed();
21+
22+
if ($a !== null) {
23+
$b = $a;
24+
}
25+
else {
26+
$b = get_optional_int();
27+
}
28+
if ($b !== null) {
29+
assertType('mixed', $a);
30+
if ($a === null) {
31+
echo 'this is absolutely possible';
32+
}
33+
}
34+
};

‎tests/PHPStan/Analyser/nsrt/bug-5051.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public function testWithBooleans($data): void
5959
assertType('1|2|3|10', $data);
6060
assertType('bool', $update);
6161
} else {
62-
assertType('1|2', $data);
62+
assertType('1|2|3|10', $data);
6363
assertType('bool', $update);
6464
}
6565

@@ -81,15 +81,15 @@ public function testWithBooleans($data): void
8181

8282
if ($data === 3) {
8383
assertType('bool', $update);
84-
assertType('true', $foo);
84+
assertType('bool', $foo);
8585
} else {
8686
assertType('bool', $update);
8787
assertType('bool', $foo);
8888
}
8989

9090
if ($data === 1 || $data === 2) {
9191
assertType('bool', $update);
92-
assertType('false', $foo);
92+
assertType('bool', $foo);
9393
} else {
9494
assertType('bool', $update);
9595
assertType('bool', $foo);

‎tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php‎

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -911,7 +911,12 @@ public function testBug4173(): void
911911
$this->polluteScopeWithLoopInitialAssignments = false;
912912
$this->checkMaybeUndefinedVariables = true;
913913
$this->polluteScopeWithAlwaysIterableForeach = true;
914-
$this->analyse([__DIR__ . '/data/bug-4173.php'], []);
914+
$this->analyse([__DIR__ . '/data/bug-4173.php'], [
915+
[
916+
'Variable $value might not be defined.', // could be fixed
917+
30,
918+
],
919+
]);
915920
}
916921

917922
public function testBug5805(): void
@@ -1114,13 +1119,29 @@ public function testDynamicAccess(): void
11141119
18,
11151120
],
11161121
[
1117-
'Undefined variable: $bar',
1122+
'Variable $foo might not be defined.',
1123+
36,
1124+
],
1125+
[
1126+
'Variable $foo might not be defined.',
1127+
37,
1128+
],
1129+
[
1130+
'Variable $bar might not be defined.',
11181131
38,
11191132
],
11201133
[
1121-
'Undefined variable: $foo',
1134+
'Variable $bar might not be defined.',
1135+
40,
1136+
],
1137+
[
1138+
'Variable $foo might not be defined.',
11221139
41,
11231140
],
1141+
[
1142+
'Variable $bar might not be defined.',
1143+
42,
1144+
],
11241145
[
11251146
'Undefined variable: $buz',
11261147
44,
@@ -1137,6 +1158,14 @@ public function testDynamicAccess(): void
11371158
'Undefined variable: $buz',
11381159
49,
11391160
],
1161+
[
1162+
'Variable $bar might not be defined.',
1163+
49,
1164+
],
1165+
[
1166+
'Variable $foo might not be defined.',
1167+
49,
1168+
],
11401169
[
11411170
'Variable $foo might not be defined.',
11421171
50,
@@ -1458,24 +1487,6 @@ public function testBug13920(): void
14581487
$this->analyse([__DIR__ . '/data/bug-13920.php'], []);
14591488
}
14601489

1461-
public function testBug12992(): void
1462-
{
1463-
$this->cliArgumentsVariablesRegistered = true;
1464-
$this->polluteScopeWithLoopInitialAssignments = false;
1465-
$this->checkMaybeUndefinedVariables = true;
1466-
$this->polluteScopeWithAlwaysIterableForeach = true;
1467-
$this->analyse([__DIR__ . '/data/bug-12992.php'], []);
1468-
}
1469-
1470-
public function testBug14227(): void
1471-
{
1472-
$this->cliArgumentsVariablesRegistered = true;
1473-
$this->polluteScopeWithLoopInitialAssignments = false;
1474-
$this->checkMaybeUndefinedVariables = true;
1475-
$this->polluteScopeWithAlwaysIterableForeach = true;
1476-
$this->analyse([__DIR__ . '/data/bug-14227.php'], []);
1477-
}
1478-
14791490
public function testBug14117(): void
14801491
{
14811492
$this->cliArgumentsVariablesRegistered = true;
@@ -1484,6 +1495,10 @@ public function testBug14117(): void
14841495
$this->polluteScopeWithAlwaysIterableForeach = true;
14851496

14861497
$this->analyse([__DIR__ . '/data/bug-14117.php'], [
1498+
[
1499+
'Variable $value might not be defined.',
1500+
33,
1501+
],
14871502
[
14881503
'Variable $value might not be defined.',
14891504
49,

‎tests/PHPStan/Rules/Variables/data/bug-12992.php‎

Lines changed: 0 additions & 40 deletions
This file was deleted.

0 commit comments

Comments
 (0)