Skip to content

Commit d883cd8

Browse files
phpstan-botVincentLangletclaude
authored
Track array value type overwrites in by-reference foreach without key variable (#5542)
Co-authored-by: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 95e0784 commit d883cd8

10 files changed

Lines changed: 376 additions & 52 deletions

File tree

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 88 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,19 +1294,41 @@ public function processStmtNode(
12941294

12951295
$originalKeyVarExpr = null;
12961296
$continueExitPointHasUnoriginalKeyType = false;
1297+
$byRefWithoutKey = $stmt->byRef
1298+
&& $stmt->keyVar === null
1299+
&& $stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name);
1300+
12971301
if ($stmt->keyVar instanceof Variable && is_string($stmt->keyVar->name)) {
12981302
$originalKeyVarExpr = new OriginalForeachKeyExpr($stmt->keyVar->name);
12991303
if ($finalScope->hasExpressionType($originalKeyVarExpr)->yes()) {
13001304
$scopesWithIterableValueType[] = $finalScope;
13011305
} else {
13021306
$continueExitPointHasUnoriginalKeyType = true;
13031307
}
1308+
} elseif ($byRefWithoutKey) {
1309+
$originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name);
1310+
if (!$finalScope->hasExpressionType($originalValueExpr)->yes()) {
1311+
$scopesWithIterableValueType[] = $finalScope;
1312+
} else {
1313+
$continueExitPointHasUnoriginalKeyType = true;
1314+
}
13041315
}
13051316

13061317
foreach ($finalScopeResult->getExitPointsByType(Continue_::class) as $continueExitPoint) {
13071318
$continueScope = $continueExitPoint->getScope();
13081319
$finalScope = $continueScope->mergeWith($finalScope);
1309-
if ($originalKeyVarExpr === null || !$continueScope->hasExpressionType($originalKeyVarExpr)->yes()) {
1320+
if ($originalKeyVarExpr !== null) {
1321+
if (!$continueScope->hasExpressionType($originalKeyVarExpr)->yes()) {
1322+
$continueExitPointHasUnoriginalKeyType = true;
1323+
continue;
1324+
}
1325+
} elseif ($byRefWithoutKey) {
1326+
$originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name);
1327+
if ($continueScope->hasExpressionType($originalValueExpr)->yes()) {
1328+
$continueExitPointHasUnoriginalKeyType = true;
1329+
continue;
1330+
}
1331+
} else {
13101332
$continueExitPointHasUnoriginalKeyType = true;
13111333
continue;
13121334
}
@@ -1327,58 +1349,82 @@ public function processStmtNode(
13271349
count($breakExitPoints) === 0
13281350
&& count($scopesWithIterableValueType) > 0
13291351
&& !$continueExitPointHasUnoriginalKeyType
1330-
&& $stmt->keyVar !== null
1352+
&& ($stmt->keyVar !== null || $byRefWithoutKey)
13311353
&& (!$hasExpr->no() || !$stmt->expr instanceof Variable)
13321354
&& $exprType->isArray()->yes()
13331355
&& $exprType->isConstantArray()->no()
13341356
) {
1335-
$arrayExprDimFetch = new ArrayDimFetch($stmt->expr, $stmt->keyVar);
1336-
$originalValueExpr = null;
1337-
if ($stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name)) {
1338-
$originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name);
1339-
}
1340-
$arrayDimFetchLoopTypes = [];
1341-
$keyLoopTypes = [];
1342-
foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) {
1343-
$dimFetchType = $scopeWithIterableValueType->getType($arrayExprDimFetch);
1344-
// Condition-based narrowings like `is_string($type)` apply to the value
1345-
// variable but not automatically to the array dim fetch, even though the
1346-
// two describe the same element for a given iteration. If the value var
1347-
// hasn't been reassigned (OriginalForeachValueExpr still tracked) we use
1348-
// the narrowed value-var type in place of the broader dim fetch type so
1349-
// the loop's final array rewrite below picks up the sharper element type.
1350-
if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) {
1351-
$valueVarType = $scopeWithIterableValueType->getType($stmt->valueVar);
1352-
if ($dimFetchType->isSuperTypeOf($valueVarType)->yes()) {
1353-
$dimFetchType = $valueVarType;
1357+
$nativeExprType = $scope->getNativeType($stmt->expr);
1358+
$arrayDimFetchLoopType = $exprType->getIterableValueType();
1359+
$arrayDimFetchLoopNativeType = $nativeExprType->getIterableValueType();
1360+
$keyLoopType = $exprType->getIterableKeyType();
1361+
$keyLoopNativeType = $nativeExprType->getIterableKeyType();
1362+
$valueTypeChanged = false;
1363+
$keyTypeChanged = false;
1364+
1365+
if ($stmt->keyVar !== null) {
1366+
$arrayExprDimFetch = new ArrayDimFetch($stmt->expr, $stmt->keyVar);
1367+
$originalValueExpr = null;
1368+
if ($stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name)) {
1369+
$originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name);
1370+
}
1371+
$arrayDimFetchLoopTypes = [];
1372+
$keyLoopTypes = [];
1373+
foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) {
1374+
$dimFetchType = $scopeWithIterableValueType->getType($arrayExprDimFetch);
1375+
// Condition-based narrowings like `is_string($type)` apply to the value
1376+
// variable but not automatically to the array dim fetch, even though the
1377+
// two describe the same element for a given iteration. If the value var
1378+
// hasn't been reassigned (OriginalForeachValueExpr still tracked) we use
1379+
// the narrowed value-var type in place of the broader dim fetch type so
1380+
// the loop's final array rewrite below picks up the sharper element type.
1381+
if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) {
1382+
$valueVarType = $scopeWithIterableValueType->getType($stmt->valueVar);
1383+
if ($dimFetchType->isSuperTypeOf($valueVarType)->yes()) {
1384+
$dimFetchType = $valueVarType;
1385+
}
13541386
}
1387+
$arrayDimFetchLoopTypes[] = $dimFetchType;
1388+
$keyLoopTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar);
13551389
}
1356-
$arrayDimFetchLoopTypes[] = $dimFetchType;
1357-
$keyLoopTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar);
1358-
}
1359-
1360-
$arrayDimFetchLoopType = TypeCombinator::union(...$arrayDimFetchLoopTypes);
1361-
$keyLoopType = TypeCombinator::union(...$keyLoopTypes);
13621390

