preserve large arrays with same keys through union

[schlndh]

Fixes https://phpstan.org/r/0071beb4-becd-47c0-8a17-b52407da0c22
Fixes phpstan/phpstan#9397
Fixes phpstan/phpstan#10080

Basically, I have a large array shape, which I'm not modifying (and it's not a tagged union either). I just use the elements individually. But this causes the array to be generalized into a "useless" non-empty-array<...>. As a workaround, I can pull the elements used in conditions into variables and then use the variables, but it is quite inconvenient.

So this PR attempts to maintain the array shape in the simplest case where 2 constant arrays with the same keys are being unioned together.

[ondrejmirtes]

According to issue-bot this fixes 2 issues, please write regression tests for them 😊 Thank you.

[ondrejmirtes]

From the looks of the name, there's already a method with a very similar goal: TypeCombinator::reduceArrays. Can't we call that one?

[schlndh]

I tried replacing the call to unionConstantArrayTypesWithSameKeys with this (if (count($reducedArrayTypes) === 1) { so that I don't turn off the generalization completely).

$reducedArrayTypes = self::reduceArrays($arrayTypes);
if (count($reducedArrayTypes) === 1) {
	return $reducedArrayTypes;
}

It worked with the test-cases that I had, but it is not quite equivalent. I added a test-case to reject this option. The problem is in ConstantArrayType::isKeysSupersetOf (

phpstan-src/src/Type/Constant/ConstantArrayType.php

Line 1540 in 27e2b53

$failOnDifferentValueType = true;

). It allows at most one value type to differ.

---

The goal of unionConstantArrayTypesWithSameKeys is to detect the union of

array{K1: T11, K2: T12, ... KN: T1N}
array{K1: T21, K2: T22, ... KN: T2N}
...
array{K1: TM1, K2: TM2, ... KN: TMN}

and return array{K1: T11|T21|...|TM1, K2: T12|T22|...|TM2, ..., KN: T1N|T2N|...|TMN}.

If the input doesn't match the simple target situation, then the method just returns null and we can proceed with the generalization.

---

reduceArrays seems to do the following:

• Separate non-empty constant arrays, non-constant arrays and empty array.
• Process the constant arrays:
  • For each pair check if one array is a "superset" of the other one, and if so merge them together.
• Return an array of all the remaining constant array types, as well as the non-constant arrays and the empty array.

It works in some cases, but it is more complicated than it needs to be for the scenario that I had in mind. And there are a lot of subtle details involved.

[ondrejmirtes]

I feel like that having two different methods that do very similar things is gonna bite us in the future. Can we improve reduceArrays and use it in more places so that it works for fixing these cases?

[schlndh]

processArrayTypes is certainly quite complicated already, and this isn't making it any simpler. But I'm not sure how to modify reduceArrays such that it can be used in both places, and it is simpler than having a separate method.

The problem for me is that reduceArrays maintains tagged unions: E.g. array{a: 1, b: int}|array{a: 2, b: string} should remain unchanged by reduceArrays. However, I assume that this is something that we can't afford when the arrays involved are too large. So there is the generalization, which would (assuming lower generalization limit) turn this into non-empty-array<'a'|'b', int|string>.

With my change we would lose the tagged union, but at least we'd maintain the general shape of the array: array{a: 1|2, b: string|int}.

So reduceArrays would have to know whether or not it should try to maintain the tagged unions. Either there would be a boolean flag, or it could re-check the size of the arrays involved, ...

---

The issue in my use-case happens because after a branching statement (if, ternary operator, cycles, ...) the scopes from non-terminating branches are unioned together and they replace the previous scope. However, in my use-case the array is not modified in any of the branches and yet the result of the union is a more general type than it was before the branching statement.

So perhaps this could be another way of fixing the issue: detect scenarios where the variable is not modified and the new type is wider and keep the previous type instead. But it seems complicated.

[ondrejmirtes]

Either there would be a boolean flag, or it could re-check the size of the arrays involved, ...

sounds good to me to at least try it!

So perhaps this could be another way of fixing the issue

this should also be possible. We have all the information available. There are three scopes at play - let’s say we’re analysing if-else. There the scope before the statement (original) and there’s the if scope and else scope. If the type of the expression is equal in all three, we don’t have to run the union() operation. There might even be performance benefit to that…

[schlndh]

sounds good to me to at least try it!

You have a tendency to be right about these things. 👍

There are three scopes at play - let’s say we’re analysing if-else. There the scope before the statement (original) and there’s the if scope and else scope. If the type of the expression is equal in all three, we don’t have to run the union() operation. There might even be performance benefit to that…

That wouldn't help in my use-case: the types are different in the branches (e.g. the condition is a null-check on one of the dimensions), but their union should be the same (or narrower) than the original scope, which it isn't, because the arrays are too large and they get generalized.

Anyway, after I wrote the previous comment, I realized that it's better to fix it in the union method (if possible), because that is a more general fix.

[schlndh]

As far as I can tell, currently the accessory types will always be empty (so I can't test this). But I added them anyway to be safe in case new accessory types are added in the future.

[schlndh]

I'm conflicted about this. On one hand, it would feel bad not to have this condition and run a lot of the method uselessly. But on the other hand, the condition depends on the particular usage of $preserveTaggedUnions. Maybe a better name for $preserveTaggedUnions would help, but I didn't come up with anything. So I at least added a comment.

[ondrejmirtes]

This is fine 👍

[ondrejmirtes]

Awesome, I really like this! 👍

[ondrejmirtes]

Thank you!

[staabm]

Nice one 👍