Skip to content

Commit 3b4e3e7

Browse files
phpstan-botVincentLangletclaudestaabm
authored
Respect @throws void on getIterator() when determining foreach Traversable throw points (#5666)
Co-authored-by: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Markus Staab <maggus.staab@googlemail.com>
1 parent a4d403a commit 3b4e3e7

3 files changed

Lines changed: 298 additions & 2 deletions

File tree

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use ArrayAccess;
66
use Closure;
7+
use IteratorAggregate;
78
use Override;
89
use PhpParser\Comment\Doc;
910
use PhpParser\Modifiers;
@@ -1437,8 +1438,9 @@ public function processStmtNode(
14371438
$throwPoints = array_merge($throwPoints, $finalScopeResult->getThrowPoints());
14381439
$impurePoints = array_merge($impurePoints, $finalScopeResult->getImpurePoints());
14391440
}
1440-
if (!(new ObjectType(Traversable::class))->isSuperTypeOf($scope->getType($stmt->expr))->no()) {
1441-
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $stmt->expr);
1441+
$traversableThrowPoint = $this->getTraversableForeachThrowPoint($scope, $stmt->expr);
1442+
if ($traversableThrowPoint !== null) {
1443+
$throwPoints[] = $traversableThrowPoint;
14421444
}
14431445
if ($context->isTopLevel() && $stmt->byRef) {
14441446
$finalScope = $finalScope->assignExpression(new ForeachValueByRefExpr($stmt->valueVar), new MixedType(), new MixedType());
@@ -4031,6 +4033,37 @@ private function tryProcessUnrolledConstantArrayForeach(
40314033
return ['bodyScope' => $bodyScope, 'endScope' => $endScope];
40324034
}
40334035

4036+
private function getTraversableForeachThrowPoint(MutatingScope $scope, Expr $iteratee): ?InternalThrowPoint
4037+
{
4038+
$exprType = $scope->getType($iteratee);
4039+
$traversableType = new ObjectType(Traversable::class);
4040+
4041+
if ($traversableType->isSuperTypeOf($exprType)->no()) {
4042+
return null;
4043+
}
4044+
4045+
$traversablePart = TypeCombinator::intersect($exprType, $traversableType);
4046+
$iteratorAggregateType = new ObjectType(IteratorAggregate::class);
4047+
4048+
if ($iteratorAggregateType->isSuperTypeOf($traversablePart)->yes()
4049+
&& $traversablePart->hasMethod('getIterator')->yes()) {
4050+
$method = $traversablePart->getMethod('getIterator', $scope);
4051+
$throwType = $method->getThrowType();
4052+
if ($throwType !== null) {
4053+
if ($throwType->isVoid()->yes()) {
4054+
return null;
4055+
}
4056+
return InternalThrowPoint::createExplicit($scope, $throwType, $iteratee, true);
4057+
}
4058+
4059+
if (!$this->implicitThrows) {
4060+
return null;
4061+
}
4062+
}
4063+
4064+
return InternalThrowPoint::createImplicit($scope, $iteratee);
4065+
}
4066+
40344067
/**
40354068
* @param callable(Node $node, Scope $scope): void $nodeCallback
40364069
*/

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

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
<?php // lint >= 8.0
2+
3+
declare(strict_types=1);
4+
5+
namespace Bug6833;
6+
7+
use PHPStan\TrinaryLogic;
8+
use function PHPStan\Testing\assertVariableCertainty;
9+
10+
class File
11+
{
12+
public function __construct(private int $id) {}
13+
public function getId(): int { return $this->id; }
14+
}
15+
16+
/**
17+
* @implements \IteratorAggregate<int, File>
18+
*/
19+
class FileCollection implements \IteratorAggregate
20+
{
21+
/** @var File[] */
22+
private array $files = [];
23+
24+
public function add(File $file): void
25+
{
26+
$this->files[] = $file;
27+
}
28+
29+
/** @throws void */
30+
public function getIterator(): \Iterator
31+
{
32+
return new \ArrayIterator($this->files);
33+
}
34+
}
35+
36+
function testThrowsVoidOnGetIterator(FileCollection $files): void
37+
{
38+
try {
39+
foreach ($files as $file) {
40+
echo $file->getId();
41+
}
42+
} catch (\Throwable) {
43+
assertVariableCertainty(TrinaryLogic::createYes(), $file);
44+
echo 'Invalid file:' . $file->getId();
45+
}
46+
}
47+
48+
/**
49+
* @implements \IteratorAggregate<int, File>
50+
*/
51+
class FileCollectionWithoutThrowsVoid implements \IteratorAggregate
52+
{
53+
/** @var File[] */
54+
private array $files = [];
55+
56+
public function getIterator(): \Iterator
57+
{
58+
return new \ArrayIterator($this->files);
59+
}
60+
}
61+
62+
function testWithoutThrowsVoid(FileCollectionWithoutThrowsVoid $files): void
63+
{
64+
try {
65+
foreach ($files as $file) {
66+
echo $file->getId();
67+
}
68+
} catch (\Throwable) {
69+
assertVariableCertainty(TrinaryLogic::createMaybe(), $file);
70+
echo $file->getId(); // error - getIterator() could throw
71+
}
72+
}
73+
74+
/**
75+
* @implements \IteratorAggregate<int, File>
76+
*/
77+
class FileCollectionExplicitThrows implements \IteratorAggregate
78+
{
79+
/** @var File[] */
80+
private array $files = [];
81+
82+
/** @throws \RuntimeException */
83+
public function getIterator(): \Iterator
84+
{
85+
return new \ArrayIterator($this->files);
86+
}
87+
}
88+
89+
function testExplicitThrowsMatchingCatch(FileCollectionExplicitThrows $files): void
90+
{
91+
try {
92+
foreach ($files as $file) {
93+
echo $file->getId();
94+
}
95+
} catch (\Throwable) {
96+
assertVariableCertainty(TrinaryLogic::createMaybe(), $file);
97+
echo $file->getId(); // error - getIterator() can throw RuntimeException
98+
}
99+
}
100+
101+
function testExplicitThrowsNonMatchingCatch(FileCollectionExplicitThrows $files): void
102+
{
103+
try {
104+
foreach ($files as $file) {
105+
if ($file->getId() < 0) {
106+
throw new \LogicException('negative');
107+
}
108+
}
109+
} catch (\LogicException) {
110+
echo $file->getId(); // no error - RuntimeException doesn't match LogicException catch
111+
}
112+
}
113+
114+
/** @param File[] $files */
115+
function testArrayForeach(array $files): void
116+
{
117+
try {
118+
foreach ($files as $file) {
119+
echo $file->getId();
120+
}
121+
} catch (\Throwable) {
122+
assertVariableCertainty(TrinaryLogic::createYes(), $file);
123+
echo $file->getId(); // no error - arrays don't call getIterator()
124+
}
125+
}
126+
127+
function testThrowsVoidFinallyScope(FileCollection $files): void
128+
{
129+
try {
130+
foreach ($files as $file) {
131+
doSomething();
132+
}
133+
} finally {
134+
assertVariableCertainty(TrinaryLogic::createMaybe(), $file);
135+
}
136+
}
137+
138+
/** @param File[]|FileCollection $files */
139+
function testArrayOrThrowsVoid(array|FileCollection $files): void
140+
{
141+
try {
142+
foreach ($files as $file) {
143+
echo $file->getId();
144+
}
145+
} catch (\Throwable) {
146+
assertVariableCertainty(TrinaryLogic::createYes(), $file);
147+
echo $file->getId(); // no error - array doesn't throw, getIterator() has @throws void
148+
}
149+
}
150+
151+
/** @param File[]|FileCollectionExplicitThrows $files */
152+
function testArrayOrExplicitThrows(array|FileCollectionExplicitThrows $files): void
153+
{
154+
try {
155+
foreach ($files as $file) {
156+
echo $file->getId();
157+
}
158+
} catch (\Throwable) {
159+
assertVariableCertainty(TrinaryLogic::createMaybe(), $file);
160+
echo $file->getId(); // error - getIterator() can throw RuntimeException
161+
}
162+
}
163+
164+
/** @param File[]|FileCollectionWithoutThrowsVoid $files */
165+
function testArrayOrNoAnnotation(array|FileCollectionWithoutThrowsVoid $files): void
166+
{
167+
try {
168+
foreach ($files as $file) {
169+
echo $file->getId();
170+
}
171+
} catch (\Throwable) {
172+
assertVariableCertainty(TrinaryLogic::createMaybe(), $file);
173+
echo $file->getId(); // error - getIterator() could throw
174+
}
175+
}
176+
177+
function doSomething(): void {}
178+
179+
/**
180+
* @implements \IteratorAggregate<int, File>
181+
*/
182+
class MaybeThrowingCollection implements \IteratorAggregate
183+
{
184+
/** @var File[] */
185+
private array $files = [];
186+
187+
public function add(File $file): void
188+
{
189+
$this->files[] = $file;
190+
}
191+
192+
public function getIterator(): \Iterator
193+
{
194+
return new \ArrayIterator($this->files);
195+
}
196+
}
197+
198+
function testUnionThrowsMatchingCatch(FileCollectionExplicitThrows|MaybeThrowingCollection $files): void
199+
{
200+
try {
201+
foreach ($files as $file) {
202+
echo $file->getId();
203+
}
204+
} catch (\Throwable) {
205+
assertVariableCertainty(TrinaryLogic::createMaybe(), $file);
206+
echo $file->getId(); // error - getIterator() can throw RuntimeException
207+
}
208+
}

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1571,4 +1571,59 @@ public function testBug10729(): void
15711571
]);
15721572
}
15731573

