Add ArrayFindFunctionReturnTypeExtension #3518
Conversation
There was a problem hiding this comment.
This cannot be Null.
There was a problem hiding this comment.
You should also test it on non empty array, and the result should be an int
There was a problem hiding this comment.
It seems like it can be improved at the same time as array_filter().
There was a problem hiding this comment.
Ideally, that's correct. This PR does not implement the logic to detect this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You should test on non-empty-array and it should gives int|string
There was a problem hiding this comment.
We can add a test case, but if you pass a non-empty-array<mixed> the function may return any int|string|null.
zonuexe force-pushed the feature/array_find branch from 581fba3 to 7dd1e43 Compare zonuexe force-pushed the feature/array_find branch from 7dd1e43 to 380c077 Compare 
| This pull request has been marked as ready for review. |
There was a problem hiding this comment.
Do this refactoring in a separate PR first please.
There was a problem hiding this comment.
Ok, create a pull request.
zonuexe force-pushed the feature/array_find branch from 8df85ed to cc25101 Compare 
| I just cherry-picked them, so there should be no conflicts when #3606 is merged. |
| Fix CS please. |
zonuexe force-pushed the feature/array_find branch 3 times, most recently from 3a80801 to 2a12198 Compare | @ondrejmirtes I fixed and rebased. |
There was a problem hiding this comment.
Why is there a section about array_walk and it has no tests? It should be a separate PR in any case.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Now you’ve removed too much 😀 It’s useful to declare the callable parameters for two reasons:
- To infer the parameter type inside the callable based on array (even when the parameter does not have a native type).
- To report wrong callable parameter tpe when it’s different to the passed array.
Please add tests for these.
| Ok, I'll get to it right away. |
zonuexe force-pushed the feature/array_find branch from 7d72141 to 8459f4d Compare zonuexe force-pushed the feature/array_find branch from 8459f4d to 104d3fb Compare | This pull request has been marked as ready for review. |
| Perfect, thank you! |
5 participants
Supports PHP RFC: array_find for PHP 8.4.