Improve type specifier for identical operator

[jlherren]

This started as a small change and became much bigger than originally intended. So I'm pushing this early as a draft to get comments and see if this is going in a useful direction or not. Or if maybe this is a too big change for pre-1.0.

The intention here was

• Make type inference in conditions more precise in general
• Remove hard-coded things for certain functions (like count() and then just improve count-related extensions)
• Probably this would help fix False positive: Variable $a in isset() always exists and is not nullable. phpstan#2816 cleanly
• Future work would be to do the same for the == operator which would benefit switch-statements and fix Array size is known inside switch on count phpstan#1861

However I also hit some limitations, mostly that TypeSpecifierContext can only tell me if something was true/truthy/false/falsey, but not what exact type something evaluated to. Hence I added TypeSpecifier::specifyTypesInExpression(). But it would be nice to have more precise information available in extensions as well (MethodTypeSpecifyingExtension & co.)

Let me know what you think.

[ondrejmirtes]

Hi, thank you, but I need to orientate myself around the code a bit :) What's the significance of TypeUtils::isOneDefiniteType()? From my experience such code is usually wrong and I want to deprecate $x instanceof *Type anyway as I stated in a different PR :) Why do you need to know isOneDefiniteType() and why the code cannot be executed for example for unions of constant scalars?

[jlherren]

TypeUtils::isOneDefiniteType() checks whether a type represents only one specific exact type, e.g. it is not a "set of multiple possibilities" like most types are. This is important to know for handling NotIdentical. I hope I can illustrate that with this example:

/** @var int<1,10> $a */
/** @var 3 $b */
if ($a !== $b) {
    // $b is one definite type, thus it can be removed from $a
    // $a is now int<1,2>|int<4,10>
}

/** @var int<1,10> $a */
/** @var int<3,4> $b */
if ($a !== $b) {
    // $b is not one definite type, it cannot be removed from $a
    // nothing can be inferred about $a, which is still int<1,10>
}

Maybe the naming is not good. And maybe it should be a method of Type instead. In any case, getting rid of instanceof sounds like a good idea.

(I believe there are plans for making dependent types work, which would certainly help here.)

[ondrejmirtes]

Great, I think I get your intention :) I prefer smaller PRs, I see multiple things here:

• Sort IntegerRangeType numerically - can be a separate PR
• TypeSpecifierTest - some constants renaming - can be a separate PR

Basically we should differentiate between refactorings and fixed bugs/new features and do them separately.

That workflow makes things more managable - we can see if something breaks in between rather than after one large PR. And the time inbetween PRs gives us time to ponder about the solutions and maybe think about other edge-cases and things to test. It worked really well when we implemented generics over the course of 50 small PRs :)

About phpstan/phpstan#2816 - this one surprised me, I'm still a little confused about TypeSpecifierContext::truthy() vs. ::true() but I think the current bugfix is clean enough?

About dependent types - I see two different features here - dependence of variables certainty on other types ("if $bool is true then $a exists"), and dependence between types - if $a is Foo then $b is Bar. The API of Scope shouldn't change because of this. I think that MutatingScope instance should have some kind of matrix in it, and it should update the variable certainty and types based on filterTruthyValue. Let's take this scenario:

	/**
	 * @phpstan-return array{float_test: float, foo: 'bar', lall: string, noop: int, test: int}
	 */
	public function data(): array
	{
		/** @var mixed $array */
		$array = [];

		return $array;
	}

public function create_complex(): self {
		$self = new self();

		foreach($this->data() as $property => $value) {
			if ($property === 'test') {
				if ($self->{$property} === null) {
					$self->{$property} = new \stdClass();
				}
			} else {
				$self->{$property} = $value;
			}

			if ($property === 'foo') {
				$self->{$property} += 1;
			}
			if ($property === 'foo') {
				$self->{$property} .= ' ';
			}
			if ($property === 'lall') {
				$self->{$property} += 1;
			}
			$tmp = 1.1;
			if ($property === 'foo') {
				$self->{$property} += $tmp;
			}
		}

		return $self;
	}

PHPStan should know that when $property is float_test then $value can only ever be float. So the matrix should be used when I call $scope->filterByTruthyValue(new Identical(new Variable('property'), new String_('float_test'))), the new Scope should tell me that type of $value is float. Without such filtering the type of $value should stay float|int|string.

This is what usually needs to be done when there' a new property + constructor argument in Scope: d3e7b9c

Would you be interested in exploring that? I think that if you make yourself familiar with TypeSpecifier in the current PRs, you'd be able to try doing dependent types too :)

Thank you!

[jlherren]

Thanks for you reply!

The whole truthy vs true confuses me too. As far as I can tell it does not make a difference which one is used, which is why they are used interchangeably in the code. Ideally it would be possibly to give more precise types in the context, as I already mentioned. I might have a try at that at some point.

Dependent types certainly sounds interesting, but also challenging. For now I'm more comfortable fixing bugs and doing small improvements. I can try to have a go at a larger feature at a later point :)

I'll submit "Sort IntegerRangeType numerically" as a separate PR, it makes sense on its own. I included it here only because it made certain things easier.

As for this PR here, it can be closed, I'll try to break the ideas down into smaller PRs. Sometimes I have to explore and try things before figuring out what is the best way forward.

[ondrejmirtes]

I get what it's supposed to mean:

if ($a) {
    // $a is truthy - true, object, non-empty array, etc...
} else {
    // $a is falsey - false, empty array, 0, etc...
}

if ($a === true) {
    // $a is true
} else {
    // $a can be anything else besides true
}

What confuses me is when I don't care about the difference - $context->true() works for both true and truthy. And that's where it's probably used in a wrong way in some places...

[jlherren]

Yes exactly. But at that point it might just as well be useful to do

if (doFoo($a) === 17) { }

And then let DoFooFunctionTypeSpecifyingExtension know about the 17, so that it can tell us something about $a. That's currently not possible. I'd like to try to add this information to TypeSpecifierContext somehow. I can think of cases where this could be useful.

[ondrejmirtes]

I can think of only two cases:

• count($a) > 0 and similar with >=, ==, ===, <, <=, !==
• Same with strlen($a) for the upcoming non-empty-string type I want to create

I can get away with both being hardcoded in TypeSpecifier. I can't think of a userland function/method that would behave like that, do you have any examples? :)

[ondrejmirtes]

OK, closing this as there will be new PRs :) (but we can continue discussing this here)

[jlherren]

It could also be get_class($a) == 'Foo', or gettype($a) == 'string'. But yes I see that it's only useful for very limited cases. It's possible that additional package might want to use this (Doctrine, etc) but I can't think of any.

[ondrejmirtes]

Good examples :) I guess it would be nice not to have to hardcode those in TypeSpecifier. So if you make this somehow part of TypeSpecifierContext, it would be nice to prove the concept on actually implementing these examples in extensions themselves :)

[VincentLanglet]

@jlherren Do you plan to restart another PR ?

It would be nice to support

if (count($collection) > 0) {
	$entity2 = $collection->first();
	$entity2; // can't be false
}

if ($collection->count() > 0) {
	$entity3 = $collection->last();
	$entity3;  // can't be false
}