Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make IntegerRangeType->isSubTypeOf analyse Unions #416

Closed
wants to merge 2 commits into from
Closed

Make IntegerRangeType->isSubTypeOf analyse Unions #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
026c6c4
Make IntegerRangeType->isSubTypeOf analyse Unions
GKFX Jan 3, 2021
ab4a783
Apply some feedback from PR
GKFX Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/Type/ImplementsSubTypeOfUnion.php
View file
Open in desktop
@@ -0,0 +1,16 @@
<?php declare(strict_types = 1);

namespace PHPStan\Type;

use PHPStan\TrinaryLogic;

/**
* Indictates that isSubTypeOf may be called with a UnionType without risking
* infinite recursion.
*/
interface ImplementsSubTypeOfUnion
{

public function isSubTypeOf(Type $type): TrinaryLogic;

}
51 changes: 49 additions & 2 deletions src/Type/IntegerRangeType.php
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\Constant\ConstantIntegerType;

class IntegerRangeType extends IntegerType implements CompoundType
class IntegerRangeType extends IntegerType implements CompoundType, ImplementsSubTypeOfUnion
{

private ?int $min;
Expand Down Expand Up @@ -248,13 +248,60 @@ public function isSubTypeOf(Type $otherType): TrinaryLogic
return $otherType->isSuperTypeOf($this);
}

if ($otherType instanceof UnionType || $otherType instanceof IntersectionType) {
if ($otherType instanceof IntersectionType) {
return $otherType->isSuperTypeOf($this);
}

if ($otherType instanceof UnionType) {
/** @var array<array{int,int}> */
$otherRanges = [];
foreach ($otherType->getTypes() as $component) {
if (get_class($component) === IntegerType::class) {
return TrinaryLogic::createYes();
}
if ($component instanceof ConstantIntegerType) {
$otherRanges[] = [$component->getValue(), $component->getValue()];
} elseif ($component instanceof self) {
$otherRanges[] = [$component->getMin() ?? PHP_INT_MIN, $component->getMax() ?? PHP_INT_MAX];
}
}
self::unionIntRanges($otherRanges);
foreach ($otherRanges as $otherRange) {
// The $otherRanges are disjoint so only one of these conditions
// will ever match.
if ($otherRange[0] <= ($this->getMin() ?? PHP_INT_MIN) &&
$otherRange[1] >= ($this->getMax() ?? PHP_INT_MAX)) {
return TrinaryLogic::createYes();
}
if (!self::isDisjoint($this->getMin(), $this->getMax(), ...$otherRange)) {
return TrinaryLogic::createMaybe();
}
}
}

return TrinaryLogic::createNo();
}

/**
* Take the union of a list of ranges, merging all adjacent ranges.
* @param array<array{int,int}> $ranges A list of closed, integer intervals.
*/
private static function unionIntRanges(array &$ranges): void
{
usort($ranges, static function ($a, $b): int {
return $a[0] <=> $b[0];
});
for ($i = 0; $i < count($ranges) - 1;) {
$thisMax = $ranges[$i][1];
if ($thisMax === PHP_INT_MAX || $thisMax + 1 >= $ranges[$i + 1][0]) {
$ranges[$i][1] = max($ranges[$i][1], $ranges[$i + 1][1]);
array_splice($ranges, $i + 1, 1);
} else {
$i++;
}
}
Copy link
Contributor

@jlherren jlherren Jan 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

}

public function isAcceptedBy(Type $acceptingType, bool $strictTypes): TrinaryLogic
{
return $this->isSubTypeOf($acceptingType);
Expand Down
2 changes: 1 addition & 1 deletion src/Type/IterableType.php
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@
use PHPStan\Type\Traits\UndecidedBooleanTypeTrait;
use PHPStan\Type\Traits\UndecidedComparisonCompoundTypeTrait;

class IterableType implements CompoundType
class IterableType implements CompoundType, ImplementsSubTypeOfUnion
{

use MaybeCallableTypeTrait;
Expand Down
2 changes: 1 addition & 1 deletion src/Type/UnionType.php
View file
Open in desktop
Expand Up @@ -77,7 +77,7 @@ public function accepts(Type $type, bool $strictTypes): TrinaryLogic

public function isSuperTypeOf(Type $otherType): TrinaryLogic
{
if ($otherType instanceof self || $otherType instanceof IterableType) {
if ($otherType instanceof self || $otherType instanceof ImplementsSubTypeOfUnion) {
return $otherType->isSubTypeOf($this);
}

Expand Down
4 changes: 4 additions & 0 deletions tests/PHPStan/Rules/Classes/InstantiationRuleTest.php
View file
Open in desktop
Expand Up @@ -193,6 +193,10 @@ public function testInstantiation(): void
'Class TestInstantiation\ClassExtendingAbstractConstructor constructor invoked with 0 parameters, 1 required.',
273,
],
[
'Parameter #2 $y of class TestInstantiation\IntRange constructor expects 1|2|3|4|5|6|7, int<1, 8> given.',
291,
],
]
);
}
Expand Down
26 changes: 26 additions & 0 deletions tests/PHPStan/Rules/Classes/data/instantiation.php
View file
Open in desktop
Expand Up @@ -274,3 +274,29 @@ public function doBar()
}

}

