Make IntegerRangeType->isSubTypeOf analyse Unions by GKFX · Pull Request #416 · phpstan/phpstan-src

GKFX · 2021-01-03T22:19:51Z

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 · 2021-01-04T21:25:18Z

tests/PHPStan/Type/IntegerRangeTypeTest.php

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.

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 · 2021-01-04T21:27:44Z

tests/PHPStan/Type/IntegerRangeTypeTest.php

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

jlherren · 2021-01-04T21:38:59Z

src/Type/IntegerRangeType.php

(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 · 2021-01-04T22:03:46Z

tests/PHPStan/Type/IntegerRangeTypeTest.php

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

ondrejmirtes · 2021-01-11T07:41:59Z

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 · 2021-01-11T17:08:51Z

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 · 2021-01-24T16:55:03Z

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

Partially resolves phpstan/phpstan#3339

GKFX · 2021-01-25T23:28:51Z

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 · 2021-01-31T17:07:29Z

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 · 2022-02-08T12:21:32Z

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

GKFX force-pushed the feature-intrange-subtype-unions branch from 1fc2d6d to 40ea36d Compare January 3, 2021 22:33

ondrejmirtes mentioned this pull request Jan 4, 2021

Improve specified type in comparisons #390

Merged

jlherren suggested changes Jan 4, 2021

View reviewed changes

GKFX force-pushed the feature-intrange-subtype-unions branch from 40ea36d to 7b0412a Compare January 11, 2021 17:07

GKFX force-pushed the feature-intrange-subtype-unions branch 4 times, most recently from bad292c to 835c69d Compare January 13, 2021 19:41

GKFX force-pushed the feature-intrange-subtype-unions branch from 835c69d to 26e77a7 Compare January 25, 2021 23:01

Make IntegerRangeType->isSubTypeOf analyse Unions

026c6c4

Partially resolves phpstan/phpstan#3339

GKFX force-pushed the feature-intrange-subtype-unions branch from 26e77a7 to ceed528 Compare January 25, 2021 23:08

Apply some feedback from PR

ab4a783

GKFX force-pushed the feature-intrange-subtype-unions branch from ceed528 to ab4a783 Compare January 25, 2021 23:14

ondrejmirtes force-pushed the master branch 2 times, most recently from efd31ce to 0471f87 Compare July 29, 2021 14:08

ondrejmirtes force-pushed the master branch from 97c2f21 to a03a78f Compare December 19, 2021 16:34

ondrejmirtes closed this Feb 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make IntegerRangeType->isSubTypeOf analyse Unions#416

Make IntegerRangeType->isSubTypeOf analyse Unions#416
GKFX wants to merge 2 commits intophpstan:masterfrom
GKFX:feature-intrange-subtype-unions

GKFX commented Jan 3, 2021

Uh oh!

jlherren Jan 4, 2021

Uh oh!

GKFX Jan 25, 2021

Uh oh!

jlherren Jan 4, 2021

Uh oh!

jlherren Jan 4, 2021 •

edited

Loading

Uh oh!

jlherren Jan 4, 2021

Uh oh!

ondrejmirtes commented Jan 11, 2021

Uh oh!

GKFX commented Jan 11, 2021

Uh oh!

ondrejmirtes commented Jan 24, 2021

Uh oh!

GKFX commented Jan 25, 2021

Uh oh!

jlherren commented Jan 31, 2021

Uh oh!

ondrejmirtes commented Feb 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

GKFX commented Jan 3, 2021

Uh oh!

jlherren Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

GKFX Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

jlherren Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

jlherren Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlherren Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jan 11, 2021

Uh oh!

GKFX commented Jan 11, 2021

Uh oh!

ondrejmirtes commented Jan 24, 2021

Uh oh!

GKFX commented Jan 25, 2021

Uh oh!

jlherren commented Jan 31, 2021

Uh oh!

ondrejmirtes commented Feb 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jlherren Jan 4, 2021 •

edited

Loading