Skip to content

Commit a4609bb

Browse files
zonuexezonuexe
committed
Narrow foreach body to non-empty under bleedingEdge
Inside a foreach body the iterated expression is provably non-empty, so it can be narrowed (list to non-empty-list, array to non-empty-array) at body entry. Before, this happened only when polluteScopeWithAlwaysIterableForeach was on, so with the flag off (as phpstan-strict-rules sets it) the narrowing disappeared even though the body is only entered when the iteratee is non-empty. The narrowing is gated behind the new bleedingEdge feature toggle narrowForeachBodyNonEmpty. With it off the body scope is built exactly as before, so stable behaviour does not change. With it on, the body narrows regardless of polluteScopeWithAlwaysIterableForeach. Narrowing the body also feeds the loop-exit scope, which with the flag off must stay conservative: we must not conclude the loop always iterated. After the body pass the iterated expression's possibly-empty-ness is restored in the after-loop scope (keeping any element types the body refined), so the value variable does not become always-defined after the loop. Only the non-emptiness the narrowing introduced is stripped; an iteratee already non-empty before the loop keeps that property. This containment is partial: definedness that flows through variables the body builds (rather than through the iterated expression itself) still reaches the after-loop scope, so a few value variables can become defined where a proof of non-emptiness exists. Closes phpstan/phpstan#13312
1 parent 7e97b9e commit a4609bb

14 files changed

Lines changed: 152 additions & 7 deletions

‎conf/bleedingEdge.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ parameters:
1515
rawMessageInBaseline: true
1616
reportNestedTooWideType: false # tmp
1717
assignToByRefForeachExpr: true
18+
narrowForeachBodyNonEmpty: true
1819
curlSetOptArrayTypes: true
1920
magicDirInInclude: true
2021
checkDateIntervalConstructor: true

‎conf/config.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ parameters:
4141
rawMessageInBaseline: false
4242
reportNestedTooWideType: false
4343
assignToByRefForeachExpr: false
44+
narrowForeachBodyNonEmpty: false
4445
curlSetOptArrayTypes: false
4546
magicDirInInclude: false
4647
checkDateIntervalConstructor: false

