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 generic typings to array_uintersect #3806

Open
wants to merge 5 commits into
base: 2.1.x
Choose a base branch
from

Conversation

axlon
Copy link
Contributor

@axlon axlon commented Feb 4, 2025

This PR adds generic typings to array_uintersect, I copied the annotations from array_udiff

Verified

This commit was signed with the committer’s verified signature.
axlon Choraimy Kroonstuiver
@axlon
Copy link
Contributor Author

axlon commented Feb 4, 2025

This PR is a partial fix for phpstan/phpstan#7707, if this PR is the correct way to do this I can add support for the other functions mentioned as well

@axlon axlon marked this pull request as draft February 4, 2025 10:14
@axlon axlon marked this pull request as ready for review February 4, 2025 10:28
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Verified

This commit was signed with the committer’s verified signature.
axlon Choraimy Kroonstuiver
@schlndh
Copy link
Contributor

schlndh commented Feb 7, 2025

@axlon BTW: The same fix is already implemented here: #3282

@axlon
Copy link
Contributor Author

axlon commented Feb 7, 2025

@axlon BTW: The same fix is already implemented here: #3282

I see, I missed this when looking through issues. However there's a slight difference in that the arrays can be different per @ondrejmirtes' suggestion.

The same issue with that PR is still present here; the definitions of these functions are wrong, but merging this would get us closer to PHPStan fully understanding these functions

@schlndh
Copy link
Contributor

schlndh commented Feb 7, 2025

@axlon You're right. Feel free to take whatever you want from the original PR, if you decide to expand this one to cover all similar functions. Alternatively, I'm also around to update my PR if ondrejmirtes prefers that.

@axlon axlon marked this pull request as draft February 10, 2025 09:40
@axlon
Copy link
Contributor Author

axlon commented Feb 10, 2025

I'll try to take a look at this soon

Verified

This commit was signed with the committer’s verified signature.
axlon Choraimy Kroonstuiver

Verified

This commit was signed with the committer’s verified signature.
axlon Choraimy Kroonstuiver
@axlon
Copy link
Contributor Author

axlon commented Feb 25, 2025

I think I've found a way to document these functions where they're quite accurate (so far), but I hit a roadblock with array shapes; PHPStan seems to add an integer key to the return type whenever the first argument is an array shape. I've added failing tests in the last commit and I will try to figure out why this is happening

@schlndh
Copy link
Contributor

schlndh commented Feb 25, 2025

@axlon It looks like it might be possible to work around it by using array<K, V> and then K, instead of T of array and key-of<T>: https://phpstan.org/r/f5f9cb93-4b83-4042-a91f-cd76d4a1789c

@axlon
Copy link
Contributor Author

axlon commented Feb 25, 2025

@axlon It looks like it might be possible to work around it by using array<K, V> and then K, instead of T of array and key-of<T>: https://phpstan.org/r/f5f9cb93-4b83-4042-a91f-cd76d4a1789c

I'll check that out! Not only does this fix the issue, it also makes the return types more accurate 👌

Verified

This commit was signed with the committer’s verified signature.
axlon Choraimy Kroonstuiver
@axlon
Copy link
Contributor Author

axlon commented Feb 25, 2025

@ondrejmirtes please check this out when you find the time, and let me know if this is the way forward. If it is I'll work on adding the remaining array functions listed in the original issue.

As far as the failing tests go:

  • The PHP 7.4 tests probably fail because of mixed (I suppose I should exclude them for PHP 7.4?)
  • The other failing workflows seem unrelated (to my eyes)

@axlon axlon marked this pull request as ready for review February 25, 2025 19:05
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

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

4 participants