Fix parameter names for multi-variant functions

[schlndh]

This is an attempt to fix parameter names for multi-variant functions (phpstan/phpstan#9849 (reply in thread)).

As far as I can tell, PHP uses the names from the "main" variant. I'm not sure how to use named and variadic parameters together, so I avoided those.

[ondrejmirtes]

Hi, this is my idea how to solve this problem:

* d6685e7

We introduce getNamedArgumentsVariant() into FunctionReflection and ExtendedMethodReflection, and use it in all important places.

The thinking behind that is that we already have the correct and official information thanks to https://github.com/phpstan/php-8-stubs.

Can you please base your work off this branch?

What remains to be done there:

• Update Php8SignatureMapProvider and FunctionSignatureMapProvider so that it adds this information. You might need to add a new method because currently getMethodSignatures and getFunctionSignatures return an array of signatures of equal importance, but this "named arguments signature" needs to be treated separately. I expect FunctionSignatureMapProvider to always return null for this signature, but Php8SignatureMapProvider will make use of php-8-stubs.
• Php8SignatureMapProvider can still enhance the signature from php-8-stubs with information from functionMap.php, if it finds a single variant, or chooses a variant from multiple ones with the same number (and optionality?) of parameters.
• Go through all FunctionReflection and ExtendedMethodReflection implementations. The interesting ones (that should ask for $namedArgumentsVariant in the constructor are NativeFunctionReflection and NativeMethodReflection. The rest can delegate the call to the wrapped reflection, or they can return null.

What we shouldn't need to do at all:

• Modify functionMap.php. We already have the right information in php-8-stubs, we don't need to edit functionMap for specific functions. We especially don't want to change parameter names because that will break people's baselines, even if they don't use named arguments or are even on PHP 7.

[ondrejmirtes]

You can also enhance the reflection golden test with information from getNamedArgumentVariant too :)

[schlndh]

Thanks for the feedback.

We introduce getNamedArgumentsVariant() into FunctionReflection and ExtendedMethodReflection, and use it in all important places.

It would have to be getNamedArgumentsVariants() - at least some of the multi-variant functions have multiple variants with named arguments as well (sometimes despite the documentation saying otherwise). E.g. strtok, stream_context_set_option, session_set_cookie_params, ...

We especially don't want to change parameter names because that will break people's baselines, even if they don't use named arguments or are even on PHP 7.

Interesting point, I haven't thought about that. But I'm not sure whether it still wouldn't be better in the long-term to have the parameter name correct in getVariants as well.

However, I now realized that you're right - getNamedArgumentsVariants is needed, since some of the variants may not work with named arguments (min/max - or at least I haven't found a way to use the variadic variant with named arguments).

[ondrejmirtes]

I think we could disregard that some functions work with named arguments with multiple variants - the stubs in php-src simply say otherwise AFAIK. It might as well be a bug 😊

[schlndh]

I reworked my previous attempt based on your branch. I allow multiple named arguments variants (the previous attempt did that as well so it was easy + it has greater flexibility for the future).

I'm wondering if I should perhaps explain the purpose of getNamedArgumentsVariants in a comment. I don't recall seeing such comments anywhere, so I didn't do it so far. But IMO it's not immediately obvious why the method is needed (i.e. not all variants are usable with named arguments) and why aren't the argument names fixed in the positional variants as well (i.e. to not break existing baselines).

I also didn't add the named arguments variants to the reflection golden test yet - it would fail the test and I want to see that there is no difference in the positional variants. So I'll add that after the PR is merged.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

This looks really promising! I'll do an in-depth review when time allows it.

There are more issues changed by this, please make sure they pass as well (they should if the changes are correct):

• Incorrect Named Parameters for Levenshtein function phpstan#9018
• setlocale named arguments not correctly identified phpstan#9399
• join() has incorrect named argument stubs phpstan#9923

[ondrejmirtes]

I really like this and couldn't find any issues :) Thank you very much!