Do not use callable parameter types as native types for closure and arrow function parameters#5632
Conversation
…rrow function parameters - In `enterAnonymousFunctionWithoutReflection` and `enterArrowFunctionWithoutReflection`, the callable parameter types (derived from calling context, often via PHPDoc/generics) were used as both expression types AND native types for closure parameters - Now only the declared PHP type hint is used for native types, while the intersected callable parameter type is used for expression types (PHPDoc-aware) - This matches the behavior of `foreach` which correctly distinguishes native vs PHPDoc iterable value types - Fixes false positives from `function.alreadyNarrowedType` when `treatPhpDocTypesAsCertain` is false and callbacks are passed to array_map, array_filter, usort, etc.
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression test for phpstan/phpstan#5271
Add a nsrt test for the constant array
$undefined_curl_functions = array_filter(
[
'curl_multi_add_handle',
'curl_multi_exec',
'curl_multi_init',
],
static function( $function_name ) {
assertType(..., $function_name);
assertNativeType(..., $function_name);
return true;
}
);
Non-regression test for phpstan/phpstan#5271. Verifies that when a constant literal array is passed to array_filter, the callback parameter gets the correct PHPDoc type (union of literal strings) and native type (mixed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
| ], | ||
| static function ($function_name) { | ||
| assertType("'curl_multi_add_handle'|'curl_multi_exec'|'curl_multi_init'", $function_name); | ||
| assertNativeType('mixed', $function_name); |
There was a problem hiding this comment.
why the native type is not
'curl_multi_add_handle'|'curl_multi_exec'|'curl_multi_init'
in this case ?
This should be consistent with
$a = [
'curl_multi_add_handle',
'curl_multi_exec',
'curl_multi_init',
];
foreach ($a as $b) {
\PHPStan\Testing\assertNativeType("'curl_multi_add_handle'|'curl_multi_exec'|'curl_multi_init'", $b);
}
There was a problem hiding this comment.
Pushed successfully. Here's a summary of the changes:
Problem: The reviewer pointed out that for constant arrays passed to array_filter, the native type of the callback parameter should be the literal string union (consistent with foreach), not mixed. The previous fix was too conservative — it always used only the declared PHP type hint as the native type, ignoring callable parameter types entirely.
Solution: Track native callable parameters separately from PHPDoc-enriched callable parameters:
-
ArrayFilterArgVisitor— Extended to attach the array argument expression to the callback closure (for the default mode case without a 3rd$modeargument), enabling native type computation. -
NodeScopeResolver::createNativeCallableParameters()— New method that computes native callable parameters from:ArrayMapArgVisitorattribute →$scope->getNativeType(arrayArg)->getIterableValueType()ArrayFilterArgVisitor::CALLBACK_ATTRIBUTE_NAMEattribute →$scope->getNativeType(arrayArg)->getIterableValueType()
-
MutatingScope— Added$nativeCallableParametersparameter toenterAnonymousFunction,enterAnonymousFunctionWithoutReflection,enterArrowFunction, andenterArrowFunctionWithoutReflection. When present, native callable parameters are used for native type computation (intersected with the declared PHP type). Whennull, the declared PHP type is used alone. -
ClosureTypeResolverandNodeScopeResolver— Updated all callers to compute and pass native callable parameters. -
Tests — Updated NSRT test to assert correct native types for constant arrays (
'curl_multi_add_handle'|'curl_multi_exec'|'curl_multi_init'), and addedarray_mapandforeachconstant array tests for consistency verification.
…ow functions When callable parameter types come from native type information (e.g., constant arrays), they should be reflected in the native type of the closure/arrow function parameter. Previously, the fix always used only the declared PHP type hint as the native type, which was too conservative for constant arrays. This adds a $nativeCallableParameters parameter to enterAnonymousFunction, enterAnonymousFunctionWithoutReflection, enterArrowFunction, and enterArrowFunctionWithoutReflection. The native callable parameters are computed from $scope->getNativeType() for array_map arguments and from the array argument's native type for array_filter callbacks. This ensures consistency with foreach: for constant arrays, the native type is the literal string union, while for PHPDoc-typed arrays, the native type remains mixed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
Do not default to null for the param nativeCallableParameters this will simplify the code
All callers already pass the argument explicitly, so the default is unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Removed the |
| $expressionTypes[$paramExprString] = $holder; | ||
| $nativeTypes[$paramExprString] = $holder; | ||
| $nativeParameterType = $declaredParameterType; | ||
| if ($nativeCallableParameters !== null) { |
There was a problem hiding this comment.
This is a duplicate of the whole logic
if ($callableParameters !== null) {
There is something to refactor
There was a problem hiding this comment.
Done. Extracted the duplicated callable parameter resolution logic into a private resolveCallableParameterType() helper method, eliminating the code duplication in both enterAnonymousFunctionWithoutReflection and enterArrowFunctionWithoutReflection. The four duplicated if/elseif/else blocks are now two-line calls each. Tests and make phpstan both pass.
| } | ||
|
|
||
| $nativeParameterType = $declaredParameterType; | ||
| if ($nativeCallableParameters !== null) { |
There was a problem hiding this comment.
Same it's a duplicate of the whole f ($callableParameters !== null) {
there is something to refactor
…ter logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
Merge both bug11014 into a single file use for both nsrt and rule test.
And use // lint >= 8.0 on it
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Merged the two bug-11014 test files into a single |
These methods are part of the public API for extension developers, so the new parameter must have a default value to avoid breaking existing callers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The rector changes is expected since they are using native type |
| } | ||
| } | ||
| } | ||
| $nativeCallableParameters = $this->nodeScopeResolver->createNativeCallableParameters($scope, $expr); |
There was a problem hiding this comment.
Why the logic for nativeCallableParameters is not the same than the one for $callableParameters ?
We ends up with a logic which seems hardcoded to only support array_map and array_filter
There was a problem hiding this comment.
Done. Here's a summary of the changes made to address the review:
Problem: The reviewer pointed out that createNativeCallableParameters in ClosureTypeResolver used completely different logic from the $callableParameters computation — it was hardcoded to only support array_map and array_filter.
Changes:
-
NodeScopeResolver: Extracted shared callable parameter resolution logic intodoCreateCallableParameters()that accepts a type-getter closure. BothcreateCallableParameters()(usinggetType) andcreateNativeCallableParameters()(usinggetNativeType) delegate to it. The newcreateNativeCallableParametersaccepts the same$argsand$nativePassedToTypeparameters ascreateCallableParameters, with attribute-based fallback for array_map/array_filter where the general mechanism can't derive native types from template resolution. -
ClosureTypeResolver: Computes$nativeCallableParametersfollowing the same 4-case structure as$callableParameters(array_map, array_filter, immediately-invoked, inFunctionCallsStack), usinggetNativeTypeinstead ofgetTypein each case. -
processClosureNode/processArrowFunctionNode: Added$nativePassedToTypeparameter, passed fromprocessArgsvia$parameterNativeType.
Make createNativeCallableParameters follow the same structure as createCallableParameters by extracting shared logic into a doCreateCallableParameters helper that accepts a type-getter closure. In ClosureTypeResolver, compute $nativeCallableParameters following the same case structure as $callableParameters (array_map, array_filter, immediately-invoked, inFunctionCallsStack) instead of delegating to a separate method with hardcoded checks. Pass $nativePassedToType through processClosureNode and processArrowFunctionNode so that the general callable parameter resolution mechanism works for native types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| $callableParameters[] = new DummyParameter('item', $scope->getType($funcCallArg->value)->getIterableValueType(), optional: false, passedByReference: PassedByReference::createNo(), variadic: false, defaultValue: null); | ||
| $nativeCallableParameters[] = new DummyParameter('item', $scope->getNativeType($funcCallArg->value)->getIterableValueType(), optional: false, passedByReference: PassedByReference::createNo(), variadic: false, defaultValue: null); | ||
| } | ||
| } elseif ($arrayFilterArrayArg !== null) { |
There was a problem hiding this comment.
Why the special if for arrayFilterArrayArg is needed ? Can't it rely on the immediatelyInvokedArgs condition ? (and maybe arrayMapArgs too ?)
There was a problem hiding this comment.
You're out of extra usage · resets 10:20pm (UTC)
| [, $inParameter] = $scope->inFunctionCallsStack[$inFunctionCallsStackCount - 1]; | ||
| if ($inParameter !== null) { | ||
| $callableParameters = $this->nodeScopeResolver->createCallableParameters($scope, $expr, null, $inParameter->getType()); | ||
| $nativePassedToType = $inParameter instanceof ExtendedParameterReflection ? $inParameter->getNativeType() : $inParameter->getType(); |
There was a problem hiding this comment.
no needed for nativePassedToType var, use it directly in the createNativeCallableParameters call
| * @param Node\Arg[]|null $args | ||
| * @return ParameterReflection[]|null | ||
| */ | ||
| public function createNativeCallableParameters(Scope $scope, Expr $closureExpr, ?array $args, ?Type $nativePassedToType): ?array |
There was a problem hiding this comment.
Why can't createNativeCallableParameters just rely on doCreateCallableParameters call like createCallableParameters ?
Summary
When
treatPhpDocTypesAsCertainis false, PHPStan incorrectly reportsfunction.alreadyNarrowedTypefor type checks inside closures/arrow functions passed toarray_map,array_filter, and similar functions. The issue is that PHPDoc-derived callable parameter types were being treated as native types.Changes
MutatingScope::enterAnonymousFunctionWithoutReflection()to track the declared PHP type (before intersection with callable parameters) separately and use it for native expression typesMutatingScope::enterArrowFunctionWithoutReflection()with the same fix — pass the declared type as the native type argument toassignVariable()Root cause
In
enterAnonymousFunctionWithoutReflection, lines 2037-2039 (before fix):The
$parameterTypeincluded the callable parameter type from the calling context (e.g.,stringfromarray_map(fn($item) => ..., $stringArray)). This type was derived from PHPDoc annotations (@param string[] $values), but was incorrectly used as the native type. WhentreatPhpDocTypesAsCertainis false, the rule uses$scope->getNativeType()which returnedstringinstead ofmixed, making it reportis_string()as always true.The same pattern existed in
enterArrowFunctionWithoutReflectionwhereassignVariable($name, $parameterType, $parameterType, ...)used the same enriched type for both the expression type and native type.This mirrors how
foreachcorrectly handles the distinction: it callsgetIterableValueType()on both the PHPDoc type AND the native type separately, usingassignVariable($name, $valueType, $nativeValueType, ...).Analogous cases probed
array_map— was broken, now fixed ✓array_map— was broken, now fixed ✓array_filter— was broken, now fixed ✓array_filter— was broken, now fixed ✓usort,uasort,uksortand all other callback-accepting functions — go through the same code path, now fixed ✓ImpossibleCheckTypeMethodCallRule,ImpossibleCheckTypeStaticMethodCallRule) — use the sameImpossibleCheckTypeHelperand same$scope->getNativeType(), so they benefit from the scope fix ✓foreachloop — was already correct (uses separate native/PHPDoc iterable types) ✓Test
testBug11014inImpossibleCheckTypeFunctionCallRuleTestwith test data covering all four combinations (closure/arrow × array_map/array_filter)tests/PHPStan/Analyser/nsrt/bug-11014.phpverifying native types viaassertNativeType('mixed', $item)for untyped callback parameters andassertNativeType('string', $item)for typed onesFixes phpstan/phpstan#11014