Make IntegerRangeType->isSubTypeOf analyse Unions

[GKFX]

This commit allows int<m,n> to be recognised as a subtype of unions like m|m+1|m+2|...|n-1|n or int<m,n-1>|n. It also creates a new interface PHPStan\Type\ImplementsSubTypeOfUnion which indicates to UnionType->superTypeOf that it should call isSubTypeOf on the other object. (Previously there was no need for such an interface as IterableType was the only such type.)

This resolves a modified form of phpstan/phpstan#3339: https://phpstan.org/r/00e327fa-55e9-41b8-8b6e-3e8924d9a3d4

<?php declare(strict_types = 1);

class HelloWorld
{
	/** @var array{bool, bool, bool, bool} */
	private $tuple;
	
	public function main(): void
	{
		$this->tuple = [true, true, true, true];
		/** @var positive-int */
		$i = 1;
		for (; $i < 4; ++$i)
		{
		    $this->tuple[$i] = false;
		}
	}
}

It doesn't address the fact that in the original snippet PHPStan thinks $i might take any value in the range [PHP_INT_MIN,2].

[jlherren]

It sounds to me like the correct answer here is 'maybe', since the second type might be a string, in which case an integer is not a sub-type of it.

[GKFX]

I think int<5,10> is a subclass of int|string; certainly float|string is a superclass of both float and string according to the isSuperTypeOf method on the UnionType.

[jlherren]

The message is wrong here (mentions isSuperTypeOf instead of isSubTypeOf)

[jlherren]

(Sorry I messed up the line range selection here... the comment is meant for the entire method unionIntRanges())

Code to pretty much do the exact same thing already exists in TypeCombinator::union(). In fact, a UnionType should probably never even contain mergeable integer ranges (or const ints). This is currently not enforced, but the idea is to use TypeCombinator::union(...) instead of new UnionType([...]) whenever in doubt that there might be overlapping types.

I understand that in your example you end up with a union type 1|2|3|4 which is then tested against int<1,4>. This currently doesn't work properly. I wonder if the cleaner solution wouldn't just be to disallow adjacent integers in unions and enforce the usage of integer ranges (e.g. always int<1,2> instead of 1|2. This ensures that there's only way one to represent the same thing. With this the sub-type check would automatically start to work correctly.

[jlherren]

Same here, although this specific union shouldn't even exist in practice, since the 5 should be merged into int<6, 10>.

[ondrejmirtes]

Looks like this PR will fix these cases:

https://phpstan.org/r/93116f33-5437-4d97-b06f-d04949258051
https://phpstan.org/r/845fb7f8-993b-495e-ba34-e4d59b385560

Can you add integration tests to respective rules? Thanks!

[GKFX]

Based on jlherren's feedback, I started this again to take advantage of the existing functionality in TypeCombinator as I didn't realise that existed. I have added the requested tests.

[ondrejmirtes]

Hi, there's still too many failing tests. Do you plan to finish this PR? Thanks!

[GKFX]

Hi Ondrej, with the approach of converting all 1|2|3 to int<1,3> etc, there was a bug introduced where the i in for loops could somehow be falsely inferred to be a small range like int<0,3>. I couldn't find why this was, and that approach also was quite disruptive in that a lot of tests had to be changed to expect range information to appear in test output. So I reverted back to my original commit and just added the requested extra tests. (The other approach is saved at https://github.com/GKFX/phpstan-src/commits/unions-to-intrange if anyone still is interested in it.)

[jlherren]

I had time to have a look at this again, here are my thoughts:

• It would be better to normalize the content of UnionType within TypeCombinator::union(). A UnionType should never contain mergeable types in the first place. It's a shame to normalize those unions every time isSubTypeOf() is called and then discard the result after using it. Also the UnionType constructor should only be called when absolutely sure that there are no mergeable types.
• This would actually be a nice opportunity to extend on the new idea introduced with IntegerRangeType::tryUnion(). There could be a new ConstantIntegerType::tryUnion() which would merge adjacant integers into a IntegerRangeType. Ultimately tryUnion() could be moved up to Type some day, greatly simplifying TypeCombinator::union().

I believe this approach would fix the issue in far fewer lines of code (but I might be mistaken of course).

[ondrejmirtes]

This is old and stale, feel free to start fresh on top of current master in a new PR. Thanks!