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

Add ArrayFindFunctionReturnTypeExtension #3518

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Sep 30, 2024

Supports PHP RFC: array_find for PHP 8.4.

function testConstant(array $array, callable $callback): void
{
assertType("1|'foo'|DateTime|null", array_find($array, $callback));
assertType("1|null", array_find($array, 'is_int'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This cannot be Null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed f0d9c25

function testMixed(array $array, callable $callback): void
{
assertType('mixed', array_find($array, $callback));
assertType('int|null', array_find($array, 'is_int'));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also test it on non empty array, and the result should be an int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like it can be improved at the same time as array_filter().

https://phpstan.org/r/ab62bff1-d485-46ae-9dfa-1931ce320a25

function testConstant(array $array, callable $callback): void
{
assertType("0|1|2|null", array_find_key($array, $callback));
assertType("0|1|2|null", array_find_key($array, 'is_int'));
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, that's correct. This PR does not implement the logic to detect this.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the current behavior ? I assume the type is considered as mixed.

This means that merging the PR as if will introduce a lot of false positif PHPstan errors on level 7 (unions) when it's currently on level 9 (mixed).
That's why IMHO if a dynamic return type extension is introduced, the implementation need to handle such case.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the current behavior ? I assume the type is considered as mixed.

just copy the test over into the playground:
https://phpstan.org/r/ece55e5c-33bf-47cb-b538-35708d025dfa

function testMixed(array $array, callable $callback): void
{
assertType('int|string|null', array_find_key($array, $callback));
assertType('int|string|null', array_find_key($array, 'is_int'));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should test on non-empty-array and it should gives int|string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add a test case, but if you pass a non-empty-array<mixed> the function may return any int|string|null.

https://3v4l.org/Biok8

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, my bad.

@zonuexe zonuexe force-pushed the feature/array_find branch from 581fba3 to 7dd1e43 Compare October 4, 2024 10:42
@zonuexe zonuexe marked this pull request as draft October 4, 2024 10:43
@zonuexe zonuexe force-pushed the feature/array_find branch from 7dd1e43 to 380c077 Compare November 2, 2024 18:12
@zonuexe zonuexe marked this pull request as ready for review November 2, 2024 18:24
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

* @param self::USE_* $mode
* @return array{list<Arg>, ?Variable, ?Variable}
*/
private function createDummyArgs(int $mode): array
Copy link
Member

Choose a reason for hiding this comment

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

Do this refactoring in a separate PR first please.

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, create a pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cherry-picked #3606

@zonuexe
Copy link
Contributor Author

zonuexe commented Nov 5, 2024

I just cherry-picked them, so there should be no conflicts when #3606 is merged.

@ondrejmirtes
Copy link
Member

Fix CS please.

@zonuexe zonuexe force-pushed the feature/array_find branch 3 times, most recently from 3a80801 to 2a12198 Compare November 5, 2024 15:59
@zonuexe
Copy link
Contributor Author

zonuexe commented Nov 5, 2024

@ondrejmirtes I fixed and rebased.

];
}

if (isset($args[0]) && (bool) $args[0]->getAttribute(ArrayWalkArgVisitor::ATTRIBUTE_NAME)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a section about array_walk and it has no tests? It should be a separate PR in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I copied too much existing code.
I'm sure you're right, I'll split out the changes to ParametersAcceptorSelector into a separate PR.

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.

Now you’ve removed too much 😀 It’s useful to declare the callable parameters for two reasons:

  1. To infer the parameter type inside the callable based on array (even when the parameter does not have a native type).
  2. To report wrong callable parameter tpe when it’s different to the passed array.

Please add tests for these.

@zonuexe
Copy link
Contributor Author

zonuexe commented Nov 5, 2024

Ok, I'll get to it right away.

@zonuexe zonuexe marked this pull request as draft November 5, 2024 19:58
@zonuexe zonuexe force-pushed the feature/array_find branch from 7d72141 to 8459f4d Compare November 5, 2024 19:59
@zonuexe zonuexe force-pushed the feature/array_find branch from 8459f4d to 104d3fb Compare November 5, 2024 20:08
@zonuexe zonuexe marked this pull request as ready for review November 5, 2024 20:13
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit 9798bd4 into phpstan:1.12.x Nov 5, 2024
452 checks passed
@ondrejmirtes
Copy link
Member

Perfect, thank you!

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

Successfully merging this pull request may close these issues.

None yet

5 participants