TypeCombinator returns non-empty-array for union of isIterableAtLeastOnce()->yes()

[staabm]

a array can be non-empty caused by different accessory types, namely NonEmptyArrayType, HasOffsetType, HasOffsetValueType.

before this PR we only returned a non-empty-array result from TypeCombinator::union() when all elements in the union were using the same accessory types, e.g.

• union(array&non-empty, array&non-empty)
• union(array&hasOffset('a'), array&hasOffset('a'))

after this PR we in addition return a non-empty-array for unions of arrays which are non-empty caused by different accessories, e.g.

• union(array&non-empty, array&hasOffset('a'))
• union(array&hasOffset('b'), array&hasOffset('a'))
• union(array&hasOffset('b'), array&hasOffsetValue(4, 'a'))

refs #3924 (comment)

[staabm]

atm this can lead to some arrays, which - while processing - have the NonEmptyArrayType 2-times.
when TypeCombinator::union() finally ends, this "duplication" gets normalized away.

we could check at this IF-condition here, whether $commonAccessoryTypes already contains NonEmptyArrayType so we don't add it a 2nd time.
for readability I have chosen not todo this additionaly check. but I am fine with adding it, in case you think we should do it.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

I'd say this isn't necessary. Intersection type of ArrayType and HasOffsetValueType should already know it's iterable at least once (it's non-empty).

The question is - why the tests you changed already don't result in non-empty-array being expected...

[staabm]

The question is - why the tests you changed already don't result in non-empty-array being expected...

before this PR the TypeCombinator only takes accessory types into the final type, when they are contained in every element of the union (except OversizedArrayType)

see

phpstan-src/src/Type/TypeCombinator.php

Lines 687 to 704 in 5df4617

	$commonAccessoryTypes = [];
	$arrayTypeCount = count($arrayTypes);
	foreach ($accessoryTypes as $accessoryType) {
	if (count($accessoryType) !== $arrayTypeCount) {
	$firstKey = array_key_first($accessoryType);
	if ($accessoryType[$firstKey] instanceof OversizedArrayType) {
	$commonAccessoryTypes[] = $accessoryType[$firstKey];
	}
	continue;
	}
	if ($accessoryType[0] instanceof HasOffsetValueType) {
	$commonAccessoryTypes[] = self::union(...$accessoryType);
	continue;
	}
	$commonAccessoryTypes[] = $accessoryType[0];
	}

therefore in e.g. TypeCombinator::union(array&hasOffset('b'), array&hasOffsetValue(4, 'a')) the resulting Type is just array (it is loosing all accessories, and therefore it no longer is e.g. non-empty)

[ondrejmirtes]

Ohhh, I get it, this makes sense then!

[ondrejmirtes]

Thank you!