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.

Sign up for GitHub

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

Use templating to allow a more flexible type inference in QueryBuilder::setParameters() method #11454

Open
wants to merge 1 commit into
base: 3.1.x
Choose a base branch
from
Open

Use templating to allow a more flexible type inference in QueryBuilder::setParameters() method #11454

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/QueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,13 +467,15 @@
* )));
* </code>
*
* @psalm-param ArrayCollection<int, Parameter> $parameters
* @psalm-param ArrayCollection<TKey, Parameter> $parameters
*
* @return $this
*
* @template TKey of int
Copy link
Member

@greg0ire greg0ire May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What's the point? We're not getting something out of this knowledge about TKey, right? Maybe we should instead tell Psalm that it's OK to pass a more specific collection, because setParameters is not going to mutate it. I believe that's what @psalm-external-mutation-free is about. Can you try annotating setParameters with that annotation instead?

Also, please add a test for what you are fixing under tests/StaticAnalysis.

Copy link
Member

Choose a reason for hiding this comment

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

The point here is that ArrayCollection<int, ...> is too restrictive over the given ArrayCollection when inference comes from call-time parameters.

If you declare new ArrayCollection(['a', 'b']), you get an ArrayCollection<1|2, string>, rather than an ArrayCollection<int, string>, which is then rejected by this QueryBuilder method.

This generalizes TKey over int, while before int specifically was required. It effectively says "we don't care what kind of int the key is, as long as it is within the bounds of this generic type.

Copy link
Member

@greg0ire greg0ire May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't wondering what the point of this change was, but whether there was the point of holding the type in a dedicated TKey name (since we don't use it). I would prefer a syntax allowing any subtype of int without having to come up with a name, but if that does not exist, let's go with that. I tried my own suggestion BTW, and it does not address the issue, I'm not sure why… I don't understand why exactly ArrayCollection<1|2, string> cannot be accepted where ArrayCollection<int, string> is. Well, I think thought that was because the method could alter the type, but that does not seem to be it.

Here is the minimal test code I used:

<?php

declare(strict_types=1);

namespace Doctrine\StaticAnalysis;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\QueryBuilder;

function foo(EntityManagerInterface $em): void
{
    $qb = new QueryBuilder($em);
    $qb->setParameters(new ArrayCollection([
        new Parameter('customStringId', 42),
        new Parameter('license', 'string'),
    ]));
}

It might be nice to store it under tests/StaticAnalysis/query-builder.php, to prevent people from breaking this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a problem of altering the type. This problem occours because when you declare an ArrayCollection<int, string> you are forcing the templating inside ArrayCollection class to be restricted only to int type.
When you instanciate a new ArrayCollection passing an array <0,1> as parameters, the templating now is forced to only int<0,1> as type, which is more specific than the one you are asking for.
Using templating inside setParameters() allows the type to be inferred from the input parameter, with the only constrain of "extending" the int type.

*/
public function setParameters(ArrayCollection $parameters): static
{
$this->parameters = $parameters;

Check failure on line 478 in src/QueryBuilder.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Property Doctrine\ORM\QueryBuilder::$parameters (Doctrine\Common\Collections\ArrayCollection<int, Doctrine\ORM\Query\Parameter>) does not accept Doctrine\Common\Collections\ArrayCollection<TKey of int, Doctrine\ORM\Query\Parameter>.

Check failure on line 478 in src/QueryBuilder.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (3.8.2, phpstan-dbal3.neon) 

Property Doctrine\ORM\QueryBuilder::$parameters (Doctrine\Common\Collections\ArrayCollection<int, Doctrine\ORM\Query\Parameter>) does not accept Doctrine\Common\Collections\ArrayCollection<TKey of int, Doctrine\ORM\Query\Parameter>.

return $this;
}
Expand Down
Loading