Apply FunctionParameterClosureTypeExtension override to the in-function-call stack so the closure's inferred return type matches its analysed body#5991
Conversation
staabm
left a comment
There was a problem hiding this comment.
this should also fix https://phpstan.org/r/afe20725-1ad2-4a7d-8043-ed517d3499d8
|
Done. Here's a summary of how I addressed staabm's review. What the reviewer asked
What I foundI retrieved both playground snippets via the PHPStan playground API. Despite the issue text calling it "array-function style", the second link is actually the arrow-function variant of the same preg_replace_callback(
'/a|(?<b>b)/',
fn (array $match) => $match['b'] !== null ? 'aa' : 'possible?',
'abcd',
flags: PREG_UNMATCHED_AS_NULL,
);The existing PR fix ( The one gap was test coverage: the PR's Change made
Verification
Committed as |
…on-call stack so the closure's inferred return type matches its analysed body - `ClosureTypeResolver` infers a closure's return type by re-deriving the callable parameter types from the parameter on top of `inFunctionCallsStack`. That parameter was the plain declared callback parameter (e.g. `callable(array<int|string, string>): string` from the function map), even when a `*ParameterClosureTypeExtension` had overridden it (e.g. `preg_replace_callback` narrowing `$matches` to the regex shape). The body was analysed with the overridden type but the return type was inferred with the original, so branches could be considered dead during return-type inference and reachable during the actual walk, producing false `Anonymous function should return X but returns Y` errors. - Add `MutatingScope::replaceInFunctionCallStackParameterType()` which replaces the type of the parameter on top of the in-function-call stack. - In `NodeScopeResolver`, after a closure/arrow-function parameter type is overridden by a parameter closure type extension, update the scope passed to `processClosureNode`/`processArrowFunctionNode` so return-type inference reads the overridden type. This covers function, method and static-method parameter closure type extensions (all three go through the same argument-processing loop).
…ctionReturnTypeRuleTest The second playground link on phpstan/phpstan#14914 uses an arrow function without an explicit return type as the preg_replace_callback callback. That case goes through ArrowFunctionReturnTypeRule (not ClosureReturnTypeRule), so add it to bug-14914.php and assert no error there. Without the replaceInFunctionCallStackParameterType fix it reports "Anonymous function should return 'aa' but returns 'aa'|'possible?'". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
de25b6f to
b93cd6c
Compare
Summary
preg_replace_callback(and any other function/method whose callback parametertype is refined by a
*ParameterClosureTypeExtension) reported a false positiveAnonymous function should return 'aa' but returns 'possible?'for a callbackwhose body clearly returns both values:
The fix makes the closure's inferred return type use the same (extension-overridden)
parameter types that the closure body is actually analysed with.
Changes
src/Analyser/MutatingScope.php: addreplaceInFunctionCallStackParameterType(),which returns a scope whose top
inFunctionCallsStackentry carries a replacedparameter type.
src/Analyser/NodeScopeResolver.php: in both theExpr\ClosureandExpr\ArrowFunctionargument branches, after a parameter closure type extensionproduces an overriding type, update the scope passed to
processClosureNode()/processArrowFunctionNode()with that type. Because thislives in the shared argument-processing loop, the same fix applies to function,
method and static-method parameter closure type extensions.
tests/PHPStan/Rules/Functions/data/bug-14914.php+ClosureReturnTypeRuleTest::testBug14914— the reported
preg_replace_callbackreproducer (function extension).tests/PHPStan/Rules/Functions/data/closure-return-type-parameter-closure-extension.php,its
.neon, andClosureReturnTypeParameterClosureExtensionRuleTest— customfunction/method/static-method parameter closure type extensions plus a closure
and an arrow function, proving the analogous method and static-method cases were
broken the same way and are now fixed.
Root cause
A closure's return type is inferred by
ClosureTypeResolver::getClosureType(),which re-derives the callable parameter types from the parameter reflection on top
of
MutatingScope::$inFunctionCallsStack. That parameter is pushed viapushInFunctionCall()as the declared callback parameter, before anyFunctionParameterClosureTypeExtension/MethodParameterClosureTypeExtension/StaticMethodParameterClosureTypeExtensionoverride is computed. The override isonly forwarded to
processClosureNode()as$passedToTypeand used to analyse thebody, so two different parameter types were in play:
(
array<int|string, string>, where$match['b']is a non-nullstring, so the!== nullcheck is always true andreturn 'possible?'looks unreachable → thereturn type collapses to
'aa');$match['b']is'b'|null, so both branches are live andreturn 'possible?'is checked againstthe collapsed
'aa').replaceInFunctionCallStackParameterType()re-aligns the stack with the override soboth paths agree. The pattern affects every parameter closure type extension; the
fix is placed in the shared call-argument loop so function, method and
static-method extensions are all covered, and both closures and arrow functions are
handled.
Test
ClosureReturnTypeRuleTest::testBug14914reproduces the exact issue snippet andfails without the fix (
return.typeerror on thereturn 'possible?'line),passes with it.
ClosureReturnTypeParameterClosureExtensionRuleTestregisters custom function,method and static-method parameter closure type extensions that widen a callback's
array offset to be nullable; without the fix the function, method and
static-method closures each raise a false
return.typeerror, and all pass withthe fix. It also exercises an arrow function on the same code path.
Fixes phpstan/phpstan#14914