Skip to content

Commit 677ca44

Browse files
VincentLangletVincentLanglet
committed
Preserve vacuously true conditional expressions during scope merging
1 parent 8b36ae3 commit 677ca44

File tree

7 files changed

+148
-50
lines changed

7 files changed

+148
-50
lines changed

‎src/Analyser/MutatingScope.php‎

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3230,7 +3230,7 @@ public function filterBySpecifiedTypes(SpecifiedTypes $specifiedTypes): self
32303230
} else {
32313231
$scope = $scope->removeTypeFromExpression($expr, $type);
32323232
}
3233-
$specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getType($expr));
3233+
$specifiedExpressions[$typeSpecification['exprString']] = ExpressionTypeHolder::createYes($expr, $scope->getScopeType($expr));
32343234
}
32353235

32363236
$conditions = [];
@@ -3362,7 +3362,7 @@ public function isInFirstLevelStatement(): bool
33623362
return $this->inFirstLevelStatement;
33633363
}
33643364

3365-
public function mergeWith(?self $otherScope): self
3365+
public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false): self
33663366
{
33673367
if ($otherScope === null || $this === $otherScope) {
33683368
return $this;
@@ -3372,6 +3372,18 @@ public function mergeWith(?self $otherScope): self
33723372

33733373
$mergedExpressionTypes = $this->mergeVariableHolders($ourExpressionTypes, $theirExpressionTypes);
33743374
$conditionalExpressions = $this->intersectConditionalExpressions($otherScope->conditionalExpressions);
3375+
if ($preserveVacuousConditionals) {
3376+
$conditionalExpressions = $this->preserveVacuousConditionalExpressions(
3377+
$conditionalExpressions,
3378+
$this->conditionalExpressions,
3379+
$theirExpressionTypes,
3380+
);
3381+
$conditionalExpressions = $this->preserveVacuousConditionalExpressions(
3382+
$conditionalExpressions,
3383+
$otherScope->conditionalExpressions,
3384+
$ourExpressionTypes,
3385+
);
3386+
}
33753387
$conditionalExpressions = $this->createConditionalExpressions(
33763388
$conditionalExpressions,
33773389
$ourExpressionTypes,
@@ -3477,6 +3489,48 @@ private function intersectConditionalExpressions(array $otherConditionalExpressi
34773489
return $newConditionalExpressions;
34783490
}
34793491

3492+
/**
3493+
* @param array<string, ConditionalExpressionHolder[]> $currentConditionalExpressions
3494+
* @param array<string, ConditionalExpressionHolder[]> $sourceConditionalExpressions
3495+
* @param array<string, ExpressionTypeHolder> $otherExpressionTypes
3496+
* @return array<string, ConditionalExpressionHolder[]>
3497+
*/
3498+
private function preserveVacuousConditionalExpressions(
3499+
array $currentConditionalExpressions,
3500+
array $sourceConditionalExpressions,
3501+
array $otherExpressionTypes,
3502+
): array
3503+
{
3504+
foreach ($sourceConditionalExpressions as $exprString => $holders) {
3505+
foreach ($holders as $key => $holder) {
3506+
if (isset($currentConditionalExpressions[$exprString][$key])) {
3507+
continue;
3508+
}
3509+
3510+
$typeHolder = $holder->getTypeHolder();
3511+
if ($typeHolder->getCertainty()->no() && !$typeHolder->getExpr() instanceof Variable) {
3512+
continue;
3513+
}
3514+
3515+
foreach ($holder->getConditionExpressionTypeHolders() as $guardExprString => $guardTypeHolder) {
3516+
if (!array_key_exists($guardExprString, $otherExpressionTypes)) {
3517+
continue;
3518+
}
3519+
3520+
$otherType = $otherExpressionTypes[$guardExprString]->getType();
3521+
$guardType = $guardTypeHolder->getType();
3522+
3523+
if ($otherType->isSuperTypeOf($guardType)->no()) {
3524+
$currentConditionalExpressions[$exprString][$key] = $holder;
3525+
break;
3526+
}
3527+
}
3528+
}
3529+
}
3530+
3531+
return $currentConditionalExpressions;
3532+
}
3533+
34803534
/**
34813535
* @param array<string, ConditionalExpressionHolder[]> $newConditionalExpressions
34823536
* @param array<string, ConditionalExpressionHolder[]> $existingConditionalExpressions

‎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);
1149+
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true);
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);
1173+
$finalScope = $scope->mergeWith($finalScope, true);
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);
1185+
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true);
11861186
$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
11871187
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
11881188
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());

‎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|3|10', $data);
62+
assertType('1|2', $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('bool', $foo);
84+
assertType('true', $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('bool', $foo);
92+
assertType('false', $foo);
9393
} else {
9494
assertType('bool', $update);
9595
assertType('bool', $foo);

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

Lines changed: 21 additions & 36 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,
@@ -1158,14 +1137,6 @@ public function testDynamicAccess(): void
11581137
'Undefined variable: $buz',
11591138
49,
11601139
],
1161-
[
1162-
'Variable $bar might not be defined.',
1163-
49,
1164-
],
1165-
[
1166-
'Variable $foo might not be defined.',
1167-
49,
1168-
],
11691140
[
11701141
'Variable $foo might not be defined.',
11711142
50,
@@ -1487,6 +1458,24 @@ public function testBug13920(): void
14871458
$this->analyse([__DIR__ . '/data/bug-13920.php'], []);
14881459
}
14891460

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+
14901479
public function testBug14117(): void
14911480
{
14921481
$this->cliArgumentsVariablesRegistered = true;
@@ -1495,10 +1484,6 @@ public function testBug14117(): void
14951484
$this->polluteScopeWithAlwaysIterableForeach = true;
14961485

14971486
$this->analyse([__DIR__ . '/data/bug-14117.php'], [
1498-
[
1499-
'Variable $value might not be defined.',
1500-
33,
1501-
],
15021487
[
15031488
'Variable $value might not be defined.',
15041489
49,

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug12992;
4+
5+
class HelloWorld
6+
{
7+
public function test(\Closure $fx): void
8+
{
9+
$capture = tmpfile();
10+
if ($capture !== false) {
11+
$capturePath = stream_get_meta_data($capture)['uri'] ?? '';
12+
if (@is_writable($capturePath)) {
13+
$errorLogPrevious = ini_set('error_log', $capturePath);
14+
} else {
15+
$capture = false;
16+
}
17+
}
18+
19+
if ($capture !== false) {
20+
fclose($capture);
21+
22+
ini_set('error_log', $errorLogPrevious);
23+
}
24+
}
25+
}
26+
27+
function test(bool $v): void
28+
{
29+
if ($v) {
30+
if (rand() === 3) {
31+
$newvar = 1;
32+
} else {
33+
$v = false;
34+
}
35+
}
36+
37+
if ($v === true) {
38+
echo $newvar;
39+
}
40+
}

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug14227;
4+
5+
function moo(): 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;
18+
}
19+
}

‎tests/PHPStan/Rules/Variables/data/dynamic-access.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,18 @@ public function testScope(): void
3535
if ($name === 'foo') {
3636
echo $$name; // ok
3737
echo $foo; // ok
38-
echo $bar;
38+
echo $bar; // not ok
3939
} elseif ($name === 'bar') {
4040
echo $$name; // ok
41-
echo $foo;
41+
echo $foo; // not ok
4242
echo $bar; // ok
4343
} else {
44-
echo $$name; // ok
45-
echo $foo;
46-
echo $bar;
44+
echo $$name; // not ok
45+
echo $foo; // not ok
46+
echo $bar; // not ok
4747
}
4848

49-
echo $$name; // ok
49+
echo $$name; // ok for foo and bar but not buz
5050
echo $foo;
5151
echo $bar;
5252
}

0 commit comments

Comments
 (0)