Improve comparison operators and fix IntegerRangeType overflowing

[jlherren]

No description provided.

[ondrejmirtes]

Hi, would you care to rewrite this as a new NodeScopeResolverTest::testFileAssert's dataProvider? It would be much clearer to see assertType('false', 1.9 > 2.1) instead of jumping between this file and rule test. Thanks!

[jlherren]

Sure thing.

[jlherren]

Done.

[ondrejmirtes]

As a future scope, we should probably think about rewriting this as a method on Type. That way, we could also write implementation of == and also nicely write implementation between different ConstantArrayTypes for example.

Remember, I want to deprecate and get rid of most instanceof *Type. But I'll merge this PR as it is once the other comment is resolved :)

Thank you!

[jlherren]

Right! Actually, I can try to have a go at this tomorrow. So it would be a new method smallerThan() and smallerThanOrEqualTo() method on Type?

[ondrejmirtes]

public function smallerThan(self $otherType): TrinaryLogic;
public function smallerThanOrEqual(self $otherType): TrinaryLogic;

Most of these method implementations have a special if condition for CompoundType which are implemented by special types like UnionType or IntersectionType, it might look like this:

	public function accepts(Type $type, bool $strictTypes): TrinaryLogic
	{
		if ($type instanceof static) {
			return TrinaryLogic::createYes();
		}

		if ($type instanceof CompoundType) {
			return $type->isAcceptedBy($this, $strictTypes);
		}

		return TrinaryLogic::createNo();
	}


	public function isSuperTypeOf(Type $type): TrinaryLogic
	{
		if ($type instanceof self) {
			return TrinaryLogic::createYes();
		}

		if ($type instanceof CompoundType) {
			return $type->isSubTypeOf($this);
		}

		return TrinaryLogic::createNo();
	}

It's there so that each type can only show interest in types that it should care about. All of these CompoundTypes are handled by a single if in most ordinary Type implementations which makes things easier. I don't know how to name the relevant CompoundType method, maybe isGreaterThan, as an inversion similar to accepts/isAcceptedBy? :) You'll see...

[jlherren]

Please see the update, I hope it is somewhat how you intended it to be. I opted to only add one method, since in almost all implementations < and <= can be handled with the same code. If desired I could still add isSmallerThanOrEqual(), which would then just call isSmallerThan(..., ..., true).

Also I added the is prefix to the method names (IMHO method names should always include at least one verb). This makes it also more similar to a probable future isEqualTo(), which needs to be clearly distinguished from equals().

Finally, I made IntegerRangeType implement CompoundType, I hope that makes sense, because it really needs to handle isGreaterThan().

[jlherren]

Hold on with this... I noticed some weirdness. The following code:

use function PHPStan\dumpType;

function (int $d): void {
	if (0 <= $d--) {
		dumpType($d);
	} else {
		dumpType($d);
	}
};

Now produces:

  10     Dumped type: int<-1, 9223372036854775806>      
  12     Dumped type: 9223372036854775807|int<min, -2>  

While it originally produces saner output: https://phpstan.org/r/fca7522c-939b-4199-83ca-5db5970f0637

This means that pre/post increment/decrement did not have proper tests :) I will investigate this and add tests.

[jlherren]

Okay, that's fixed. I added the tests for pre/post inc/dec and noticed that some situations weren't even handled, I added those as well.

[ondrejmirtes]

Thank you! I wonder if the current isSmallerThan methods could be implemented by preserving what originally was in MutatingScope with the isSuperTypeOf logic and just improving specific Type::isSuperTypeOf implementations to also fix ranges like that.

[jlherren]

I'm not entirely sure what you mean... Possibly it's what I'm doing right now: Making TypeSpecifer infer correct types for if ($a < $b) when either or both of $a and $b are IntegerRangeType, ConstantIntegerType, ConstantFloatType, UnionType, IntersectionType.

My current approach is to add Type::getSmallerType() and Type::getGreaterType(). For example (new ConstantIntegerType(17))->getSmallerType() would return mixed~int<17, max>.

[ondrejmirtes]

What I mean is that in theory the original code should stay in MutatingScope:

		if ($rightType instanceof ConstantIntegerType) {
				$rightValue = $rightType->getValue();

				if ($node instanceof Expr\BinaryOp\Greater) {
					$rightIntervalType = IntegerRangeType::fromInterval($rightValue + 1, null);
				} elseif ($node instanceof Expr\BinaryOp\GreaterOrEqual) {
					$rightIntervalType = IntegerRangeType::fromInterval($rightValue, null);
				} elseif ($node instanceof Expr\BinaryOp\Smaller) {
					$rightIntervalType = IntegerRangeType::fromInterval(null, $rightValue - 1);
				} else {
					$rightIntervalType = IntegerRangeType::fromInterval(null, $rightValue);
				}

				return $rightIntervalType->isSuperTypeOf($leftType->toInteger())->toBooleanType();
...

Instead of adding new Type::isSmallerThan() method, we could continue expressing this operation using Type::isSuperTypeOf and open-ended IntegerRangeType (that has infinity on one side).

[jlherren]

But then it would not work for "bar" < "foo", null < true, etc. Or am I wrong?

Given $a < 10, we cannot check that int<min, 9> is a super type of $a, because $a might not be an integer at all (it might be -7.1 and then it should still return true). It previously worked anyway because of the ->toInteger() but that was buggy and made 12.5 < 12 to be considered true.

[ondrejmirtes]

Of course, you're right :)