Generify collections using Psalm

[nschoellhorn]

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]

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]

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

[nschoellhorn]

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]

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

[weirdan]

@Ocramius sure, will do.

[weirdan]

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

[muglug]

Looks great!

[muglug]

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

[muglug]

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

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

[nschoellhorn]

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?

[Ocramius]

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]

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

[weirdan]

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

[alcaeus]

👍 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]

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]

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

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]

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]

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]

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

[muglug]

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.

[weirdan]

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

[nschoellhorn]

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]

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

[nschoellhorn]

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]

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

[SenseException]

Looks neato.

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

[Ocramius]

Yes, type checks propagate statically

[SenseException]

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

[Ocramius]

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)

[weirdan]

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]

Looks good, I can add that if wanted.

[Ocramius]

Let's please defer that for now

[Ocramius]

Or better: separate patch (based on this one)

[nschoellhorn]

Sure, I will prepare one soon then.

[Ocramius]

Right, calling the merge here 🚢