Narrow type of Collection::first() when using Collection::isEmpty()

[ruudk]

I really wanted support for Collection::isEmpty() together with Collection::first() as proposed in #153.

This adds tests and they pass.

However, I'm not too sure if the logic in the extension is correct.

When I change the code to:

$classReflection = $methodReflection->getDeclaringClass();
		$methodVariants = $classReflection->getNativeMethod(self::FIRST_METHOD_NAME)->getVariants();

		if ($context->truthy()) {
			return $this->typeSpecifier->create(
				new MethodCall($node->var, self::FIRST_METHOD_NAME),
				new ConstantBooleanType(false),
				$context
			);
		}

		return $this->typeSpecifier->create(
			new MethodCall($node->var, self::FIRST_METHOD_NAME),
			TypeCombinator::remove(ParametersAcceptorSelector::selectSingle($methodVariants)->getReturnType(), new ConstantBooleanType(false)),
			$context
		);

The test fails for this scenario:

if (!$collection->isEmpty()) {
	$entity = $collection->first();
	$entity; // 27: Variable $entity is: MyEntity|false
}

[ondrejmirtes]

I think the code is fine except for this one comment.

Your other code that you posted in your comment doesn't work, it does something else. When the context is truthy, the type in the second parameter is intersected with the current expression type.
When the context is falsey, the type in the second parameter is removed from the current expression type.

[ruudk]

@ondrejmirtes That explains it. Really clever!

I updated the PR and squashed the commits into one.

[ondrejmirtes]

If the tests with the lowest dependencies fail, it usually means that the inspected method (isEmpty/first) is missing in the installed dependency version...

[ruudk]

Hmm, I installed the lowest possible version:

$ composer update --prefer-lowest --no-interaction --no-progress --no-suggest
  - Downgrading doctrine/collections (1.6.7 => v1.4.0): Extracting archive

https://github.com/doctrine/collections/compare/v1.4.0..v1.6.2#diff-b2aa5c3356ae59544706f6b7a11003f05bfb44af519cf90444bbc357cf58f87cR64-R167

The methods do exist however the generic annotations are not there.

I tried to add them to the Stub file, but I guess that's not how it works. Any advice how to proceed?

[ondrejmirtes]

I'd just bump the dep version in require-dev...

[ruudk]

😁 Done!

[ondrejmirtes]

Thank you!

[ruudk]

❤️

Can this also be supported? Or impossible?

if (count($collection) > 0) {
	$entity2 = $collection->first();
	$entity2;
}

if ($collection->count() > 0) {
	$entity3 = $collection->first();
	$entity3;
}

[ondrejmirtes]

Right now it's not possible. We've had some relevant discussion here: phpstan/phpstan-src#355 (comment)

[VincentLanglet]

@ruudk @ondrejmirtes I tried by modifying the tests in https://github.com/phpstan/phpstan-doctrine/blob/master/tests/Type/Doctrine/Collection/data/collection.php and there is now the the following behavior

if ($collection->isEmpty()) {
        $collection->add(1);
	$first = $collection->first();
	$first; // Is considered as false.
}

Should we do something about it ? Like resetting the context when modifying the collection.

[ondrejmirtes]

@VincentLanglet Please open a new issue about it. It depends on what $collection->add(1); returns. If void then the method is considered impure and this shouldn't be happening. If the return type is different, we need to add @phpstan-impure above the method. See for more details: https://phpstan.org/blog/remembering-and-forgetting-returned-values

[VincentLanglet]

Done #203
I'll try to take a look in the week when I'll find time.