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

array_fill(): handle negative cases, support integer ranges and non-empty-array #603

Merged
merged 12 commits into from Aug 20, 2021

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 31, 2021

array_fill expected the first arg to be an int.

see https://3v4l.org/Sq4tH

closes phpstan/phpstan#5284

@staabm staabm changed the title array_fill() always returns false on negative 2nd arg array_fill() always returns false on negative 2nd arg Jul 31, 2021
@staabm
Copy link
Contributor Author

staabm commented Jul 31, 2021

array_fill expected the first arg to be an int.

when passing in e.g. 'abc' then it will return null.
see https://3v4l.org/TfY0e

not sure whether/how this function signature violating cases should be handled

@staabm
Copy link
Contributor Author

staabm commented Jul 31, 2021

build error looks unrelated to me. should be good to go.

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.

This isn't ideal. Ideally you'd return array when you're sure the count is 0 or positive, false if you're sure it's negative, and array|false if it's just an int or for example int<-3, 5..

For PHP 8 we should return NeverType here, and also modify functionMap_php80delta.php to have 0|positive-int there.

Copy link
Contributor Author

@staabm staabm left a comment

Choose a reason for hiding this comment

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

This isn't ideal. Ideally you'd return array when you're sure the count is 0 or positive, false if you're sure it's negative, and array|false if it's just an int or for example int<-3, 5..

For PHP 8 we should return NeverType here, and also modify functionMap_php80delta.php to have 0|positive-int there.

oh wow, a lot of cases I didn't think about. great feedback as always <3.
I think it should be incorporated into the PR

@staabm staabm changed the title array_fill() always returns false on negative 2nd arg array_fill() handle negative cases and support integer ranges Aug 17, 2021
@staabm
Copy link
Contributor Author

staabm commented Aug 17, 2021

the latest commit adds the test-cases from #602 - which I closed now

@staabm staabm changed the title array_fill() handle negative cases and support integer ranges array_fill() handle negative cases, support integer and non-empty-array ranges Aug 17, 2021
@staabm staabm changed the title array_fill() handle negative cases, support integer and non-empty-array ranges array_fill(): handle negative cases, support integer ranges and non-empty-array Aug 17, 2021
@@ -146,6 +146,7 @@
'old' => [

'array_combine' => ['associative-array|false', 'keys'=>'string[]|int[]', 'values'=>'array'],
'array_fill' => ['array', 'start_key'=>'int', 'num'=>'0|positive', 'val'=>'mixed'],
Copy link
Member

Choose a reason for hiding this comment

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

If you add something to the old section, you just remove the key from the final map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what todo. I moved this line to the new section now.

is there anything I can do to test the change to this file?

Copy link
Member

Choose a reason for hiding this comment

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

It's not necessary to test it. You can test it yourself simply by analysing file with wrong array_fill() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - tested the latest change locally and it works:

$ php bin/phpstan analyze test.php --level max
Note: Using configuration file C:\xampp7.3\htdocs\phpstan-src\phpstan.neon.dist.
 1/1 [============================] 100%

 ------ --------------------------------------------------------------------
  Line   test.php
 ------ --------------------------------------------------------------------
  4      Parameter #2 $count of function array_fill expects int<0, max>, -5
         given.
 ------ --------------------------------------------------------------------


 [ERROR] Found 1 error

@@ -271,7 +271,7 @@
'array_diff_uassoc\'1' => ['array', 'arr1'=>'array', 'arr2'=>'array', 'arr3'=>'array', 'arg4'=>'array|callable(mixed,mixed):int', '...rest='=>'array|callable'],
'array_diff_ukey' => ['array', 'arr1'=>'array', 'arr2'=>'array', 'key_comp_func'=>'callable(mixed,mixed):int'],
'array_diff_ukey\'1' => ['array', 'arr1'=>'array', 'arr2'=>'array', 'arr3'=>'array', 'arg4'=>'array|callable(mixed,mixed):int', '...rest='=>'array|callable(mixed,mixed):int'],
'array_fill' => ['array', 'start_key'=>'int', 'num'=>'int', 'val'=>'mixed'],
'array_fill' => ['array', 'start_index'=>'int', 'count'=>'int', 'value'=>'mixed'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjusted the param names to the ones named on php.net: https://www.php.net/manual/en/function.array-fill.php

Copy link
Member

Choose a reason for hiding this comment

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

I don't want this. The parameter names are correct because on PHP 8 these signatures are merged with https://github.com/phpstan/php-8-stubs. So there's no need for this change.

@@ -271,7 +271,7 @@
'array_diff_uassoc\'1' => ['array', 'arr1'=>'array', 'arr2'=>'array', 'arr3'=>'array', 'arg4'=>'array|callable(mixed,mixed):int', '...rest='=>'array|callable'],
'array_diff_ukey' => ['array', 'arr1'=>'array', 'arr2'=>'array', 'key_comp_func'=>'callable(mixed,mixed):int'],
'array_diff_ukey\'1' => ['array', 'arr1'=>'array', 'arr2'=>'array', 'arr3'=>'array', 'arg4'=>'array|callable(mixed,mixed):int', '...rest='=>'array|callable(mixed,mixed):int'],
'array_fill' => ['array', 'start_key'=>'int', 'num'=>'int', 'val'=>'mixed'],
'array_fill' => ['array', 'start_index'=>'int', 'count'=>'int', 'value'=>'mixed'],
Copy link
Member

Choose a reason for hiding this comment

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

I don't want this. The parameter names are correct because on PHP 8 these signatures are merged with https://github.com/phpstan/php-8-stubs. So there's no need for this change.

@@ -87,6 +87,10 @@ public function testSchedule(
array $expectedJobSizes
): void
{
if ($numberOfFiles < 0) {
throw new \PHPStan\ShouldNotHappenException();
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather just have 0|positive-int in the PHPDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since phpstan does not verify the phpdoc against the phpunit-data-provider I figured it would be more safe to use a IF check here.

will change to your suggestion though

@ondrejmirtes ondrejmirtes merged commit fdf732c into phpstan:master Aug 20, 2021
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the array_fill_always_false branch August 20, 2021 11:11
@ntzm
Copy link
Contributor

ntzm commented Aug 20, 2021

Thanks @staabm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants