Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

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.

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

Merged
ondrejmirtes merged 1 commit into phpstan:master from ruudk:doctrine-collection-first
Feb 9, 2021

Conversation

@ruudk
Copy link
Contributor

@ruudk ruudk commented Feb 9, 2021

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
}

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

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 ruudk force-pushed the doctrine-collection-first branch from c2f10c6 to 9ce54d8 Compare February 9, 2021 14:16
Copy link
Contributor Author

ruudk commented Feb 9, 2021

@ondrejmirtes That explains it. Really clever!

I updated the PR and squashed the commits into one.

@ruudk ruudk changed the title (削除) Doctrine collection first (削除ここまで) (追記) Narrow type of Collection::first() when using Collection::isEmpty() (追記ここまで) Feb 9, 2021
Copy link
Member

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

Copy link
Contributor Author

ruudk commented Feb 9, 2021

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?

Copy link
Member

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

@ruudk ruudk force-pushed the doctrine-collection-first branch from 0e726dd to fdba25e Compare February 9, 2021 14:48
Copy link
Contributor Author

ruudk commented Feb 9, 2021

😁 Done!

@ondrejmirtes ondrejmirtes merged commit 2486f59 into phpstan:master Feb 9, 2021
Copy link
Member

Thank you!

@ruudk ruudk deleted the doctrine-collection-first branch February 9, 2021 14:58
Copy link
Contributor Author

ruudk commented Feb 9, 2021 •
edited
Loading

❤️

Can this also be supported? Or impossible?

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

Copy link
Member

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

ruudk reacted with thumbs up emoji

Copy link
Contributor

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

Copy link
Member

@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

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /