Skip to content

Commit a939250

Browse files
committed
Fix phpstan/phpstan#14227: Variable might not be defined false positive with unset in mutually exclusive branch
- Added preserveSafeConditionalExpressions() to MutatingScope to preserve conditional expressions through merges when one branch invalidates a variable (e.g. via unset()) but the guard condition is disjoint from the other branch's types - This also fixes related false positives for variables defined in different branches of if/elseif/else (bug-4173, dynamic-access test improvements) - Restricted preservation to simple Variable expressions to avoid stale method call narrowing - New regression test in tests/PHPStan/Rules/Variables/data/bug-14227.php
1 parent 0c740e3 commit a939250

File tree

3 files changed

+124
-24
lines changed

3 files changed

+124
-24
lines changed

src/Analyser/MutatingScope.php

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3952,6 +3952,8 @@ public function mergeWith(?self $otherScope): self
39523952

39533953
$mergedExpressionTypes = $this->mergeVariableHolders($ourExpressionTypes, $theirExpressionTypes);
39543954
$conditionalExpressions = $this->intersectConditionalExpressions($otherScope->conditionalExpressions);
3955+
$conditionalExpressions = $this->preserveSafeConditionalExpressions($conditionalExpressions, $this->conditionalExpressions, $theirExpressionTypes);
3956+
$conditionalExpressions = $this->preserveSafeConditionalExpressions($conditionalExpressions, $otherScope->conditionalExpressions, $ourExpressionTypes);
39553957
$conditionalExpressions = $this->createConditionalExpressions(
39563958
$conditionalExpressions,
39573959
$ourExpressionTypes,
@@ -4051,6 +4053,75 @@ private function intersectConditionalExpressions(array $otherConditionalExpressi
40514053
return $newConditionalExpressions;
40524054
}
40534055

4056+
/**
4057+
* Preserve conditional expressions from one scope that were dropped by
4058+
* intersectConditionalExpressions because they don't exist in the other scope.
4059+
*
4060+
* This handles the case where a variable was invalidated (e.g. by unset()) in one
4061+
* branch, causing its conditional expressions to be removed. When the guard condition
4062+
* of a conditional expression is disjoint from the other scope's types for the guard
4063+
* variable, the conditional expression is still valid and should be preserved.
4064+
*
4065+
* @param array<string, ConditionalExpressionHolder[]> $currentConditionalExpressions
4066+
* @param array<string, ConditionalExpressionHolder[]> $scopeConditionalExpressions
4067+
* @param array<string, ExpressionTypeHolder> $otherExpressionTypes
4068+
* @return array<string, ConditionalExpressionHolder[]>
4069+
*/
4070+
private function preserveSafeConditionalExpressions(
4071+
array $currentConditionalExpressions,
4072+
array $scopeConditionalExpressions,
4073+
array $otherExpressionTypes,
4074+
): array
4075+
{
4076+
foreach ($scopeConditionalExpressions as $exprString => $holders) {
4077+
if (array_key_exists($exprString, $currentConditionalExpressions)) {
4078+
continue;
4079+
}
4080+
4081+
if (array_key_exists($exprString, $otherExpressionTypes)) {
4082+
continue;
4083+
}
4084+
4085+
if (count($holders) === 0) {
4086+
continue;
4087+
}
4088+
4089+
$firstHolder = $holders[array_key_first($holders)];
4090+
$subjectExpr = $firstHolder->getTypeHolder()->getExpr();
4091+
if (!$subjectExpr instanceof Variable || !is_string($subjectExpr->name)) {
4092+
continue;
4093+
}
4094+
4095+
$safeHolders = [];
4096+
foreach ($holders as $key => $holder) {
4097+
$safe = true;
4098+
foreach ($holder->getConditionExpressionTypeHolders() as $guardExprString => $guardHolder) {
4099+
if (!array_key_exists($guardExprString, $otherExpressionTypes)) {
4100+
$safe = false;
4101+
break;
4102+
}
4103+
if (!$otherExpressionTypes[$guardExprString]->getType()->isSuperTypeOf($guardHolder->getType())->no()) {
4104+
$safe = false;
4105+
break;
4106+
}
4107+
}
4108+
if (!$safe) {
4109+
continue;
4110+
}
4111+
4112+
$safeHolders[$key] = $holder;
4113+
}
4114+
4115+
if (count($safeHolders) <= 0) {
4116+
continue;
4117+
}
4118+
4119+
$currentConditionalExpressions[$exprString] = $safeHolders;
4120+
}
4121+
4122+
return $currentConditionalExpressions;
4123+
}
4124+
40544125
/**
40554126
* @param array<string, ConditionalExpressionHolder[]> $newConditionalExpressions
40564127
* @param array<string, ConditionalExpressionHolder[]> $existingConditionalExpressions

tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php

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

922917
public function testBug5805(): void
@@ -1119,29 +1114,13 @@ public function testDynamicAccess(): void
11191114
18,
11201115
],
11211116
[
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.',
1117+
'Undefined variable: $bar',
11311118
38,
11321119
],
11331120
[
1134-
'Variable $bar might not be defined.',
1135-
40,
1136-
],
1137-
[
1138-
'Variable $foo might not be defined.',
1121+
'Undefined variable: $foo',
11391122
41,
11401123
],
1141-
[
1142-
'Variable $bar might not be defined.',
1143-
42,
1144-
],
11451124
[
11461125
'Undefined variable: $buz',
11471126
44,
@@ -1400,4 +1379,19 @@ public function testBug14117(): void
14001379
]);
14011380
}
14021381

1382+
public function testBug14227(): void
1383+
{
1384+
$this->cliArgumentsVariablesRegistered = true;
1385+
$this->polluteScopeWithLoopInitialAssignments = false;
1386+
$this->checkMaybeUndefinedVariables = true;
1387+
$this->polluteScopeWithAlwaysIterableForeach = true;
1388+
1389+
$this->analyse([__DIR__ . '/data/bug-14227.php'], [
1390+
[
1391+
'Variable $value might not be defined.',
1392+
33,
1393+
],
1394+
]);
1395+
}
1396+
14031397
}
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug14227;
4+
5+
function foo(): void {
6+
$key = rand(0, 2);
7+
8+
if ($key === 1) {
9+
$value = 'test';
10+
}
11+
12+
if ($key === 2) {
13+
unset($value);
14+
}
15+
16+
if ($key === 1) {
17+
echo $value; // should not report "might not defined"
18+
}
19+
}
20+
21+
function bar(): void {
22+
$key = rand(0, 2);
23+
24+
if ($key === 1) {
25+
$value = 'test';
26+
}
27+
28+
if ($key !== 0) {
29+
unset($value);
30+
}
31+
32+
if ($key === 1) {
33+
echo $value; // SHOULD report - unset also runs when $key === 1
34+
}
35+
}

0 commit comments

Comments
 (0)