Skip to content

Track array_walk by-ref closure parameter modifications on array type#5527

Merged
ondrejmirtes merged 3 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-6vyyttn
Apr 24, 2026
Merged

Track array_walk by-ref closure parameter modifications on array type#5527
ondrejmirtes merged 3 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-6vyyttn

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

Fixes phpstan/phpstan#14525

When array_walk($array, function (&$value) { $value = (string) $value; }) is called, PHPStan now tracks the by-reference modifications to $value inside the closure and updates the array's value type accordingly.

How it works

In FuncCallHandler::processExpr, when an array_walk call is detected with a closure callback that has a by-ref first parameter:

  1. The original array type is captured before processArgs
  2. A wrapper around the node callback intercepts the ClosureReturnStatementsNode emitted during closure processing
  3. The wrapper collects the by-ref parameter's type at all exit points (return statements + end-of-body scope) using StatementResult::getExitPoints() and StatementResult::getScope()
  4. After processArgs, the array type is updated via processVirtualAssign with the new value type

Key design decisions

  • Uses StatementResult::getExitPoints() instead of ClosureReturnStatementsNode::getReturnStatements() because exit point scopes are MutatingScope objects, while return statement scopes can be FiberScope objects whose getNativeType() would suspend the fiber and cause the type capture to be deferred past the point where it's needed
  • Type capture runs before forwarding to the original $nodeCallback to avoid fiber suspension from rules processing the ClosureReturnStatementsNode
  • Uses TypeTraverser::map to replace value types while preserving array structure (constant arrays, lists, non-empty arrays, etc.)
  • Only handles array_walk, not array_walk_recursive (which would need recursive leaf-value type replacement)

Supported scenarios

  • Basic value modification ($value = (string) $value)
  • Conditional modification with early return (union of modified and unmodified types)
  • No modification (closure reads but doesn't change values)
  • Non-by-ref parameter (no type change)
  • Preserves array subtypes: constant arrays, non-empty-array, list
  • Both PHPDoc and native types are tracked

Updated existing test

tests/PHPStan/Rules/Arrays/data/bug-7469.php - type of $data['languages'] after array_walk with strtolower(trim($item)) is now correctly tracked as non-empty-list<lowercase-string> instead of non-empty-list<string>.

When array_walk() is called with a closure that modifies values by
reference, PHPStan now tracks the type changes and updates the array's
value type accordingly.

Fixes phpstan/phpstan#14525
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also first verify our hypothesis with a failing test.

$arrayWalkOriginalArrayNativeType = $scope->getNativeType($arrayWalkArrayArg);

$nodeCallbackForArgs = static function (Node $node, Scope $scope) use ($nodeCallback, $firstParamName, &$arrayWalkValueTypes): void {
if ($node instanceof ClosureReturnStatementsNode) {
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Two commits pushed:

  1. Test commit: Added two nested closure test cases — one with array_map callback inside array_walk, one with a by-ref capturing closure — to verify correct type tracking with nested closures.

  2. Fix commit: Changed the ClosureReturnStatementsNode handler to compare $node->getClosureExpr() === $callbackArg, ensuring it only fires for the array_walk callback closure and ignores any nested closures within it. This is the same pattern used elsewhere in NodeScopeResolver (line 2776) where getAnonymousFunctionReflection() 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 ClosureReturnStatementsNode always fires last (depth-first traversal) and overwrites the inner's values. However, the getClosureExpr() comparison makes the code correct by construction rather than by accident of traversal order.

phpstan-bot and others added 2 commits April 24, 2026 17:07
Verify that array_walk correctly tracks by-ref parameter types
when the callback contains nested closures (e.g. array_map callback,
closure capturing variables by reference).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Compare ClosureReturnStatementsNode::getClosureExpr() against the
callback argument to ensure we only process the array_walk callback
closure, not nested closures within it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ondrejmirtes ondrejmirtes merged commit 3c7977a into phpstan:2.1.x Apr 24, 2026
655 of 659 checks passed
@ondrejmirtes ondrejmirtes deleted the create-pull-request/patch-6vyyttn branch April 24, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants