-
Notifications
You must be signed in to change notification settings - Fork 506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add callback types for array_uintersect etc v2 #3312
Conversation
array_diff_uassoc array_diff_ukey array_intersect_uassoc array_intersect_ukey array_udiff_assoc array_udiff_uassoc array_uintersect_assoc array_uintersect_uassoc array_uintersect
…iddle variadic param
Fixes regression of issue 9697
@@ -201,8 +201,11 @@ public static function decideType( | |||
} | |||
|
|||
if ( | |||
(!$phpDocType instanceof NeverType || ($type instanceof MixedType && !$type->isExplicitMixed())) | |||
&& $type->isSuperTypeOf(TemplateTypeHelper::resolveToBounds($phpDocType))->yes() | |||
($type->isCallable()->yes() && $phpDocType->isCallable()->yes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, I could broaden the type of the callbacks in function map to callable
and then I wouldn't need this. I'm not sure what's better.
|| ( | ||
$className === null | ||
// These functions have a variadic parameter in the middle. This is not well described by php8-stubs. | ||
&& in_array( | ||
$functionName, | ||
[ | ||
'array_diff_uassoc', | ||
'array_diff_ukey', | ||
'array_intersect_uassoc', | ||
'array_intersect_ukey', | ||
'array_udiff', | ||
'array_udiff_assoc', | ||
'array_udiff_uassoc', | ||
'array_uintersect', | ||
'array_uintersect_assoc', | ||
'array_uintersect_uassoc', | ||
], | ||
true, | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to complicate it with the php8-stubs. The issue is that the stubs look like this:
/** @param array|callable $rest */
function array_intersect_uassoc(array $array, ...$rest): array
{
}
which then overrides the newly introduced stub. So I needed to skip them somehow. I suppose an alternative would be to skip php8-stubs where the last parameter is ...$rest
, but I prefer this more explicit way.
Hi, I appreciate your effort but I'm not really sure about this approach. This isn't really valid PHP: function array_udiff(
array $array,
array ...$arrays,
callable $value_compare_func,
): array {} So stubs shouldn't be written like this and info with this structure should not appear in reflection. The fact you need to handle this situation in src/Rules/FunctionCallParametersCheck.php (in the rules layer) is a sign we might break a bunch of code that would forget to handle it. To modify the signature on the fly based on the actual call args with my suggestion from here #3282 (comment) would be much more palatable for me. |
This is a more advanced alternative to #3282
Fixes phpstan/phpstan#7707
I didn't go the route you suggested here: #3282 (comment) . Instead, I bypassed php8-stubs for these functions so that they don't conflict with the new stubs. And I added support for in-the-middle variadic parameter to
FunctionCallParametersCheck
. IMO handling it directly in the reflection should cause less inconsistency.