Skip to content

Commit 90d4a7d

Browse files
zonuexezonuexe
committed
Narrow foreach body to non-empty regardless of polluteScopeWithAlwaysIterableForeach
Inside a foreach body the iterated expression is provably non-empty, so it is narrowed (list -> non-empty-list, array -> non-empty-array) at body entry. Previously this only happened when polluteScopeWithAlwaysIterableForeach was on, so with the flag off (as phpstan-strict-rules sets it) the narrowing silently disappeared even though the body is only entered when the iteratee is non-empty. The narrowing is now applied to the body scope unconditionally. To keep the flag's contract intact, the narrowing must not leak past the loop: with the flag off we must not conclude the loop always iterated. After the body pass the iterated expression's possibly-empty-ness is therefore restored in the after-loop scope (keeping any element types the body refined), so the loop-exit merge forms the same conservative scope as before -- in particular it must not decide the value variable is always defined after the loop. Only the non-emptiness the narrowing itself introduced is stripped: an iteratee that was already provably non-empty before the loop keeps that property. Closes phpstan/phpstan#13312
1 parent 7e97b9e commit 90d4a7d

6 files changed

Lines changed: 138 additions & 7 deletions

File tree

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@
143143
use PHPStan\ShouldNotHappenException;
144144
use PHPStan\TrinaryLogic;
145145
use PHPStan\Type\ClosureType;
146+
use PHPStan\Type\Constant\ConstantArrayType;
146147
use PHPStan\Type\Constant\ConstantIntegerType;
147148
use PHPStan\Type\Constant\ConstantStringType;
148149
use PHPStan\Type\FileTypeMapper;
@@ -1453,6 +1454,13 @@ public function processStmtNode(
14531454
$stmt->expr,
14541455
new Array_([]),
14551456
);
1457+
// Inside the loop body the iterated expression is provably non-empty, so
1458+
// narrow it (list -> non-empty-list, array -> non-empty-array) at body entry.
1459+
// This is independent of polluteScopeWithAlwaysIterableForeach: the narrowing
1460+
// is used only while analysing the body, and is stripped back out of the
1461+
// after-loop scope below when the flag is off, so it cannot leak the
1462+
// "loop always iterated" assumption past the loop.
1463+
$nonEmptyIterateeScope = $scope->filterByTruthyValue($arrayComparisonExpr);
14561464
$this->callNodeCallback($nodeCallback, new InForeachNode($stmt), $scope, $storage);
14571465
$originalScope = $scope;
14581466
$bodyScope = $scope;
@@ -1475,7 +1483,7 @@ public function processStmtNode(
14751483
if ($context->isTopLevel()) {
14761484
$storage = $originalStorage->duplicate();
14771485

1478-
$originalScope = $this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope;
1486+
$originalScope = $nonEmptyIterateeScope;
14791487
$unrolledResult = $this->tryProcessUnrolledConstantArrayForeach($stmt, $originalScope, $originalStorage, $context);
14801488
if ($unrolledResult !== null) {
14811489
$bodyScope = $unrolledResult['bodyScope'];
@@ -1486,7 +1494,7 @@ public function processStmtNode(
14861494
$count = 0;
14871495
do {
14881496
$prevScope = $bodyScope;
1489-
$bodyScope = $bodyScope->mergeWith($this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope);
1497+
$bodyScope = $bodyScope->mergeWith($nonEmptyIterateeScope);
14901498
$storage = $originalStorage->duplicate();
14911499
$bodyScope = $this->enterForeach($bodyScope, $storage, $originalScope, $stmt, $nodeCallback);
14921500
$bodyScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, new NoopNodeCallback(), $context->enterDeep())->filterOutLoopExitPoints();
@@ -1506,7 +1514,7 @@ public function processStmtNode(
15061514
}
15071515
}
15081516

1509-
$bodyScope = $bodyScope->mergeWith($this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope);
1517+
$bodyScope = $bodyScope->mergeWith($nonEmptyIterateeScope);
15101518
$storage = $originalStorage;
15111519
$bodyScope = $this->enterForeach($bodyScope, $storage, $originalScope, $stmt, $nodeCallback);
15121520
$finalPassContext = $unrolledTotalKeys !== null ? $context->enterUnrolledForeach($unrolledTotalKeys) : $context;
@@ -1654,6 +1662,40 @@ public function processStmtNode(
16541662
}
16551663

16561664
$isIterableAtLeastOnce = $exprType->isIterableAtLeastOnce();
1665+
1666+
$iterateeCertainty = $finalScope->hasExpressionType($stmt->expr);
1667+
if (
1668+
!$this->polluteScopeWithAlwaysIterableForeach
1669+
&& !$iterateeCertainty->no()
1670+
&& !$isIterableAtLeastOnce->yes()
1671+
) {
1672+
// The in-body non-empty narrowing must not survive into the after-loop
1673+
// scope: with polluteScopeWithAlwaysIterableForeach off we must not assume
1674+
// the loop iterated. Restore the possibly-empty-ness of the iterated
1675+
// expression (keeping the element types the loop body may have refined),
1676+
// so the loop-exit merge forms the same conservative scope as without the
1677+
// narrowing — in particular it must not conclude "$iteratee is non-empty
1678+
// therefore the value variable is always defined after the loop".
1679+
//
1680+
// Only strip the non-emptiness that the narrowing itself introduced: when
1681+
// the iteratee was already provably non-empty before the loop
1682+
// ($isIterableAtLeastOnce->yes()) that non-emptiness is independent of the
1683+
// loop and must be preserved. Likewise a literal such as
1684+
// `foreach ([1, 2] as $v)` is not a tracked expression, so skip it.
1685+
$finalIterateeType = $finalScope->getType($stmt->expr);
1686+
if ($finalIterateeType->isArray()->yes()) {
1687+
$finalIterateeNativeType = $finalScope->getNativeType($stmt->expr);
1688+
$finalScope = $finalScope->specifyExpressionType(
1689+
$stmt->expr,
1690+
TypeCombinator::union($finalIterateeType, new ConstantArrayType([], [])),
1691+
$finalIterateeNativeType->isArray()->yes()
1692+
? TypeCombinator::union($finalIterateeNativeType, new ConstantArrayType([], []))
1693+
: $finalIterateeNativeType,
1694+
$iterateeCertainty,
1695+
);
1696+
}
1697+
}
1698+
16571699
if ($isIterableAtLeastOnce->maybe() || $exprType->isIterable()->no()) {
16581700
$finalScope = $finalScope->mergeWith($scope->filterByTruthyValue(new BooleanOr(
16591701
new BinaryOp\Identical(

‎tests/PHPStan/Analyser/Bug13312NoPolluteTest.php‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Analyser;
4+
5+
use PHPStan\Testing\TypeInferenceTestCase;
6+
use PHPUnit\Framework\Attributes\DataProvider;
7+
8+
class Bug13312NoPolluteTest extends TypeInferenceTestCase
9+
{
10+
11+
public static function dataFileAsserts(): iterable
12+
{
13+
yield from self::gatherAssertTypes(__DIR__ . '/data/bug-13312-no-pollute.php');
14+
}
15+
16+
/**
17+
* @param mixed ...$args
18+
*/
19+
#[DataProvider('dataFileAsserts')]
20+
public function testFileAsserts(
21+
string $assertType,
22+
string $file,
23+
...$args,
24+
): void
25+
{
26+
$this->assertFileAsserts($assertType, $file, ...$args);
27+
}
28+
29+
public static function getAdditionalConfigFiles(): array
30+
{
31+
return [
32+
__DIR__ . '/bug-13312-no-pollute.neon',
33+
];
34+
}
35+
36+
}

‎tests/PHPStan/Analyser/bug-13312-no-pollute.neon‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
parameters:
2+
polluteScopeWithAlwaysIterableForeach: false

‎tests/PHPStan/Analyser/data/bug-13312-no-pollute.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug13312NoPollute;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
/** @param list<mixed> $arr */
8+
function foo(array $arr): void {
9+
assertType('list<mixed>', $arr);
10+
foreach ($arr as $v) {
11+
assertType('non-empty-list<mixed>', $arr);
12+
}
13+
assertType('list<mixed>', $arr);
14+
15+
for ($i = 0; $i < count($arr); ++$i) {
16+
assertType('non-empty-list<mixed>', $arr);
17+
}
18+
assertType('list<mixed>', $arr);
19+
}
20+
21+
/** @param array<string, int> $arr */
22+
function fooStringKeyed(array $arr): void {
23+
assertType('array<string, int>', $arr);
24+
foreach ($arr as $v) {
25+
assertType('non-empty-array<string, int>', $arr);
26+
}
27+
assertType('array<string, int>', $arr);
28+
}
29+
30+
/** @param list<mixed> $arr */
31+
function fooReassign(array $arr): void {
32+
foreach ($arr as $v) {
33+
$arr = [];
34+
assertType('array{}', $arr);
35+
}
36+
assertType('array{}', $arr);
37+
}

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,24 @@ function foo(array $arr): void {
3232
}
3333

3434

35+
/** @param array<string, int> $arr */
36+
function fooStringKeyed(array $arr): void {
37+
assertType('array<string, int>', $arr);
38+
foreach ($arr as $v) {
39+
assertType('non-empty-array<string, int>', $arr);
40+
}
41+
assertType('array<string, int>', $arr);
42+
}
43+
44+
/** @param list<mixed> $arr */
45+
function fooReassign(array $arr): void {
46+
foreach ($arr as $v) {
47+
$arr = [];
48+
assertType('array{}', $arr);
49+
}
50+
assertType('array{}', $arr);
51+
}
52+
3553
function fooBar(mixed $mixed): void {
3654
assertType('mixed', $mixed);
3755
foreach ($mixed as $v) {

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -961,10 +961,6 @@ public function testBug8467c(): void
961961
'Variable $v might not be defined.',
962962
16,
963963
],
964-
[
965-
'Variable $v might not be defined.',
966-
18,
967-
],
968964
]);
969965
}
970966

0 commit comments

Comments
 (0)