Track array value type overwrites in by-reference foreach without key variable#5542
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
I don't understand how it's related to the fact it's "by ref without key" since the same issue exists when a key is used
foreach ($a as $k => &$testArray) {
$testArray['two'] = 'two';
}
|
I processed this review but have nothing to report. |
| $arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes); | ||
|
|
||
| $valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType()); | ||
| } elseif ($stmt->keyVar !== null) { |
There was a problem hiding this comment.
You can keep the same if order
- if $stmt->keyVar !== null
- elseif byRefWithoutKey
There was a problem hiding this comment.
Done. Swapped the if/elseif branch order so $stmt->keyVar !== null is checked first and $byRefWithoutKey is the elseif, matching the reviewer's suggestion. All tests pass and PHPStan self-analysis is clean.
VincentLanglet
left a comment
There was a problem hiding this comment.
I don't understand why it was working for ref overwrite when a key variable is used. Please explain me.
The fact a key is used or not shouldn't impact the value
|
Now let me address the reviewer's actual question. They want to understand why the with-key case already worked but the without-key case didn't. This is an explanation question, not a code change request. Here's the answer: The reviewer's question is: why does the presence/absence of a key variable affect whether value type overwrites are tracked? The answer is that the key variable isn't used to track the key's type — it's the mechanism by which PHPStan reads back the overwritten value type from the array after the loop body executes. Here's how the with-key case works:
Without a key variable ( The fix adds a parallel path for the keyless case: instead of reading The |
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression test for issue 1311
phpstan/phpstan#1311
class HelloWorld
{
/**
* @var array<int, string>
*/
private $list = [];
public function convertList(): void
{
$temp = [1, 2, 3];
foreach ($temp as &$item) {
$item = (string) $item;
}
$this->list = $temp;
}
}
(new HelloWorld())->convertList();
|
Done. Added a non-regression test for phpstan/phpstan#1311 in |
…ey variable - Extend post-loop array type rewrite in NodeScopeResolver to handle `foreach ($arr as &$v)` (without `$key =>`) by tracking the value variable's type via `OriginalForeachValueExpr` instead of relying on `OriginalForeachKeyExpr` + `ArrayDimFetch` - When the value is always overwritten (OriginalForeachValueExpr absent in all paths), replace the array's value type with the final value variable type — matching the existing behavior for the key-based case - Handle continue exit points for the by-ref without key case with the same logic: check OriginalForeachValueExpr presence in each continue scope - Update assertions in existing tests (bug-13809, bug-14083, bug-14084, bug-14124, bug-14124b, overwritten-arrays) that were asserting the old imprecise behavior
…works The reviewer pointed out that modifying array sub-elements through a by-reference foreach with a key variable also had issues. Testing confirms this case already works correctly on the current branch — both with and without a key variable. The reviewer's example was likely tested against the stable phpstan.org release which runs an older version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per reviewer feedback, keep the original branch order so the existing keyVar path comes first and the new byRefWithoutKey path is the elseif. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verifies that by-reference foreach without key variable correctly tracks type overwrites when casting values (e.g. `$item = (string) $item`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9b3d9ad to
0c39707
Compare
|
Reverted |
…without key variable (phpstan#5542)" This reverts commit 6c69c44.
Summary
When iterating an array by reference without a key variable (
foreach ($arr as &$v) { $v = X; }), PHPStan did not track that the array's element type was being overwritten. The same construct WITH a key variable (foreach ($arr as $k => &$v) { $v = X; }) already worked correctly.This PR extends the post-loop array type rewrite mechanism in
NodeScopeResolverto also handle the by-reference-without-key case.Changes
src/Analyser/NodeScopeResolver.php: Extended the foreach post-loop array type rewrite (around line 1293-1475) to handle$stmt->byRef && $stmt->keyVar === null:$byRefWithoutKeyflag detectionOriginalForeachValueExpr(value was overwritten) instead ofOriginalForeachKeyExpr(key was not modified)ArrayDimFetch($expr, $keyVar))$stmt->keyVar !== nullto$stmt->keyVar !== null || $byRefWithoutKeyUpdated test assertions in existing files that were asserting the old imprecise behavior:
tests/PHPStan/Analyser/nsrt/overwritten-arrays.php—doFoo7:array<int, 1|string>→array<int, 1>tests/PHPStan/Analyser/nsrt/bug-13809.php—foo():list<mixed>→list<'foo'>,bar3():list<mixed>→list<'foo'|'maybe'>,baz():list<string>→list<'bar'>tests/PHPStan/Analyser/nsrt/bug-14083.php—example():list<string>→list<uppercase-string>tests/PHPStan/Analyser/nsrt/bug-14084.php—example()andexample2():list<string>→list<uppercase-string>tests/PHPStan/Rules/Variables/data/bug-14124.php—example3a():list<string>→list<uppercase-string>tests/PHPStan/Rules/Variables/data/bug-14124b.php—example3a():list<list<string>>→list<list<uppercase-string>>Root cause
The post-loop array type rewrite in
NodeScopeResolverwas gated on$stmt->keyVar !== null. This gate exists because the mechanism relies on:OriginalForeachKeyExprto verify the key wasn't modified$arr[$key]from the final scope to determine the new element typeWithout a key variable, neither of these steps is possible. The fix adds a parallel mechanism for the by-reference case: it uses
OriginalForeachValueExpr(already tracked for other purposes) to verify the value was overwritten, and reads the value variable's type directly instead of through anArrayDimFetch.Test
tests/PHPStan/Analyser/nsrt/bug-1940.php: Comprehensive regression test covering:$this->prop)intval)Fixes phpstan/phpstan#1940