Skip to content

Commit 8ef4512

Browse files
VincentLangletphpstan-bot
VincentLanglet
authored and
phpstan-bot
committed
Record conditional definedness guards when merging if branches with a side-effect-free condition
- Add `MutatingScope::createConditionalDefinednessExpressions()`, which records that a variable defined only in one `if` branch is defined again whenever the same branch condition holds. The condition is reconstructed from the expressions it narrowed: "type guards" (defined in both branches with a type differing from the merged type, e.g. `$s` narrowed to `'banana'`, or an array gaining a `hasOffset` accessory) and "existence guards" (made certain only in our branch, e.g. `$a['x']` via `isset()`). - All guards are combined into a single `ConditionalExpressionHolder`, so reusing only part of an `&&` condition does not wrongly imply the variable is defined. At least one type guard is required so untracked superglobal accesses (empty other-branch scope) cannot create spurious guards. - Gate the new behavior behind a `createConditionalDefinedness` flag on `mergeWith()`, set by the `if` handler only when the condition has no impure points and there are no elseif branches — a deterministic condition is what guarantees a later identical `if` picks the same branch. This keeps non-deterministic conditions (`rand() && $x === 1`) sound. - Works uniformly for `isset()`, `array_key_exists()`, `instanceof`, `=== `, `!== null`, property comparisons, `is_*()`, etc., and for both `if`/`if-else`, because it builds on the generic branch-merge guard mechanism rather than special-casing functions.
1 parent 23947f1 commit 8ef4512

3 files changed

Lines changed: 244 additions & 3 deletions

File tree

‎src/Analyser/MutatingScope.php‎

Lines changed: 105 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3584,7 +3584,7 @@ public function isInFirstLevelStatement(): bool
35843584
return $this->inFirstLevelStatement;
35853585
}
35863586