1363-
$arrayDimFetchLoopNativeTypes = [];
1364-
$keyLoopNativeTypes = [];
1365-
foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) {
1366-
$dimFetchNativeType = $scopeWithIterableValueType->getNativeType($arrayExprDimFetch);
1367-
if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) {
1368-
$valueVarNativeType = $scopeWithIterableValueType->getNativeType($stmt->valueVar);
1369-
if ($dimFetchNativeType->isSuperTypeOf($valueVarNativeType)->yes()) {
1370-
$dimFetchNativeType = $valueVarNativeType;
1391+
$arrayDimFetchLoopType = TypeCombinator::union(...$arrayDimFetchLoopTypes);
1392+
$keyLoopType = TypeCombinator::union(...$keyLoopTypes);
1393+
1394+
$arrayDimFetchLoopNativeTypes = [];
1395+
$keyLoopNativeTypes = [];
1396+
foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) {
1397+
$dimFetchNativeType = $scopeWithIterableValueType->getNativeType($arrayExprDimFetch);
1398+
if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) {
1399+
$valueVarNativeType = $scopeWithIterableValueType->getNativeType($stmt->valueVar);
1400+
if ($dimFetchNativeType->isSuperTypeOf($valueVarNativeType)->yes()) {
1401+
$dimFetchNativeType = $valueVarNativeType;
1402+
}
13711403
}
1404+
$arrayDimFetchLoopNativeTypes[] = $dimFetchNativeType;
1405+
$keyLoopNativeTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar);
13721406
}
1373-
$arrayDimFetchLoopNativeTypes[] = $dimFetchNativeType;
1374-
$keyLoopNativeTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar);
1375-
}
13761407

1377-
$arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes);
1378-
$keyLoopNativeType = TypeCombinator::union(...$keyLoopNativeTypes);
1408+
$arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes);
1409+
$keyLoopNativeType = TypeCombinator::union(...$keyLoopNativeTypes);
1410+
1411+
$valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType());
1412+
$keyTypeChanged = !$keyLoopType->equals($exprType->getIterableKeyType());
1413+
} elseif ($byRefWithoutKey) {
1414+
$arrayDimFetchLoopTypes = [];
1415+
foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) {
1416+
$arrayDimFetchLoopTypes[] = $scopeWithIterableValueType->getType($stmt->valueVar);
1417+
}
1418+
$arrayDimFetchLoopType = TypeCombinator::union(...$arrayDimFetchLoopTypes);
1419+
1420+
$arrayDimFetchLoopNativeTypes = [];
1421+
foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) {
1422+
$arrayDimFetchLoopNativeTypes[] = $scopeWithIterableValueType->getNativeType($stmt->valueVar);
1423+
}
1424+
$arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes);
13791425

1380-
$valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType());
1381-
$keyTypeChanged = !$keyLoopType->equals($exprType->getIterableKeyType());
1426+
$valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType());
1427+
}
13821428

