Merge all callable acceptors' parameters into NativeParameterReflection when a closure is passed to a union of callables - #6128
Conversation
| @@ -0,0 +1,84 @@ | |||
| <?php // lint >= 8.0 | |||
There was a problem hiding this comment.
crash tests should be moved into a AnalyserIntegrationTest
There was a problem hiding this comment.
this test should 1:1 reflect the snippet the user reported in https://phpstan.org/r/fa4c94b1-ab67-442d-8c89-36ea65b86346
There was a problem hiding this comment.
Addressed the review — the crash reproducer now lives in AnalyserIntegrationTest.
What changed (commit 0915dc14c):
tests/PHPStan/Analyser/nsrt/bug-15003.php→tests/PHPStan/Analyser/data/bug-15003.php, with theassertType()calls dropped (a crash test doesn't need them) and two adjustments so the file analyses cleanly at the integration test's level:InvokableRule::__invoke()got a: voidreturn type (otherwise "has no return type specified");- the two
Fieldcalls are wrapped in functions that return the result, instead of being standalone statements ("call on a separate line has no effect").
AnalyserIntegrationTest::testBug15003()added,#[RequiresPhp('>= 8.0.0')], asserting no errors.
Verified the moved test still catches the bug: with src/Analyser/NodeScopeResolver.php and src/Reflection/Native/NativeParameterReflection.php checked out from the pre-fix commit, testBug15003 errors with Call to undefined method PHPStan\Reflection\Php\ExtendedDummyParameter::union(); with the fix it passes.
nsrt/closure-passed-to-union-of-callables.php stays where it is — it asserts inferred closure parameter types (order independence, variadics, the Invokable<string> acceptor), so it's a type inference test rather than a crash test, even though its three-acceptor case exercises the same code path.
make tests (17708 tests, 97 skipped) and make phpstan are both green.
There was a problem hiding this comment.
Waiting for make tests to finish; I'll report when it lands.
…ion` when a closure is passed to a union of callables - `NodeScopeResolver::doCreateCallableParameters()` normalizes every acceptor's parameters into `NativeParameterReflection` up front, instead of only doing so for the first acceptor and for the ones it unions. A parameter that existed only in a later acceptor used to be stored raw (e.g. `ExtendedDummyParameter`, produced by generic/template resolution), so the next acceptor's merge called the `NativeParameterReflection`-only `union()` method on it and crashed the analysis with "Call to undefined method ...::union()". - The merge loop now walks `max(count($callableParameters), count($acceptorParameters))` indices instead of only the current acceptor's ones. Previously a shorter acceptor truncated the accumulated list, so the closure's inferred parameter types depended on the order of the union's members - the same closure got `string` or `mixed` for its second parameter depending on whether `callable(int): void|callable(int, string): void` or the reversed union was declared. This is what made the crash surface only after union types stopped being re-sorted in place. - Added `NativeParameterReflection::toOptional()`, used for parameters that only some of the merged signatures declare, mirroring how `ParametersAcceptorSelector::combineAcceptors()` marks parameters beyond the minimum arity as optional. - Both `createCallableParameters()` (PHPDoc types) and `createNativeCallableParameters()` (native types) go through the fixed method, and closures and arrow functions both use its result. - Probed the analogous paths: the `$args !== null` branch of the same method never calls `union()` (it only uses a single acceptor), intersections of callables already merged correctly, and `ParametersAcceptorSelector::combineAcceptors()` already keeps the longest parameter list - no changes needed there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0915dc1 to
8299962
Compare
Summary
Analysis crashed with
Call to undefined method PHPStan\Reflection\Php\ExtendedDummyParameter::union()when a closure (or arrow function) was passed to a parameter typed as a union of callables — a shape that shows up in Laravel Nova'sField::rules()and in any@phpstan-typealias that unions an invokable interface with acallable(...)signature.NodeScopeResolver::doCreateCallableParameters()merges the parameter lists of every callable acceptor of the passed-to type. It converted parameters toNativeParameterReflectiononly when seeding the list from the first acceptor and when unioning two existing parameters — a parameter that appeared for the first time in a later acceptor was stored as-is. If a further acceptor also declared that index, the merge calledunion()(aNativeParameterReflection-only method) on that raw reflection and the analysis died.The same loop also only iterated the current acceptor's parameters, so a shorter acceptor silently truncated everything accumulated so far, making the closure's inferred parameter types depend on the order of the union's members.
Changes
src/Analyser/NodeScopeResolver.php—doCreateCallableParameters():NativeParameterReflectiononce, before any merging, so the accumulated list is homogeneous;max(count($callableParameters), count($acceptorParameters))indices, so parameters declared by only some of the signatures survive instead of being dropped by whichever acceptor happens to come last.src/Reflection/Native/NativeParameterReflection.php— addedtoOptional(), used for parameters that not all merged signatures declare.createNativeCallableParameters()(native types) andcreateCallableParameters()(PHPDoc types) both delegate to the fixed method, and both closures and arrow functions consume its result — all covered by the one change, and all four combinations are tested.$args !== nullbranch of the same method keeps rawParameterReflectioninstances too, but only ever for a single acceptor and never callsunion()on them — not broken, left alone.(callable(int): void)&(callable(int, string): void)) go through the same merge; they were already correct before and after.ParametersAcceptorSelector::combineAcceptors()performs the same kind of merge for function/method variants; it already keeps the longest parameter list and marks parameters beyond the minimum arity as optional — no fix needed, and its optionality rule is whattoOptional()mirrors.Root cause
A heterogeneous accumulator: the merge assumed
$callableParametersonly ever containedNativeParameterReflection, but one of its three write sites wrote whatever the acceptor handed over. Acceptors whose parameters areExtendedDummyParameter(produced byResolvedFunctionVariantWithOriginalwhen resolving generics, and byParametersAcceptorSelectorwhen combining variants) therefore leaked into the list, and the nextunion()call fataled.The crash needed three or more acceptors with a growing arity, which is why it only appeared for large real-world unions. It became reachable in 2.2.6 because
UnionType::getSortedTypes()stopped sorting$this->typesin place (d6aeed4) — the acceptor order changed, and this merge is order-sensitive. The second part of the fix removes that order sensitivity, so a union's declaration order no longer decides how many parameters the closure gets typed:Both now infer
string.Test
tests/PHPStan/Analyser/nsrt/bug-15003.php— both playground reproducers from the issue (the@phpstan-import-typeunion with an invokable interface, and the Laravel-Nova-shapedField::rules()/creationRules()). Without the fix the file aborts withCall to undefined method ...::union().tests/PHPStan/Analyser/nsrt/closure-passed-to-union-of-callables.php— the order-independence and minimal-crash cases:callable(int, string): void|callable(int): voidand the reversed union infer the same closure parameter types (previouslymixedvsstring), for both closures and arrow functions;callable(int...): void|callable(string, string): voidand its reverse likewise;callable(int): void|Invokable<string>|callable(bool, float): void— the minimal three-acceptor crash reproducer, where the generic__invokesupplies theExtendedDummyParameter;make testsandmake phpstanare green. (make name-collisionfails ontests/PHPStan/Build/data/final-class-rule-pipe.phpon this PHP version, both with and without these changes.)Fixes phpstan/phpstan#15003