3587-
public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false): self
3587+
public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false, bool $createConditionalDefinedness = false): self
35883588
{
35893589
if ($otherScope === null || $this === $otherScope) {
35903590
return $this;
@@ -3618,6 +3618,20 @@ public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals =
36183618
$ourExpressionTypes,
36193619
$mergedExpressionTypes,
36203620
);
3621+
if ($createConditionalDefinedness) {
3622+
$conditionalExpressions = $this->createConditionalDefinednessExpressions(
3623+
$conditionalExpressions,
3624+
$ourExpressionTypes,
3625+
$theirExpressionTypes,
3626+
$mergedExpressionTypes,
3627+
);
3628+
$conditionalExpressions = $this->createConditionalDefinednessExpressions(
3629+
$conditionalExpressions,
3630+
$theirExpressionTypes,
3631+
$ourExpressionTypes,
3632+
$mergedExpressionTypes,
3633+
);
3634+
}
36213635

36223636
$filter = static function (ExpressionTypeHolder $expressionTypeHolder) {
36233637
if ($expressionTypeHolder->getCertainty()->yes()) {
@@ -3900,6 +3914,96 @@ private function createConditionalExpressions(
39003914
return $conditionalExpressions;
39013915
}
39023916

3917+
/**
3918+
* Records that a variable defined only in our branch is defined again
3919+
* whenever the same branch condition holds — so a later `if` reusing that
3920+
* condition does not report the variable as possibly undefined.
3921+
*
3922+
* The branch condition is reconstructed from the expressions that the
3923+
* condition narrowed in our branch:
3924+
* - "type guards": expressions defined in both branches whose type in our
3925+
* branch differs from the merged type (e.g. `$s` narrowed to `'banana'`
3926+
* by `$s === 'banana'`, or an array gaining a `hasOffset` accessory), and
3927+
* - "existence guards": expressions made certain (Yes) in our branch but not
3928+
* in the other one (e.g. `$a['x']` made certain by `isset($a['x'])`).
3929+
*
3930+
* All guards are combined into a single condition, so re-establishing only
3931+
* part of it later (`if ($a && $b)` then `if ($a)`) does not wrongly imply
3932+
* the variable is defined. At least one type guard is required, which keeps
3933+
* out spurious guards produced when the other branch's scope tracks nothing
3934+
* for an expression (e.g. an untracked superglobal access).
3935+
*
3936+
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
3937+
* @param array<string, ExpressionTypeHolder> $ourExpressionTypes
3938+
* @param array<string, ExpressionTypeHolder> $theirExpressionTypes
3939+
* @param array<string, ExpressionTypeHolder> $mergedExpressionTypes
3940+
* @return array<string, ConditionalExpressionHolder[]>
3941+
*/
3942+
private function createConditionalDefinednessExpressions(
3943+
array $conditionalExpressions,
3944+
array $ourExpressionTypes,
3945+
array $theirExpressionTypes,
3946+
array $mergedExpressionTypes,
3947+
): array
3948+
{
3949+
$guards = [];
3950+
$hasTypeGuard = false;
3951+
$newlyDefined = [];
3952+
foreach ($ourExpressionTypes as $exprString => $holder) {
3953+
if ($holder->getExpr() instanceof VirtualNode) {
3954+
continue;
3955+
}
3956+
if (!$holder->getCertainty()->yes()) {
3957+
continue;
3958+
}
3959+
if (!array_key_exists($exprString, $mergedExpressionTypes)) {
3960+
continue;
3961+
}
3962+
3963+
$theirHolder = $theirExpressionTypes[$exprString] ?? null;
3964+
if ($theirHolder === null || !$theirHolder->getCertainty()->yes()) {
3965+
$guards[$exprString] = $holder;
3966+
3967+
$expr = $holder->getExpr();
3968+
if (
3969+
$theirHolder === null
3970+
&& $expr instanceof Variable
3971+
&& is_string($expr->name)
3972+
&& !$this->isGlobalVariable($expr->name)
3973+
) {
3974+
$newlyDefined[$exprString] = $holder;
3975+
}
3976+
3977+
continue;
3978+
}
3979+
3980+
if ($mergedExpressionTypes[$exprString]->equalTypes($holder)) {
3981+
continue;
3982+
}
3983+
3984+
$guards[$exprString] = $holder;
3985+
$hasTypeGuard = true;
3986+
}
3987+
3988+
if (!$hasTypeGuard || count($newlyDefined) === 0) {
3989+
return $conditionalExpressions;
3990+
}
3991+
3992+
foreach ($newlyDefined as $exprString => $holder) {
3993+
$variableGuards = $guards;
3994+
unset($variableGuards[$exprString]);
3995+
3996+
if (count($variableGuards) === 0) {
3997+
continue;
3998+
}
3999+
4000+
$conditionalExpression = new ConditionalExpressionHolder($variableGuards, $holder);
4001+
$conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression;
4002+
}
4003+
4004+
return $conditionalExpressions;
4005+
}
4006+
39034007
/**
39044008
* @param array<string, ExpressionTypeHolder> $ourVariableTypeHolders
39054009
* @param array<string, ExpressionTypeHolder> $theirVariableTypeHolders

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,6 +1311,13 @@ public function processStmtNode(
13111311
$alwaysTerminating = true;
13121312
$hasYield = $condResult->hasYield();
13131313

1314+
// Only a deterministic (side-effect-free) condition guarantees that a
1315+
// later `if` with the same condition picks the same branch, so that a
1316+
// variable defined here is defined there too. Elseif chains rebind the
1317+
// condition and are left to the regular per-branch tracking.
1318+
$createConditionalDefinedness = count($condResult->getImpurePoints()) === 0
1319+
&& count($stmt->elseifs) === 0;
1320+
13141321
$branchScopeStatementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $condResult->getTruthyScope(), $storage, $nodeCallback, $context);
13151322

13161323
if (!$conditionType->isTrue()->no()) {
@@ -1375,7 +1382,7 @@ public function processStmtNode(
13751382

13761383
if ($stmt->else === null) {
13771384
if (!$ifAlwaysTrue && !$lastElseIfConditionIsTrue) {
1378-
$finalScope = $scope->mergeWith($finalScope, true);
1385+
$finalScope = $scope->mergeWith($finalScope, true, $createConditionalDefinedness);
13791386
$alwaysTerminating = false;
13801387
}
13811388
} else {
@@ -1387,7 +1394,7 @@ public function processStmtNode(
13871394
$throwPoints = array_merge($throwPoints, $branchScopeStatementResult->getThrowPoints());
13881395
$impurePoints = array_merge($impurePoints, $branchScopeStatementResult->getImpurePoints());
13891396
$branchScope = $branchScopeStatementResult->getScope();
1390-
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true);
1397+
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true, $createConditionalDefinedness);
13911398
$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
13921399
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
13931400
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug7718;
4+
5+
use PHPStan\TrinaryLogic;
6+
use function PHPStan\Testing\assertVariableCertainty;
7+
8+
/**
9+
* @param array<string, int> $elements
10+
*/
11+
function isset_twice(array $elements): void
12+
{
13+
if (isset($elements['a'])) {
14+
$a = 1;
15+
}
16+
17+
if (isset($elements['a'])) {
18+
assertVariableCertainty(TrinaryLogic::createYes(), $a);
19+
}
20+
}
21+
22+
function strict_compare_twice(string $s): void
23+
{
24+
if ($s === 'banana') {
25+
$bananas = 1;
26+
}
27+
28+
if ($s === 'banana') {
29+
assertVariableCertainty(TrinaryLogic::createYes(), $bananas);
30+
}
31+
}
32+
33+
function int_compare_twice(int $x): void
34+
{
35+
if ($x === 1) {
36+
$a = 1;
37+
}
38+
39+
if ($x === 1) {
40+
assertVariableCertainty(TrinaryLogic::createYes(), $a);
41+
}
42+
}
43+
44+
/**
45+
* @param array<string, int> $elements
46+
*/
47+
function array_key_exists_twice(array $elements): void
48+
{
49+
if (array_key_exists('a', $elements)) {
50+
$a = 1;
51+
}
52+
53+
if (array_key_exists('a', $elements)) {
54+
assertVariableCertainty(TrinaryLogic::createYes(), $a);
55+
}
56+
}
57+
58+
function instanceof_twice(mixed $x): void
59+
{
60+
if ($x instanceof \Throwable) {
61+
$a = 1;
62+
}
63+
64+
if ($x instanceof \Throwable) {
65+
assertVariableCertainty(TrinaryLogic::createYes(), $a);
66+
}
67+
}
68+
69+
function not_null_twice(?int $x): void
70+
{
71+
if ($x !== null) {
72+
$a = 1;
73+
}
74+
75+
if ($x !== null) {
76+
assertVariableCertainty(TrinaryLogic::createYes(), $a);
77+
}
78+
}
79+
80+
// A different condition must NOT prove the variable defined.
81+
function different_condition(string $s): void
82+
{
83+
if ($s === 'a') {
84+
$a = 1;
85+
}
86+
87+
if ($s === 'b') {
88+
assertVariableCertainty(TrinaryLogic::createMaybe(), $a);
89+
}
90+
}
91+
92+
/**
93+
* @param array<string, int> $elements
94+
*/
95+
function isset_then_else(array $elements): void
96+
{
97+
if (isset($elements['a'])) {
98+
$a = 1;
99+
} else {
100+
$a = 2;
101+
}
102+
103+
if (isset($elements['a'])) {
104+
assertVariableCertainty(TrinaryLogic::createYes(), $a);
105+
}
106+
}
107+
108+
// Reusing only part of an `&&` condition must NOT prove the variable defined.
109+
function partial_condition_reuse(int $x, int $y): void
110+
{
111+
if ($x === 1 && $y === 2) {
112+
$a = 1;
113+
}
114+
115+
if ($x === 1) {
116+
assertVariableCertainty(TrinaryLogic::createMaybe(), $a);
117+
}
118+
}
119+
120+
// A non-deterministic condition must NOT prove the variable defined when reused.
121+
function impure_condition(int $x): void
122+
{
123+
if (rand(0, 1) === 1 && $x === 1) {
124+
$v = 1;
125+
}
126+
127+
if ($x === 1) {
128+
assertVariableCertainty(TrinaryLogic::createMaybe(), $v);
129+
}
130+
}

0 commit comments

Comments
 (0)