Skip to content
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

fix named arguments for few multi-variant methods #2748

Merged

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Nov 19, 2023

There are a few cases where PHPStorm stubs have the wrong parameter names. So I'd remove the parameter rename for named params. Since named params are only available from PHP 8, we should have the correct names from php-8-stubs.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please tell me where the "named argument variants" are being read from phpstorm-stubs. I'd say that PHP functions in php-8-stubs should always be read from there.

@@ -593,6 +593,10 @@ private function createMethod(

$phpDocParameterNameMapping[$signatureParameters[$paramI]->getName()] = $reflectionParameter->getName();
}

if ($signatureType === 'named') {
$phpDocParameterNameMapping = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the significance of this change?

@schlndh
Copy link
Contributor Author

schlndh commented Nov 19, 2023

Please tell me where the "named argument variants" are being read from phpstorm-stubs. I'd say that PHP functions in php-8-stubs should always be read from there.

It's not the "named argument variants" that are being read from phpstorm-stubs. But the arguments are renamed based on the phpstorm-stubs. It's exactly the change that you highlighted ($phpDocParameterNameMapping = []) that turns this off for named arguments.

@ondrejmirtes
Copy link
Member

So let's say that in phpstorm-stubs there's:

/**
 * @param int $a
 * @param string $b
 */
public function foo($a, $b): void

And in reality the named arguments are named:

public function foo($c, $d): void

I'd still expect them to be typed with int and string based on the PHPDoc from phpstorm-stubs.

Is that what this PR changes? Or something else?

@schlndh
Copy link
Contributor Author

schlndh commented Nov 19, 2023

You're right - the $phpDocParameterNameMapping is still needed inside createNativeMethodVariant to match the phpdoc types etc. Currently it makes no difference since the named variants are only used for multi-variant methods. But if they were to expand, it might have caused some bugs. I changed it accordingly.

@ondrejmirtes ondrejmirtes merged commit 808b637 into phpstan:1.10.x Nov 20, 2023
424 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@schlndh schlndh deleted the fix-namedArgumentsBrokenByPhpstormStubs branch November 20, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants