Resolve intrinsic argument overrides by parameter position instead of the order arguments are written - #6488
phpstan-bot wants to merge 3 commits into
Conversation
… the order arguments are written * Added `PHPStan\Parser\ArgumentPositionHelper::getArgsByPosition()`, which maps a call's arguments onto the positions of the callee's parameters, honouring named arguments written out of order. * Every intrinsic arg visitor (`ArrayFilterArgVisitor`, `ArrayMapArgVisitor`, `ArrayWalkArgVisitor`, `ArrayFindArgVisitor`, `ImplodeArgVisitor`, `CurlSetOptArgVisitor`, `CurlSetOptArrayArgVisitor`, `ClosureBindArgVisitor`, `ClosureBindToVarVisitor`) now declares a `PARAMETER_NAMES` constant and marks the argument that actually fills the parameter it cares about, instead of the argument that happens to be written first. * `ParametersAcceptorSelector::applyIntrinsicArgOverrides()` reads its sibling arguments through the same mapping, so the array / option / callback / closure argument is found wherever it is written. * `ArrayMapArgVisitor` now attaches `arrayMapArgs` to the callback argument's value in all argument orders, which is where `ContextualClosureParameterResolver` and `ScopeOps::nodeKey()` expect it. * Fixed the same order-dependence for `array_map`, `array_walk`, `array_find`/`array_any`/`array_all`/`array_find_key`, `curl_setopt`, `curl_setopt_array` and `Closure::bind`; `Closure::bindTo` and `implode` were probed and were already order-independent, but go through the shared mapping now too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| * | ||
| * @internal | ||
| */ | ||
| final class ArgumentPositionHelper |
There was a problem hiding this comment.
How is it different from the ArgumentsNormalizer and his method ?
Is there some deduplication to do ?
At least the getArgsByPosition method could be in ArgumentsNormalizer.
There was a problem hiding this comment.
Addressed and pushed as f7dffae91.
What changed: ArgumentPositionHelper is gone; getArgsByPosition() now lives on ArgumentsNormalizer (marked @internal), and the "does this call use named arguments" scan that both it and reorderArgs() were doing inline is now a shared private hasNamedArgs(). All nine visitors plus ParametersAcceptorSelector call through the new home.
On the overlap with reorderArgs(): they're close cousins but can't collapse into one, for two reasons:
reorderArgs()takes aParametersAcceptor. The parser visitors run during parsing, before any reflection exists, so they can't get one — that's why the callers spell out the parameter names themselves via each visitor'sPARAMETER_NAMES.reorderArgs()returns copies of theArgnodes (newArgobjects carryingORIGINAL_ARG_ATTRIBUTE, names stripped). The visitors need the original objects, because setting an attribute on a copy would never reach the AST that gets analysed. It also does things this use has no need for: variadic handling, filling holes with default values, appending unmatched named args, and returningnullon shapes it can't normalize.
I wrote that distinction into the method's PHPDoc so the next reader doesn't have to re-derive it. Beyond the hasNamedArgs() extraction there wasn't more to deduplicate without making reorderArgs() (which is @api and on the hot path) route through a weaker primitive.
make tests (21958 tests), make phpstan and make cs are all green.
ArgumentPositionHelper was a separate class for what is argument reordering logic, which already has a home. Move the method there and share the "does this call use named arguments" check with reorderArgs(), which was scanning for it inline. The two remain distinct: reorderArgs() needs a ParametersAcceptor and produces copies of the Arg nodes, while getArgsByPosition() takes the parameter names spelled out by the caller, so it also works in the parser visitors where no reflection is available yet, and returns the original Arg objects so the attributes those visitors set land on the analysed AST. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| * returns the original Arg objects instead of copies, which is what makes | ||
| * the attributes those visitors set visible on the analysed AST. | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
I'm not sure this has to be internal ; someone writing a Visitor could need it
There was a problem hiding this comment.
Addressed and pushed as 17e5fce29.
Dropped the @internal from getArgsByPosition() (src/Analyser/ArgumentsNormalizer.php:438). ArgumentsNormalizer itself carries a class-level @api and none of its other public methods tag themselves individually, so removing the line is all that's needed — the method is now covered by the same BC promise as reorderArgs() and friends, and a third-party visitor facing the same named-argument problem can call it.
make tests (21958 tests, 96 skipped), make phpstan and make cs are green.
The class is already marked @api, so the @internal on this one method was the odd one out. Visitors written outside phpstan-src face the same named-argument problem the intrinsic arg visitors do, so let them use it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
array_filter(callback: $fn, array: $arr)reported a false positiveParameter $callback of function array_filter expects (callable(mixed): bool)|null, Closure(string): bool given., while the same call written asarray_filter(array: $arr, callback: $fn)analysed cleanly.The callback's expected signature is not in the function map - it is synthesized at analysis time from the type of the array argument. That synthesis looked the array argument up as
$args[0], i.e. by its position in the source, which named arguments are free to change. The same holds for every other intrinsic argument override PHPStan applies.Changes
PHPStan\Parser\ArgumentPositionHelper::getArgsByPosition()(src/Parser/ArgumentPositionHelper.php) - maps a call's arguments onto the positions of the callee's parameters. It is a no-op (returns the arguments unchanged) when no named arguments are in play, so the hot path is unaffected.src/Parser/gained aPARAMETER_NAMESconstant and now marks the argument that fills the parameter it cares about:ArrayFilterArgVisitor,ArrayWalkArgVisitor,ArrayFindArgVisitor,CurlSetOptArgVisitor- the first parameterCurlSetOptArrayArgVisitor- theoptionsparameterClosureBindArgVisitor- theclosureparameter,ClosureBindToVarVisitor- thenewThisparameterImplodeArgVisitor- theseparatorparameter, falling back toarrayforimplode(array: $a)ArrayMapArgVisitor- thecallbackargument's value (previously the source-first argument's value, which is not the closure once the arguments are reordered;ContextualClosureParameterResolverandScopeOps::nodeKey()both look the attribute up on the closure node)ParametersAcceptorSelector::applyIntrinsicArgOverrides()(src/Reflection/ParametersAcceptorSelector.php) reads all of its sibling arguments ($args[0],$args[1],$args[2]) through the same mapping instead of indexing the call's arguments directly.Analogous cases fixed alongside the reported one, each with its own failing test:
array_filter(callback: …, array: …)callable(mixed): bool(reported bug)array_filter(mode: …, callback: …, array: …)ARRAY_FILTER_USE_KEY/ARRAY_FILTER_USE_BOTHnot honoured, callback typedcallable(mixed): boolarray_map(array: …, callback: …)arrayMapArgsattached to the array expression rather than the callbackarray_walk(callback: …, array: …)callable(mixed, mixed): mixedarray_walk(arg: …, callback: …, array: …)array_find/array_any/array_all/array_find_keywith reversed named argumentscallable(mixed, mixed): boolcurl_setopt(option: …, handle: …, value: …)CURLOPT_*value type not narrowed (false negative)curl_setopt_array(options: …, handle: …)Closure::bind(newThis: …, closure: …)@param-closure-thisnot enforced (false negative)Probed and found already order-independent (they go through the shared mapping now, but their behaviour is unchanged):
Closure::bindTowithnewScope:written beforenewThis:,implode/joinin every named-argument order, and the closure parameter type inference performed by theFunctionParameterClosureTypeExtensions (which already resolved named arguments correctly).Root cause
A parser visitor tags one argument of a well-known call with an attribute;
ParametersAcceptorSelector::applyIntrinsicArgOverrides()then detects that attribute and reads the other arguments by index to synthesize a more precise parameter type (the callback signature for the array functions, theCURLOPT_*value type for curl, the@param-closure-thistype forClosure::bind).Both halves assumed the source position of an argument equals the position of the parameter it fills. Named arguments break that assumption in two different ways depending on the caller:
ParametersAcceptorSelector::selectFromArgs()passes the arguments as written, so the marker was found but the sibling reads picked up the wrong arguments.ArgumentsHandlerpasses arguments already reordered byArgumentsNormalizer, so the marker travelled with the argument it was set on and ended up at the wrong index, and the override was skipped entirely.Resolving both the marking and the reads through a signature-position mapping makes the two halves agree in either case, and makes the analysis independent of the order named arguments are written in.
Test
tests/PHPStan/Rules/Functions/data/bug-15195.php- the reproducer from the issue's playground link verbatim, expected to analyse clean (CallToFunctionParametersRuleTest::testBug15195, with explicit/implicit mixed checks on to match the playground's level 10).tests/PHPStan/Rules/Functions/data/named-arguments-order-intrinsic.php-array_filter(plain,ARRAY_FILTER_USE_KEY,ARRAY_FILTER_USE_BOTH),array_map,array_walk(with and withoutarg:),curl_setopt,curl_setopt_arrayandimplodewith named arguments in reversed order; each construct has a correct call that must stay clean and a deliberately wrong one that must be reported.tests/PHPStan/Rules/Functions/data/named-arguments-order-array-find.php- the same forarray_find,array_find_key,array_anyandarray_all(PHP 8.4).tests/PHPStan/Rules/Methods/data/closure-bind-param-closure-this-named-args.phpand.../closure-bind-to-param-closure-this-named-args.php-@param-closure-thisenforcement forClosure::bind()andClosure::bindTo()in both named-argument orders.All of these were confirmed to fail before the fix (false positives on the correct calls, missing errors on the wrong ones) and pass after it.
make tests,make phpstanandmake csare green.Fixes phpstan/phpstan#15195
🤖 Generated with Claude Code