Skip to content

Commit 1b1f48c

Browse files
VincentLangletondrejmirtes
VincentLanglet
and
ondrejmirtes
authored
Consider ArrayAccess side effects for dead catch analysis (#5117)
Co-authored-by: ondrejmirtes <104888+ondrejmirtes@users.noreply.github.com>
1 parent 36e5540 commit 1b1f48c

File tree

3 files changed

+294
-2
lines changed

3 files changed

+294
-2
lines changed

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@
175175
use PHPStan\Type\Constant\ConstantIntegerType;
176176
use PHPStan\Type\Constant\ConstantStringType;
177177
use PHPStan\Type\ConstantTypeHelper;
178+
use PHPStan\Type\ErrorType;
178179
use PHPStan\Type\FileTypeMapper;
179180
use PHPStan\Type\GeneralizePrecision;
180181
use PHPStan\Type\Generic\TemplateTypeHelper;
@@ -2108,6 +2109,18 @@ private function processStmtNode(
21082109
$throwPoints = array_merge($throwPoints, $exprResult->getThrowPoints());
21092110
$impurePoints = array_merge($impurePoints, $exprResult->getImpurePoints());
21102111
if ($var instanceof ArrayDimFetch && $var->dim !== null) {
2112+
$varType = $scope->getType($var->var);
2113+
if (!$varType->isArray()->yes() && !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) {
2114+
$throwPoints = array_merge($throwPoints, $this->processExprNode(
2115+
$stmt,
2116+
new MethodCall($var->var, 'offsetUnset'),
2117+
$scope,
2118+
$storage,
2119+
new NoopNodeCallback(),
2120+
ExpressionContext::createDeep(),
2121+
)->getThrowPoints());
2122+
}
2123+
21112124
$clonedVar = $this->deepNodeCloner->cloneNode($var->var);
21122125
$traverser = new NodeTraverser();
21132126
$traverser->addVisitor(new class () extends NodeVisitorAbstract {
@@ -3664,6 +3677,18 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context, $sto
36643677
$impurePoints = array_merge($impurePoints, $result->getImpurePoints());
36653678
$isAlwaysTerminating = $isAlwaysTerminating || $result->isAlwaysTerminating();
36663679
$scope = $result->getScope();
3680+
3681+
$varType = $scope->getType($expr->var);
3682+
if (!$varType->isArray()->yes() && !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) {
3683+
$throwPoints = array_merge($throwPoints, $this->processExprNode(
3684+
$stmt,
3685+
new MethodCall($expr->var, 'offsetGet'),
3686+
$scope,
3687+
$storage,
3688+
new NoopNodeCallback(),
3689+
$context,
3690+
)->getThrowPoints());
3691+
}
36673692
} elseif ($expr instanceof Array_) {
36683693
$itemNodes = [];
36693694
$hasYield = false;
@@ -3961,6 +3986,24 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context, $sto
39613986
$impurePoints = array_merge($impurePoints, $result->getImpurePoints());
39623987
$isAlwaysTerminating = $isAlwaysTerminating || $result->isAlwaysTerminating();
39633988
$nonNullabilityResults[] = $nonNullabilityResult;
3989+
3990+
if (!($var instanceof ArrayDimFetch)) {
3991+
continue;
3992+
}
3993+
3994+
$varType = $scope->getType($var->var);
3995+
if ($varType->isArray()->yes() || (new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) {
3996+
continue;
3997+
}
3998+
3999+
$throwPoints = array_merge($throwPoints, $this->processExprNode(
4000+
$stmt,
4001+
new MethodCall($var->var, 'offsetExists'),
4002+
$scope,
4003+
$storage,
4004+
new NoopNodeCallback(),
4005+
$context,
4006+
)->getThrowPoints());
39644007
}
39654008
foreach (array_reverse($expr->vars) as $var) {
39664009
$scope = $this->lookForUnsetAllowedUndefinedExpressions($scope, $var);
@@ -6431,10 +6474,15 @@ private function processAssignVar(
64316474
$scope = $scope->assignExpression($expr, $type, $nativeType);
64326475
}
64336476

6434-
if (!$varType->isArray()->yes() && !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) {
6477+
$setVarType = $scope->getType($originalVar->var);
6478+
if (
6479+
!$setVarType instanceof ErrorType
6480+
&& !$setVarType->isArray()->yes()
6481+
&& !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($setVarType)->no()
6482+
) {
64356483
$throwPoints = array_merge($throwPoints, $this->processExprNode(
64366484
$stmt,
6437-
new MethodCall($var, 'offsetSet'),
6485+
new MethodCall($originalVar->var, 'offsetSet'),
64386486
$scope,
64396487
$storage,
64406488
new NoopNodeCallback(),

‎tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleTest.php‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,44 @@ public function testBug9267(): void
614614
$this->analyse([__DIR__ . '/data/bug-9267.php'], []);
615615
}
616616

617+
public function testBug11427(): void
618+
{
619+
$this->analyse([__DIR__ . '/data/bug-11427.php'], [
620+
[
621+
'Dead catch - Exception is never thrown in the try block.',
622+
124,
623+
],
624+
[
625+
'Dead catch - Exception is never thrown in the try block.',
626+
130,
627+
],
628+
[
629+
'Dead catch - Exception is never thrown in the try block.',
630+
136,
631+
],
632+
[
633+
'Dead catch - Exception is never thrown in the try block.',
634+
142,
635+
],
636+
[
637+
'Dead catch - Exception is never thrown in the try block.',
638+
183,
639+
],
640+
[
641+
'Dead catch - Exception is never thrown in the try block.',
642+
189,
643+
],
644+
[
645+
'Dead catch - Exception is never thrown in the try block.',
646+
197,
647+
],
648+
[
649+
'Dead catch - Exception is never thrown in the try block.',
650+
203,
651+
],
652+
]);
653+
}
654+
617655
#[RequiresPhp('>= 8.4')]
618656
public function testPropertyHooks(): void
619657
{

‎tests/PHPStan/Rules/Exceptions/data/bug-11427.php‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
<?php
2+
3+
namespace Bug11427;
4+
5+
/** @implements \ArrayAccess<int, int> */
6+
class C implements \ArrayAccess {
7+
#[\ReturnTypeWillChange]
8+
public function offsetExists($offset) {
9+
throw new \Exception("exists");
10+
}
11+
12+
#[\ReturnTypeWillChange]
13+
public function offsetGet($offset) {
14+
throw new \Exception("get");
15+
}
16+
17+
#[\ReturnTypeWillChange]
18+
public function offsetSet($offset, $value) {
19+
throw new \Exception("set");
20+
}
21+
22+
#[\ReturnTypeWillChange]
23+
public function offsetUnset($offset) {
24+
throw new \Exception("unset");
25+
}
26+
}
27+
28+
function test(C $c): void {
29+
try {
30+
$x = isset($c[1]);
31+
} catch (\Exception $e) {
32+
// offsetExists can throw
33+
}
34+
35+
try {
36+
$x = $c[1];
37+
} catch (\Exception $e) {
38+
// offsetGet can throw
39+
}
40+
41+
try {
42+
$c[1] = 1;
43+
} catch (\Exception $e) {
44+
// offsetSet can throw
45+
}
46+
47+
try {
48+
unset($c[1]);
49+
} catch (\Exception $e) {
50+
// offsetUnset can throw
51+
}
52+
}
53+
54+
/**
55+
* Union type where isArray() returns maybe and isSuperTypeOf(ArrayAccess) returns maybe.
56+
* This ensures the conditions in NodeScopeResolver are tested with types
57+
* that distinguish !->yes() from ->no() and !->no() from ->yes().
58+
*
59+
* @param array<int, int>|C $c
60+
*/
61+
function testArrayOrArrayAccess($c): void {
62+
try {
63+
$x = isset($c[1]);
64+
} catch (\Exception $e) {
65+
// offsetExists can throw when $c is C
66+
}
67+
68+
try {
69+
$x = $c[1];
70+
} catch (\Exception $e) {
71+
// offsetGet can throw when $c is C
72+
}
73+
74+
try {
75+
$c[1] = 1;
76+
} catch (\Exception $e) {
77+
// offsetSet can throw when $c is C
78+
}
79+
80+
try {
81+
unset($c[1]);
82+
} catch (\Exception $e) {
83+
// offsetUnset can throw when $c is C
84+
}
85+
}
86+
87+
class D implements \ArrayAccess {
88+
/**
89+
* @throws void
90+
*/
91+
#[\ReturnTypeWillChange]
92+
public function offsetExists($offset) {
93+
throw new \Exception("exists");
94+
}
95+
96+
/**
97+
* @throws void
98+
*/
99+
#[\ReturnTypeWillChange]
100+
public function offsetGet($offset) {
101+
throw new \Exception("get");
102+
}
103+
104+
/**
105+
* @throws void
106+
*/
107+
#[\ReturnTypeWillChange]
108+
public function offsetSet($offset, $value) {
109+
throw new \Exception("set");
110+
}
111+
112+
/**
113+
* @throws void
114+
*/
115+
#[\ReturnTypeWillChange]
116+
public function offsetUnset($offset) {
117+
throw new \Exception("unset");
118+
}
119+
}
120+
121+
function test2(D $c): void {
122+
try {
123+
$x = isset($c[1]);
124+
} catch (\Exception $e) {
125+
// offsetExists cannot throw
126+
}
127+
128+
try {
129+
$x = $c[1];
130+
} catch (\Exception $e) {
131+
// offsetGet cannot throw
132+
}
133+
134+
try {
135+
$c[1] = 1;
136+
} catch (\Exception $e) {
137+
// offsetSet cannot throw
138+
}
139+
140+
try {
141+
unset($c[1]);
142+
} catch (\Exception $e) {
143+
// offsetUnset cannot throw
144+
}
145+
}
146+
147+
/**
148+
* @param array<C> $arrayOfC
149+
*/
150+
function test3(array $arrayOfC): void
151+
{
152+
try {
153+
$x = isset($arrayOfC[0][1]);
154+
} catch (\Exception $e) {
155+
// offsetExists can throw
156+
}
157+
158+
try {
159+
$x = $arrayOfC[0][1];
160+
} catch (\Exception $e) {
161+
// offsetGet can throw
162+
}
163+
164+
try {
165+
$arrayOfC[0][1] = 1;
166+
} catch (\Exception $e) {
167+
// offsetSet can throw
168+
}
169+
170+
try {
171+
unset($arrayOfC[0][1]);
172+
} catch (\Exception $e) {
173+
// offsetUnset can throw
174+
}
175+
}
176+
177+
/**
178+
* @param array<C> $arrayOfC
179+
*/
180+
function test4(array $arrayOfC): void {
181+
try {
182+
$x = isset($arrayOfC[0]);
183+
} catch (\Exception $e) {
184+
// offsetExists cannot throw
185+
}
186+
187+
try {
188+
$x = $arrayOfC[0];
189+
} catch (\Exception $e) {
190+
// offsetGet cannot throw
191+
}
192+
193+
/** @var C<int, int> $newC */
194+
$newC = new C();
195+
try {
196+
$arrayOfC[0] = $newC;
197+
} catch (\Exception $e) {
198+
// offsetSet cannot throw
199+
}
200+
201+
try {
202+
unset($arrayOfC[0]);
203+
} catch (\Exception $e) {
204+
// offsetUnset cannot throw
205+
}
206+
}

0 commit comments

Comments
 (0)