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

Variadic parameters in PHP8 can have int|string key #725

Merged
merged 1 commit into from Oct 22, 2021

Conversation

canvural
Copy link
Contributor

This PR is an attempt to fix phpstan/phpstan#4850

When injecting PhpVersion into the MutatingScope constructor, I had to change a lot of other places. Hope that's ok.
And I used $this->phpVersion->supportsNamedArguments() because it's a side effect of the named arguments feature. But if you like, I can add another method with different name.

@@ -2954,7 +2961,11 @@ private function enterFunctionLike(
foreach (ParametersAcceptorSelector::selectSingle($functionReflection->getVariants())->getParameters() as $parameter) {
$parameterType = $parameter->getType();
if ($parameter->isVariadic()) {
$parameterType = new ArrayType(new IntegerType(), $parameterType);
if ($this->phpVersion->supportsNamedArguments()) {
$parameterType = new ArrayType(new BenevolentUnionType([new IntegerType(), new StringType()]), $parameterType);
Copy link
Member

Choose a reason for hiding this comment

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

I'm pondering if it should be a benevolent union type or not. Maybe we want a normal union type so that the user always finds out about a possible bug?

Copy link
Contributor Author

@canvural canvural Oct 20, 2021

Choose a reason for hiding this comment

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

I changed it to UnionType and it found an error in PHPStan itself here. (Parameter #3 $length of function array_slice expects int|null, int|string given.)
Which kinda makes sense I guess. But it can also be considered false positive? With BenevolentUnion it wouldn't complain.

Copy link
Member

Choose a reason for hiding this comment

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

That's great, we should actually do $types = array_values($types); as the first thing in the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -93,7 +93,9 @@ public function dataFileAsserts(): iterable
yield from $this->gatherAssertTypes(__DIR__ . '/data/catch-without-variable.php');
}
yield from $this->gatherAssertTypes(__DIR__ . '/data/mixed-typehint.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-2600.php');
if (PHP_VERSION_ID < 80000) {
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-2600.php');
Copy link
Member

Choose a reason for hiding this comment

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

Why it's no longer tested on PHP 8+? If there are different asserts, please create bug-2600-php8.php for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -427,7 +429,9 @@ public function dataFileAsserts(): iterable

if (self::$useStaticReflectionProvider || PHP_VERSION_ID >= 70400) {
yield from $this->gatherAssertTypes(__DIR__ . '/data/arrow-function-types.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-4902.php');
if (PHP_VERSION_ID < 80000) {
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-4902.php');
Copy link
Member

Choose a reason for hiding this comment

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

Why it's no longer tested on PHP 8+? If there are different asserts, please create bug-4902-php8.php for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@ondrejmirtes ondrejmirtes merged commit 99e4ae0 into phpstan:master Oct 22, 2021
@ondrejmirtes
Copy link
Member

Thank you!

@canvural canvural deleted the variadic-php8 branch October 22, 2021 12:02
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