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.

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

Generify collections using Psalm #177

Merged
merged 18 commits into from Mar 24, 2019

Conversation

@nschoellhorn
Copy link
Contributor

commented Mar 13, 2019

I've talked to @Ocramius and we find that generics/template types would make a very good addition to this library. Since PHP doesn't come with that feature natively, I have decided to implement it using Psalm and Psalm's template annotations.

How is that useful?
A lot of people already use static analysis to reduce the risk of introducing bugs in your application. Psalm is one of the most popular tools out there to do that. By adding the annotations to Psalm and using the correct @psalm-var annotations when using the collections in projects, Psalm makes sure that you only put values of the right type in your collection automatically, since it also scans the vendor directory.
I have created a very minimal proof of concept project over at nschoellhorn/psalm-collections-test. You can clone that, install the dependencies and run ./vendor/bin/psalm to check it out.

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Huge 👍 from me on this.

This is meta-code that provides immediate value for downstream consumers that use Psalm, without any runtime being affected. The only possible issues that might arise are with doctrine/annotations and people that are (erroneously) trying to parse them from vendor/

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

To be checked: scrutinizer-ci failure (can't run composer install)

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Needed to bump the PHP version to allow the installation of Psalm to complete. Should be no issue though, since it went from 7.1.0 only up to 7.1.3

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@muglug @weirdan can you glance over this, if you have some time?

@weirdan

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@Ocramius sure, will do.

@weirdan
Copy link
Contributor

left a comment

Overall, it might make sense to use two generic parameters for Collection: Collection<TKey,TVal>, so that people may narrow down the key type further:

/** @psalm-var Collection<string,string> $coll */
$coll = new ArrayCollection;
$coll->containsKey(42); // then Psalm will be able to flag this as invalid
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved psalm.xml.dist Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Criteria.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/AbstractLazyCollection.php Outdated
@muglug
Copy link
Contributor

left a comment

Looks great!

Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
@@ -170,6 +198,8 @@ public function next();
* @param Closure $p The predicate.
*
* @return bool TRUE if the predicate is TRUE for at least one element, FALSE otherwise.
*
* @psalm-param Closure(int|string, T):bool $p

This comment has been minimized.

Copy link
@muglug

muglug Mar 13, 2019

Contributor

@psalm-param Closure(int|string=, T=):bool $p should have the same effect I think?

Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
@@ -209,6 +248,9 @@ public function map(Closure $func);
* @return Collection[] An array with two elements. The first element contains the collection
* of elements where the predicate returned TRUE, the second element
* contains the collection of elements where the predicate returned FALSE.
*
* @psalm-param Closure(int|string, T):bool $p
* @psalm-return Collection<T>[]

This comment has been minimized.

Copy link
@muglug

muglug Mar 13, 2019

Contributor

You can also use @psalm-return array{0: Collection<T>, 1: Collection<T>} for explicitness

Show resolved Hide resolved lib/Doctrine/Common/Collections/Criteria.php Outdated

Accepted as a sign I agree with this. Some work still left to do though

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Thank you for the review guys, especially @weirdan for all the suggestions. Going through them atm, but I think we should agree on whether to use array-key or an additional template parameter for the collection key? I am currently more in favor of array-key since it comes closer to Java's Collection API, which I am generally a fan of. But that's very subjective. What do you guys think?

weirdan added a commit to weirdan/doctrine-psalm-plugin that referenced this pull request Mar 13, 2019

Fixed wrong Collection::exists() signature
Found while reviewing doctrine/collections#177

Also:
- Limited key type to array-key
- Added future tests, pending psalm implementation (see vimeo/psalm#1462)
@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

or an additional template parameter for the collection key?

The key can be int|string, and should be a template parameter, because the possible values for it are int, string and int|string, as far as I know.

@weirdan

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@muglug if all those ArrayCollection docblocks were replaced with {@inheritDoc}, would Psalm inherit the signatures from Collection interface?

@weirdan

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@nschoellhorn You will need to adjust ArrayCollection signatures as well (things like first, next, etc).

@alcaeus
Copy link
Member

left a comment

👍 I'm in favour of this. We should try to allow our users to make full use of the static analysis tools they provide. Since psalm provides functionality around generics, this makes perfect sense. Once phpstan supports something like this, we should add that as well. Of course, I'd prefer it if @muglug and @ondrejmirtes could come up with an annotation that doesn't include a tool prefix, but that's for them to settle, not us.

Last but not least, checking our code with phpstan and psalm is also a plus as there are some things that are not detected in both tools, so this should improve the code quality as well.

@ondrejmirtes

This comment has been minimized.

Copy link

commented Mar 14, 2019

could come up with an annotation that doesn't include a tool prefix

Yeah, I'm usually more aggressive about annotation adoption, I'm not worried about using new syntax in at-param, at-var etc. Thanks to shoving intersection types there, they are currently supported by PhpStorm and other tools, which was my goal (driving innovation).

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@muglug if all those ArrayCollection docblocks were replaced with {@inheritDoc}, would Psalm inherit the signatures from Collection interface?

Sadly, Psalm does not seem to catch that. I tried that with my test repository that I linked above and when running Psalm after removing the annotations from ArrayCollection::add(), it doesn't complain anymore. So I think we have to keep the annotations in there.

@nschoellhorn You will need to adjust ArrayCollection signatures as well (things like first, next, etc).

Yeah, still at it. Going for an additional template parameter TKey as array-key now, so people can either make a collection of type Collection<array-key, something> if they don't care about the exact type of their key or they can restrict to either int or string if they like.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Sadly, Psalm does not seem to catch that. So I think we have to keep the annotations in there.

While I'm all in for adding psalm specific notations here (since there doesn't exist any standard for this so far), I'm not in favor of incorporating psalm bug specific workarounds. This is something that should be fixed by upstream (psalm), not us having to copy these to every subclass.

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

I'm not in favor of incorporating psalm bug specific workarounds.

Must back this too: doctrine/collections, being very central to the PHP ecosystem, should not have workarounds, as the types do indeed propagate statically, which will affect downstream consumers. Therefore, while this patch can indeed be polished and finished up, we'll need to address the upstream issue in psalm before merging.

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I'd also prefer that. Maybe @muglug can check if there's something that I've done wrong, causing the inheritance not to work properly. If it's not my fault, I'd offer submitting an issue to Psalm.

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Well, that seems to be fixed: https://github.com/vimeo/psalm/releases/tag/3.2

@muglug

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Yup, thanks for letting me know! Templated inheritance is non-trivial. I already had it working for templated return types, but had neglected templated param types.

@nschoellhorn nschoellhorn changed the title Generify collections using Psalm [WIP] Generify collections using Psalm Mar 14, 2019

@weirdan
Copy link
Contributor

left a comment

I think it'll be good to merge after this round of suggestions.

Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/ArrayCollection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/ArrayCollection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/ArrayCollection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/AbstractLazyCollection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/AbstractLazyCollection.php Outdated
@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

After another round of improvements (thanks @weirdan), we are now only missing the discussion on the (possibly) optional parameters of the closures for map(), filter(), etc.

@weirdan
Copy link
Contributor

left a comment

Collection::partition() should probably be able to accept argless closures. Other than that this looks fine.

Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated
Show resolved Hide resolved lib/Doctrine/Common/Collections/Collection.php Outdated

nschoellhorn added some commits Mar 15, 2019

Update Psalm and roll back workaround
Signed-off-by: Niklas Schoellhorn <schoellhorn.niklas@gmail.com>
Merge dependency changes
Signed-off-by: Niklas Schoellhorn <schoellhorn.niklas@gmail.com>

@nschoellhorn nschoellhorn force-pushed the nschoellhorn:master branch from dc5004b to 067a137 Mar 18, 2019

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Thank you @muglug for the fast update once again. I've bumped the dependency of Psalm to the new version and rolled back the change regarding uasort(). No issues anymore.

@Ocramius
Copy link
Member

left a comment

@doctrine/doctrinecore can I have a vote on this, please?

@SenseException
Copy link
Contributor

left a comment

Looks neato.

If some psalm annotations change in the classes/interfaces, would this break psalm checks of other projects using Collections?

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Yes, type checks propagate statically

@SenseException

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Should we have some kind of tests that prevent logical changes in the psalm annotations of doctrine/collections?

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

That would be like roave/backwards-compatibility-check: the ecosystem doesn't have that kind of stuff yet (even BC checker is way too new)

@alcaeus alcaeus requested a review from Majkl578 Mar 18, 2019

@weirdan

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Should we have some kind of tests that prevent logical changes in the psalm annotations of doctrine/collections?

You can take a look at https://github.com/weirdan/doctrine-psalm-plugin/blob/master/tests/acceptance/Collections.feature - most of those tests would apply here as well. Feel free to borrow them.

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Looks good, I can add that if wanted.

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Let's please defer that for now

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Or better: separate patch (based on this one)

@nschoellhorn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Sure, I will prepare one soon then.

@Ocramius

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Right, calling the merge here 🚢

@Ocramius Ocramius merged commit d348a61 into doctrine:master Mar 24, 2019

2 checks passed

Scrutinizer Analysis: No new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Ocramius Ocramius added this to the 1.6.0 milestone Mar 24, 2019

@Ocramius Ocramius self-assigned this Mar 24, 2019

@Ocramius Ocramius referenced this pull request May 27, 2019

Closed

Built-in assertion and mock type definitions #3708

10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.