Skip to content

Add Type::getIterableCount() #1828

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

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Oct 12, 2022

No description provided.

@herndlm herndlm force-pushed the get-iterable-count branch from b7048c8 to 112d0df Compare October 12, 2022 21:32
@herndlm herndlm marked this pull request as ready for review October 12, 2022 21:47
@ondrejmirtes
Copy link
Member

I'm blown away, this is really nice!

What it allows us to do in the future is to track precise array size:

if (count($a) >= 3) {
    array_shift($a);
    // now we now it's >= 2
}

So there should be something like AccessoryArraySizeType in the future. What's really nice with the "everything is a method on Type" approach is that we can nicely implement the array modifications with methods on Type like push, pop, shift, unshift. No more weird TypeUtils::getAccessoryTypes() juggling!

The precise array size will allow us to tell that when we have a list of size 3, $list[3] will definitely not exist.

What's hard about this is that we need to solve all interactions between AccessoryArraySizeType and NonEmptyArrayType. Unions/intersections need to behave deterministically. We should probably deprecate NonEmptyArrayType.

Another thing to think about is type description. Maybe AccessoryArraySizeType should stay hidden similarly to HasPropertyType and HasMethodType because in most situations, it will be irrelevant to the user that we know "this array has size 14". Maybe we should keep saying "non-empty-array" if we have AccessoryArraySizeType of > 0.

Anyway, thanks for this PR, it's great.

@ondrejmirtes ondrejmirtes merged commit 43d6b0b into phpstan:1.9.x Oct 13, 2022
@ondrejmirtes
Copy link
Member

BTW the traits situation currently is a bit messy. These are the warnings I get in PhpStorm:

Screenshot 2022-10-13 at 9 04 16

I think using a trait only makes sense if:

  1. Type traits themselves have disjoint sets of methods. Meaning that there cannot be multiple traits that define the same method. We can verify this by iterating all used traits by a class (recursively) and see if there are any duplicate methods.
  2. If the Type implementation class doesn't override any of the methods used from traits.

So if we want to continue using traits in types, we should write internal PHPStan rules to check these usages in PHPStan :)

@herndlm
Copy link
Contributor Author

herndlm commented Oct 13, 2022

Interesting! Yeah, moving the array modifications to Type is what I wanted to do next.
And regarding traits: I see. Yes, this should be adressed sooner than later I guess :/

@herndlm herndlm deleted the get-iterable-count branch October 13, 2022 07:12
@herndlm
Copy link
Contributor Author

herndlm commented Oct 13, 2022

but regarding the trait example you mentioned with HasPropertyType - I didn't see where e.g. isOversizedArray() clashed. can you? is PHPStorm maybe confused by our traits? :D I went through all traits and the class itself twice and only saw i once in NonArrayType. I know other classes where such clashes are though

@ondrejmirtes
Copy link
Member

I also didn't find it, but we should have the rules to make sure it's clean.

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.

2 participants