Using array_merge and array shapes

[nicholasruunu]

Bug report

There seems to be a small issue with specificity when using array_merge and array shapes.
It seems to force non-empty-array<'page'|'perPage', int> over the more specific array{page: int, perPage?: int}.
It also passes when rewritten with if statements without array_merge.

Code snippet that reproduces the problem

phpstan snippet

Expected output

I'm expecting it to pass since the array shape is more specific in that page is always present but perPage not always.
Also passes in Psalm

Did PHPStan help you today? Did it make you happy in any way?

Great project, keep it up!

[phpstan-bot]

@nicholasruunu After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-10: Method Paginator::toArray() should return array{page: int, perPage?: int} but returns non-empty-array<'page'|'perPage', int>.
+10: Method Paginator::toArray() should return array{page: int, perPage?: int} but returns array{}.

Full report

Line	Error
10	Method Paginator::toArray() should return array{page: int, perPage?: int} but returns array{}.

[ondrejmirtes]

@canvural Feel free to look at the extension, something about the logic is clearly wrong :) Thanks.

[ondrejmirtes]

I just reverted the changed, I found out the array_merge extension already handles constant arrays mostly fine, except for this case 🤦

[canvural]

@canvural Feel free to look at the extension, something about the logic is clearly wrong :) Thanks.

No, thanks 😄 It was just funny that now it inferred empty array.

[herndlm]

I'll take a look. I messed around with that extension in the past as well

UPDATE: it's the constant array union it cannot deal with yet. if one arg is a non constant array it just generalizes it to a generic array right now

UPDATE2: it is most likely not necessarily a precondition here, but I'm honestly thinking about creating a Type::isConstantArray(), that would help with detecting if we only deal with constant arrays or not quite a bit I guess

[ondrejmirtes]

@herndlm More useful method is getConstantArrays() I think.

[herndlm]

@herndlm More useful method is getConstantArrays() I think.

what I currently don't fully understand with such methods as we have them in e.g. TypeUtils too is that I can't find out if all types of a union are e.g. constant array. wouldn't this lead to code that maybe misses things?
I guess here I could use a helper to get all arrays and then loop over them, but how do I deal with a union like array<string>|array{string}|string?

[herndlm]

maybe it's a use case for the TypeTraverser after all.. :)

[ondrejmirtes]

array<string>|array{string} can't happen, that's going to be normalized into array<string> :)

If we're interested only in unions that consist only of constant arrays, or we want to extract constant arrays from non-exclusive unions, I think those should be two different methods.

I think the problem with TypeTraverser is that it's easy to misuse it and dive into nested arrays like array<array<string>> which is rarely what you want.

[herndlm]

yeah that was a bad example and good point.

I have to think about this a bit more, array_merge works with many other seem-ably constant array unions, but turns out they are all normalized to a constant array. So in this case where the empty array is retained and we have array{}|array{perPage: int} it doesn't work. this could also be an indicator that tagged arrays could lead to a couple of errors here and there, will be interesting :)

[ondrejmirtes]

The vision is that all instanceof ConstantArrayType and all calls to TypeUtils::getConstantArrays() and TypeUtils::getOldConstantArrays() should be replaced with this new method on Type.

The code that currently calls TypeUtils::getConstantArrays() doesn't usually handle optional keys, so that's what we need to be aware of.

[herndlm]

sorry, one more related to this issue: is there currently a way to force-collapse constant array unions? e.g. array{foo: 'bar'}|array{} should become array{foo?: 'bar'}

[ondrejmirtes]

This is all about how we change/optimize reduceArrays().