Track array value type overwrites in by-reference foreach on constant arrays#5556
Track array value type overwrites in by-reference foreach on constant arrays#5556phpstan-bot wants to merge 4 commits intophpstan:2.1.xfrom
foreach on constant arrays#5556Conversation
…without key variable (phpstan#5542)" This reverts commit 6c69c44.
1a447f6 to
c1924d6
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Merge bug-1311-constant-array and bug-1311 in a single file
Merge bug-1940 and bug-1940-with-key in a single file
Ensure the code
$a = [
[
'one' => 'one',
'two' => 'two',
],
[
'one' => 'one',
],
];
foreach ($a as &$testArray) {
$testArray['two'] = 'two';
}
$key = 'three';
foreach ($a as $offset => $testArray) {
$a[$offset][$key] = $key;
}
foreach ($a as $testArray) {
$testArray['two'];
$testArray['three'];
}
Does not produce the error Offset 'two' might not exist on array{one: 'one', two?: 'two', three: 'three'}. anymore.
VincentLanglet
left a comment
There was a problem hiding this comment.
Could this PR be simplified by improving part of MutatingScope ?
MutatingScope::enterForeach seems to already process byRef but is skipping constantArray
if ($valueByRef && $iterateeType->isArray()->yes() && $iterateeType->isConstantArray()->no())
Also MutatingScope::tryProcessUnrolledConstantArrayForeach currently has an early return for byRef...
- Merge bug-1311-constant-array.php into bug-1311.php (single file, single namespace) - Merge bug-1940-with-key.php into bug-1940.php (single file, single namespace) - Add test for constant array by-ref foreach with sub-element modification to verify no false positive about missing offset after mutations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All three review items addressed and pushed:
Both |
Remove the isConstantArray()->no() guard from the with-key IntertwinedVariableByReferenceWithExpr setup in enterForeach. The ArrayDimFetch-based intertwined expression correctly targets individual elements via the key variable, so it works for constant arrays (unlike the SetExistingOffsetValueTypeExpr path which updates all values at once and must remain guarded). This lets the intertwined mechanism automatically propagate by-ref type changes to the dim fetch for constant arrays, removing the need for the elseif ($stmt->byRef) fallback branches in the post-loop rewrite in NodeScopeResolver. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed successfully. Here's a summary of what was done: Simplification via
|
Summary
When iterating over a constant array (e.g.
[1, 2, 3]) by reference and modifying the value variable, PHPStan did not track the type change back to the array. After the loop, the array retained its original type instead of reflecting the modifications.This affected both
foreach ($arr as &$v)(without key) andforeach ($arr as $k => &$v)(with key) on constant arrays.Changes
src/Analyser/NodeScopeResolver.php:isConstantArray()->no()guard at line 1355 to allow the value type rewrite when$stmt->byRefis true. The guard existed because non-by-ref constant array foreach is handled by loop unrolling, but by-ref foreach skips unrolling (tryProcessUnrolledConstantArrayForeachreturns null for$stmt->byRef).TypeTraverser::mapcallbacks (for expr type and native type): uses$type->traverse(fn () => $newValueType)to replace all value types while preserving theConstantArrayTypestructure. This check is placed before theinstanceof ArrayTypecheck becauseConstantArrayTypeimplementsTypedirectly without extendingArrayType.$stmt->byRefandOriginalForeachValueExprabsent), use the value variable's type as the dim fetch type. This is needed because theIntertwinedVariableByReferenceWithExprmechanism (which normally propagates by-ref changes to the array dim fetch) is not set up for constant arrays.Root cause
Three interacting guards prevented constant arrays from having their value types updated after by-ref foreach:
tryProcessUnrolledConstantArrayForeachreturns null for$stmt->byRef, so loop unrolling (which normally handles constant arrays) is skippedisConstantArray()->no()condition blocked the post-loop value type rewrite for constant arraysConstantArrayTypedoesn't extendArrayType, so theinstanceof ArrayTypecheck in the TypeTraverser callback returned early without modifying itIntertwinedVariableByReferenceWithExpris only set up for non-constant arrays (guarded byisConstantArray()->no()inMutatingScope::enterForeach), so the dim fetch type didn't reflect by-ref modificationsAnalogous cases probed and found to be already correct:
foreachwithout key variable #5542)IntertwinedVariableByReferenceWithExpr)Test
Added
tests/PHPStan/Analyser/nsrt/bug-1311-constant-array.phpcovering:strlen)Fixes phpstan/phpstan#1311