Track array_walk by-ref closure parameter modifications on array type #5527
+233
−3
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace Bug14525; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| function basicArrayWalk(): void | ||
| { | ||
| $array = ['a' => 1, 'b' => 2]; | ||
| array_walk($array, function (&$value, $key): void { | ||
| $value = (string) $value; | ||
| }); | ||
| assertType("array{a: '1'|'2', b: '1'|'2'}", $array); | ||
| } | ||
|
|
||
| function arrayWalkGeneric(): void | ||
| { | ||
| /** @var array<string, int> $array */ | ||
| $array = ['a' => 1, 'b' => 2]; | ||
| array_walk($array, function (&$value, $key): void { | ||
| $value = (string) $value; | ||
| }); | ||
| assertType("array<string, lowercase-string&numeric-string&uppercase-string>", $array); | ||
| } | ||
|
|
||
| function arrayWalkNoModification(): void | ||
| { | ||
| /** @var array<string, int> $array */ | ||
| $array = ['a' => 1, 'b' => 2]; | ||
| array_walk($array, function (&$value, $key): void { | ||
| echo $value; | ||
| }); | ||
| assertType("array<string, int>", $array); | ||
| } | ||
|
|
||
| function arrayWalkConditionalModification(): void | ||
| { | ||
| /** @var array<string, int> $array */ | ||
| $array = ['a' => 1, 'b' => 2]; | ||
| array_walk($array, function (&$value, string $key): void { | ||
| if ($key === 'a') { | ||
| $value = 'modified'; | ||
| return; | ||
| } | ||
| }); | ||
| assertType("array<string, 'modified'|int>", $array); | ||
| } | ||
|
|
||
| function arrayWalkWithoutByRef(): void | ||
| { | ||
| /** @var array<string, int> $array */ | ||
| $array = ['a' => 1, 'b' => 2]; | ||
| array_walk($array, function ($value, $key): void { | ||
| $value = (string) $value; | ||
| }); | ||
| assertType("array<string, int>", $array); | ||
| } | ||
|
|
||
| function arrayWalkNonEmptyArray(): void | ||
| { | ||
| /** @var non-empty-array<string, int> $array */ | ||
| $array = ['a' => 1]; | ||
| array_walk($array, function (&$value): void { | ||
| $value = (string) $value; | ||
| }); | ||
| assertType("non-empty-array<string, lowercase-string&numeric-string&uppercase-string>", $array); | ||
| } | ||
|
|
||
| function arrayWalkList(): void | ||
| { | ||
| /** @var list<int> $list */ | ||
| $list = [1, 2, 3]; | ||
| array_walk($list, function (&$value): void { | ||
| $value = (string) $value; | ||
| }); | ||
| assertType("list<lowercase-string&numeric-string&uppercase-string>", $list); | ||
| } | ||
|
|
||
| function arrayWalkAlwaysTerminating(): void | ||
| { | ||
| /** @var array<string, int> $array */ | ||
| $array = ['a' => 1, 'b' => 2]; | ||
| array_walk($array, function (&$value): void { | ||
| $value = (string) $value; | ||
| return; | ||
| }); | ||
| assertType("array<string, lowercase-string&numeric-string&uppercase-string>", $array); | ||
| } | ||
|
|
||
| function arrayWalkNestedArray(): void | ||
| { | ||
| $array = ['a' => ['x' => 1, 'y' => 2], 'b' => ['z' => 3]]; | ||
| array_walk($array, function (&$value): void { | ||
| $value = count($value); | ||
| }); | ||
| assertType("array{a: 1|2, b: 1|2}", $array); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would fire multiple times in case of nested closures. We only want this to fire once for the top-most closure. Ideally we'd check
$scope->getAnonymousFunctionReflection()like in other places in NodeScopeResolver, but we probably do not have the anonymous function reflection to compare it against... so we probably need to get the last firing of ClosureReturnStatementsNode? Try to come up with a nice solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Two commits pushed:
Test commit: Added two nested closure test cases — one with
array_mapcallback insidearray_walk, one with a by-ref capturing closure — to verify correct type tracking with nested closures.Fix commit: Changed the
ClosureReturnStatementsNodehandler to compare$node->getClosureExpr() === $callbackArg, ensuring it only fires for thearray_walkcallback closure and ignores any nested closures within it. This is the same pattern used elsewhere inNodeScopeResolver(line 2776) wheregetAnonymousFunctionReflection()is compared to filter nested closure events.Note: I couldn't construct a test case where the nested closure issue produces observably wrong results with the current code, because the outer
ClosureReturnStatementsNodealways fires last (depth-first traversal) and overwrites the inner's values. However, thegetClosureExpr()comparison makes the code correct by construction rather than by accident of traversal order.