13831429
if ($valueTypeChanged || $keyTypeChanged) {
13841430
$newExprType = TypeTraverser::map($exprType, static function (Type $type, callable $traverse) use ($arrayDimFetchLoopType, $keyLoopType, $valueTypeChanged, $keyTypeChanged): Type {
@@ -1395,7 +1441,6 @@ public function processStmtNode(
13951441
$valueTypeChanged ? $arrayDimFetchLoopType : $type->getIterableValueType(),
13961442
);
13971443
});
1398-
$nativeExprType = $scope->getNativeType($stmt->expr);
13991444
$newExprNativeType = TypeTraverser::map($nativeExprType, static function (Type $type, callable $traverse) use ($arrayDimFetchLoopNativeType, $keyLoopNativeType, $valueTypeChanged, $keyTypeChanged): Type {
14001445
if ($type instanceof UnionType || $type instanceof IntersectionType) {
14011446
return $traverse($type);

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
namespace Bug1311;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
class HelloWorld
8+
{
9+
/**
10+
* @var array<int, string>
11+
*/
12+
private $list = [];
13+
14+
/**
15+
* @param array<int, int> $temp
16+
*/
17+
public function convertList(array $temp): void
18+
{
19+
foreach ($temp as &$item) {
20+
$item = (string) $item;
21+
}
22+
23+
assertType('array<int, lowercase-string&numeric-string&uppercase-string>', $temp);
24+
25+
$this->list = $temp;
26+
}
27+
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ function foo(array $list): void
1313
$value = 'foo';
1414
}
1515

16-
assertType('list<mixed>', $list);
16+
assertType("list<'foo'>", $list);
1717
}
1818

1919
/**
@@ -56,7 +56,7 @@ function bar3(array $list): void
5656
}
5757
}
5858

59-
assertType("list<mixed>", $list); // could be list<'foo'|'maybe'>
59+
assertType("list<'foo'|'maybe'>", $list);
6060
}
6161

6262
/**
@@ -68,7 +68,7 @@ function baz(array $list): void
6868
$value = 'bar';
6969
}
7070

71-
assertType('list<string>', $list);
71+
assertType("list<'bar'>", $list);
7272
}
7373

7474
/**

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ function example(array $convert): void {
1313
foreach ($convert as &$item) {
1414
$item = strtoupper($item);
1515
}
16-
assertType('list<string>', $convert);
16+
assertType('list<uppercase-string>', $convert);
1717
}
1818

1919
/**

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ function example(array $convert): void
1616
$val = strtoupper($val); // https://github.com/phpstan/phpstan/issues/14083
1717
}
1818
}
19-
assertType('array<string, list<string>>', $convert);
19+
assertType('array<string, list<uppercase-string>>', $convert);
2020
}
2121

2222
/**
@@ -29,7 +29,7 @@ function example2(array $convert): void
2929
$inner[$key] = strtoupper($val);
3030
}
3131
}
32-
assertType('array<string, list<string>>', $convert);
32+
assertType('array<string, list<uppercase-string>>', $convert);
3333
}
3434

3535
/**

‎tests/PHPStan/Analyser/nsrt/bug-1940-with-key.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php
2+
3+
namespace Bug1940WithKey;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
/**
8+
* Reviewer's exact example: by-ref with key, modifying sub-element.
9+
* This already worked before this PR.
10+
* @param list<array{one: string}> $a
11+
*/
12+
function byRefWithKeyModifySubElement(array $a): void
13+
{
14+
foreach ($a as $k => &$testArray) {
15+
$testArray['two'] = 'two';
16+
}
17+
18+
assertType("list<array{one: string, two: 'two'}>", $a);
19+
}
20+
21+
/**
22+
* By-ref WITHOUT key, modifying sub-element.
23+
* Parallel case to the reviewer's example but without key variable.
24+
* @param list<array{one: string}> $a
25+
*/
26+
function byRefWithoutKeyModifySubElement(array $a): void
27+
{
28+
foreach ($a as &$testArray) {
29+
$testArray['two'] = 'two';
30+
}
31+
32+
assertType("list<array{one: string, two: 'two'}>", $a);
33+
}
34+
35+
/**
36+
* By-ref with key, direct overwrite (already worked before this PR)
37+
* @param array<int, string> $arr
38+
*/
39+
function byRefWithKeyDirectOverwrite(array $arr): void
40+
{
41+
foreach ($arr as $k => &$v) {
42+
$v = 1;
43+
}
44+
45+
assertType('array<int, 1>', $arr);
46+
}
47+
48+
/**
49+
* By-ref without key, direct overwrite (this PR's main fix)
50+
* @param array<int, string> $arr
51+
*/
52+
function byRefWithoutKeyDirectOverwrite(array $arr): void
53+
{
54+
foreach ($arr as &$v) {
55+
$v = 1;
56+
}
57+
58+
assertType('array<int, 1>', $arr);
59+
}

0 commit comments

Comments
 (0)