1574+
#[RequiresPhp('>= 8.0.0')]
1575+
public function testBug6833(): void
1576+
{
1577+
$this->cliArgumentsVariablesRegistered = true;
1578+
$this->polluteScopeWithLoopInitialAssignments = true;
1579+
$this->checkMaybeUndefinedVariables = true;
1580+
$this->polluteScopeWithAlwaysIterableForeach = true;
1581+
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-6833.php'], [
1582+
[
1583+
'Variable $file might not be defined.',
1584+
69,
1585+
],
1586+
[
1587+
'Variable $file might not be defined.',
1588+
70,
1589+
],
1590+
[
1591+
'Variable $file might not be defined.',
1592+
96,
1593+
],
1594+
[
1595+
'Variable $file might not be defined.',
1596+
97,
1597+
],
1598+
[
1599+
'Variable $file might not be defined.',
1600+
134,
1601+
],
1602+
[
1603+
'Variable $file might not be defined.',
1604+
159,
1605+
],
1606+
[
1607+
'Variable $file might not be defined.',
1608+
160,
1609+
],
1610+
[
1611+
'Variable $file might not be defined.',
1612+
172,
1613+
],
1614+
[
1615+
'Variable $file might not be defined.',
1616+
173,
1617+
],
1618+
[
1619+
'Variable $file might not be defined.',
1620+
205,
1621+
],
1622+
[
1623+
'Variable $file might not be defined.',
1624+
206,
1625+
],
1626+
]);
1627+
}
1628+
15741629
}

0 commit comments

Comments
 (0)