‎conf/parametersSchema.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ parametersSchema:
4444
rawMessageInBaseline: bool()
4545
reportNestedTooWideType: bool()
4646
assignToByRefForeachExpr: bool()
47+
narrowForeachBodyNonEmpty: bool()
4748
curlSetOptArrayTypes: bool()
4849
magicDirInInclude: bool()
4950
checkDateIntervalConstructor: bool()

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 49 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;
@@ -232,6 +233,8 @@ public function __construct(
232233
private readonly bool $polluteScopeWithLoopInitialAssignments,
233234
#[AutowiredParameter]
234235
private readonly bool $polluteScopeWithAlwaysIterableForeach,
236+
#[AutowiredParameter(ref: '%featureToggles.narrowForeachBodyNonEmpty%')]
237+
private readonly bool $narrowForeachBodyNonEmpty,
235238
#[AutowiredParameter]
236239
private readonly bool $polluteScopeWithBlock,
237240
#[AutowiredParameter]
@@ -1453,6 +1456,16 @@ public function processStmtNode(
14531456
$stmt->expr,
14541457
new Array_([]),
14551458
);
1459+
// Inside the loop body the iterated expression is non-empty, so narrow it
1460+
// (list to non-empty-list, array to non-empty-array) at body entry. Under
1461+
// narrowForeachBodyNonEmpty this happens even with
1462+
// polluteScopeWithAlwaysIterableForeach off; the code below then strips the
1463+
// narrowing back out of the after-loop scope, so the "loop always iterated"
1464+
// assumption never leaks past the loop. With the toggle off the body scope
1465+
// matches the old behaviour exactly.
1466+
$bodyIterateeScope = $this->narrowForeachBodyNonEmpty || $this->polluteScopeWithAlwaysIterableForeach
1467+
? $scope->filterByTruthyValue($arrayComparisonExpr)
1468+
: $scope;
14561469
$this->callNodeCallback($nodeCallback, new InForeachNode($stmt), $scope, $storage);
14571470
$originalScope = $scope;
14581471
$bodyScope = $scope;
@@ -1475,7 +1488,7 @@ public function processStmtNode(
14751488
if ($context->isTopLevel()) {
14761489
$storage = $originalStorage->duplicate();
14771490

1478-
$originalScope = $this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope;
1491+
$originalScope = $bodyIterateeScope;
14791492
$unrolledResult = $this->tryProcessUnrolledConstantArrayForeach($stmt, $originalScope, $originalStorage, $context);
14801493
if ($unrolledResult !== null) {
14811494
$bodyScope = $unrolledResult['bodyScope'];
@@ -1486,7 +1499,7 @@ public function processStmtNode(
14861499
$count = 0;
14871500
do {
14881501
$prevScope = $bodyScope;
1489-
$bodyScope = $bodyScope->mergeWith($this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope);
1502+
$bodyScope = $bodyScope->mergeWith($bodyIterateeScope);
14901503
$storage = $originalStorage->duplicate();
14911504
$bodyScope = $this->enterForeach($bodyScope, $storage, $originalScope, $stmt, $nodeCallback);
14921505
$bodyScopeResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $bodyScope, $storage, new NoopNodeCallback(), $context->enterDeep())->filterOutLoopExitPoints();
@@ -1506,7 +1519,7 @@ public function processStmtNode(
15061519
}
15071520
}
15081521

1509-
$bodyScope = $bodyScope->mergeWith($this->polluteScopeWithAlwaysIterableForeach ? $scope->filterByTruthyValue($arrayComparisonExpr) : $scope);
1522+
$bodyScope = $bodyScope->mergeWith($bodyIterateeScope);
15101523
$storage = $originalStorage;
15111524
$bodyScope = $this->enterForeach($bodyScope, $storage, $originalScope, $stmt, $nodeCallback);
15121525
$finalPassContext = $unrolledTotalKeys !== null ? $context->enterUnrolledForeach($unrolledTotalKeys) : $context;
@@ -1654,6 +1667,39 @@ public function processStmtNode(
16541667
}
16551668

16561669
$isIterableAtLeastOnce = $exprType->isIterableAtLeastOnce();
1670+
1671+
$iterateeCertainty = $finalScope->hasExpressionType($stmt->expr);
1672+
if (
1673+
$this->narrowForeachBodyNonEmpty
1674+
&& !$this->polluteScopeWithAlwaysIterableForeach
1675+
&& !$iterateeCertainty->no()
1676+
&& !$isIterableAtLeastOnce->yes()
1677+
) {
1678+
// Keep the in-body narrowing out of the after-loop scope. With
1679+
// polluteScopeWithAlwaysIterableForeach off we cannot assume the loop ran,
1680+
// so restore the possibly-empty-ness of the iterated expression (keeping any
1681+
// element types the body refined). The loop-exit merge then builds the same
1682+
// conservative scope it would without the narrowing, and does not treat the
1683+
// value variable as always defined after the loop.
1684+
//
1685+
// Strip only the non-emptiness the narrowing added. If the iteratee was
1686+
// already non-empty before the loop ($isIterableAtLeastOnce->yes()), that
1687+
// holds regardless of the loop, so keep it. A literal like
1688+
// `foreach ([1, 2] as $v)` is not a tracked expression, so skip it.
1689+
$finalIterateeType = $finalScope->getType($stmt->expr);
1690+
if ($finalIterateeType->isArray()->yes()) {
1691+
$finalIterateeNativeType = $finalScope->getNativeType($stmt->expr);
1692+
$finalScope = $finalScope->specifyExpressionType(
1693+
$stmt->expr,
1694+
TypeCombinator::union($finalIterateeType, new ConstantArrayType([], [])),
1695+
$finalIterateeNativeType->isArray()->yes()
1696+
? TypeCombinator::union($finalIterateeNativeType, new ConstantArrayType([], []))
1697+
: $finalIterateeNativeType,
1698+
$iterateeCertainty,
1699+
);
1700+
}
1701+
}
1702+
16571703
if ($isIterableAtLeastOnce->maybe() || $exprType->isIterable()->no()) {
16581704
$finalScope = $finalScope->mergeWith($scope->filterByTruthyValue(new BooleanOr(
16591705
new BinaryOp\Identical(

‎src/Testing/RuleTestCase.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ protected function createNodeScopeResolver(): NodeScopeResolver
113113
self::getContainer()->getByType(DeepNodeCloner::class),
114114
$this->shouldPolluteScopeWithLoopInitialAssignments(),
115115
$this->shouldPolluteScopeWithAlwaysIterableForeach(),
116+
self::getContainer()->getParameter('featureToggles')['narrowForeachBodyNonEmpty'],
116117
self::getContainer()->getParameter('polluteScopeWithBlock'),
117118
[],
118119
[],

‎src/Testing/TypeInferenceTestCase.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ protected static function createNodeScopeResolver(): NodeScopeResolver
8787
self::getContainer()->getByType(DeepNodeCloner::class),
8888
$container->getParameter('polluteScopeWithLoopInitialAssignments'),
8989
$container->getParameter('polluteScopeWithAlwaysIterableForeach'),
90+
$container->getParameter('featureToggles')['narrowForeachBodyNonEmpty'],
9091
$container->getParameter('polluteScopeWithBlock'),
9192
static::getEarlyTerminatingMethodCalls(),
9293
static::getEarlyTerminatingFunctionCalls(),

‎tests/PHPStan/Analyser/AnalyserTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -829,6 +829,7 @@ private function createAnalyser(): Analyser
829829
$container->getByType(DeepNodeCloner::class),
830830
false,
831831
true,
832+
false,
832833
true,
833834
[],
834835
[],

‎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/Fiber/FiberNodeScopeResolverRuleTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ protected function createNodeScopeResolver(): NodeScopeResolver
132132
self::getContainer()->getByType(DeepNodeCloner::class),
133133
$this->shouldPolluteScopeWithLoopInitialAssignments(),
134134
$this->shouldPolluteScopeWithAlwaysIterableForeach(),
135+
self::getContainer()->getParameter('featureToggles')['narrowForeachBodyNonEmpty'],
135136
self::getContainer()->getParameter('polluteScopeWithBlock'),
136137
[],
137138
[],

‎tests/PHPStan/Analyser/Fiber/FiberNodeScopeResolverTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ protected static function createNodeScopeResolver(): NodeScopeResolver
6565
$container->getByType(DeepNodeCloner::class),
6666
$container->getParameter('polluteScopeWithLoopInitialAssignments'),
6767
$container->getParameter('polluteScopeWithAlwaysIterableForeach'),
68+
$container->getParameter('featureToggles')['narrowForeachBodyNonEmpty'],
6869
$container->getParameter('polluteScopeWithBlock'),
6970
static::getEarlyTerminatingMethodCalls(),
7071
static::getEarlyTerminatingFunctionCalls(),

0 commit comments

Comments
 (0)