final class IntRange
{
/**
* @psalm-var 1|2|3|4|5|6|7|8
*/
private $x;

public static function fromInt(int $x): self
{
if ($x < 1 || $x > 8) {
throw new \InvalidArgumentException;
}

return new self($x, $x);
}

/**
* @psalm-param 1|2|3|4|5|6|7|8 $x
* @psalm-param 1|2|3|4|5|6|7 $y
*/
private function __construct(int $x, int $y)
{
$y = $this->x = $x;
}
}
4 changes: 4 additions & 0 deletions tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php
View file
Open in desktop
Expand Up @@ -61,6 +61,10 @@ public function testReturnTypeRule(): void
'Function ReturnTypes\returnNever() should never return but return statement found.',
181,
],
[
'Function ReturnTypes\returnRangeBad() should return 1|2|3|4|5|6|7 but returns int<1, 8>.',
203,
],
]);
}

Expand Down
22 changes: 22 additions & 0 deletions tests/PHPStan/Rules/Functions/data/returnTypes.php
View file
Open in desktop
Expand Up @@ -180,3 +180,25 @@ function returnNever()
{
return;
}

/**
* @return 1|2|3|4|5|6|7|8
*/
function returnRange(int $x) : int {
if ($x < 1 || $x > 8) {
throw new InvalidArgumentException;
}

return $x;
}

/**
* @return 1|2|3|4|5|6|7
*/
function returnRangeBad(int $x) : int {
if ($x < 1 || $x > 8) {
throw new InvalidArgumentException;
}

return $x;
}
75 changes: 75 additions & 0 deletions tests/PHPStan/Type/IntegerRangeTypeTest.php
View file
Open in desktop
@@ -0,0 +1,75 @@
<?php declare(strict_types = 1);

namespace PHPStan\Type;

use PHPStan\TrinaryLogic;
use PHPStan\Type\Constant\ConstantIntegerType;

class IntegerRangeTypeTest extends \PHPStan\Testing\TestCase
{

public function dataIsSubTypeOf(): iterable
{
yield [
IntegerRangeType::fromInterval(5, 10),
new IntegerType(),
TrinaryLogic::createYes(),
];

yield [
IntegerRangeType::fromInterval(5, 10),
new ConstantIntegerType(10),
TrinaryLogic::createMaybe(),
];

yield [
IntegerRangeType::fromInterval(5, 10),
new ConstantIntegerType(20),
TrinaryLogic::createNo(),
];

yield [
IntegerRangeType::fromInterval(5, 10),
IntegerRangeType::fromInterval(0, 15),
TrinaryLogic::createYes(),
];

yield [
IntegerRangeType::fromInterval(5, 10),
IntegerRangeType::fromInterval(6, 9),
TrinaryLogic::createMaybe(),
];

yield [
IntegerRangeType::fromInterval(5, 10),
new UnionType([new IntegerType(), new StringType()]),
TrinaryLogic::createYes(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

];

yield [
IntegerRangeType::fromInterval(5, 10),
new UnionType([new ConstantIntegerType(5), IntegerRangeType::fromInterval(6, 10), new StringType()]),
TrinaryLogic::createYes(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

];

yield [
IntegerRangeType::fromInterval(5, 10),
new StringType(),
TrinaryLogic::createNo(),
];
}

/**
* @dataProvider dataIsSubTypeOf
*/
public function testIsSubTypeOf(IntegerRangeType $type, Type $otherType, TrinaryLogic $expectedResult): void
{
$actualResult = $type->isSubTypeOf($otherType);
$this->assertSame(
$expectedResult->describe(),
$actualResult->describe(),
sprintf('%s -> isSubTypeOf(%s)', $type->describe(VerbosityLevel::precise()), $otherType->describe(VerbosityLevel::precise()))
);
}

}