-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use templating to allow a more flexible type inference in QueryBuilder::setParameters()
method
#11454
base: 3.1.x
Are you sure you want to change the base?
Conversation
…er::setParameters()` method
* | ||
* @return $this | ||
* | ||
* @template TKey of int |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good. Does it apply to lower branches? I'm thinking it would qualify as a forward-compatibility improvement and should therefore go to 2.20
Not sure which is the best way to fix the Psalm error. Should we put another |
@MatteoFeltrin is it possible to declare the property as templated too? Moving at class level could be a bit of a mess 🤔 Alternatively, suppress inline, for now? |
Let's indeed not move it at the class level, as it would mean static analysis would ask users to specify the template parameters in many places. The only reason to do that would be to provide more insight about what |
In 2.19 was already forcely templated with |
I have tried to add the templating to My templated property: /**
* @template TKey of int
*
* @psalm-var ArrayCollection<TKey, Parameter>
*/
private ArrayCollection $parameters; May be, for example, a While the argument i'm passing to
We may just suppress the error on
Will wait for your feed on how to proceed... |
If there is no issue with 2.19, let's fix this on 3. Weird, I don't get that second issue with the parameter assignment when trying it on psalm.dev: https://psalm.dev/r/444d4274aa |
Hmmm, now that you pointed it out I noticed is a problem of PHPStan, not Psalm...
Let's just suppress it? |
I asked for guidance here: https://symfony-devs.slack.com/archives/C8SFXTD2M/p1716213222050249 I'd really like to understand why the original issue you observe is only a Psalm issue, and why
|
This is generally a big topic in both Psalm and PHPStan BTW: the inferred type should be "strict enough to be the most precise possible, but also lax enough to pass any usage locations". As a comparison, see: <?php declare(strict_types = 1);
/**
* @template TKey of array-key
* @template TValue of mixed
*/
final class ArrayCollection
{
/** @param array<TKey, TValue> $items */
public function __construct(public readonly array $items) {}
}
$c = new ArrayCollection(['foo' => 'bar']);
/** @psalm-trace $c */
return $c; This is not really a bug in either: it's a decision on how to infer a type. The problem occurs though when considering the variance of <?php declare(strict_types = 1);
/**
* @template TKey of array-key
* @template TValue of mixed
*/
final class ArrayCollection
{
/** @param array<TKey, TValue> $items */
public function __construct(public readonly array $items) {}
}
/** @param ArrayCollection<int, string> $v */
function usesAStringKeyedCollection(ArrayCollection $v): void {
echo var_export($v, true);
}
usesAStringKeyedCollection(new ArrayCollection(['foo', 'bar'])); What I can also do is bring this up with PHPStan / Psalm in upstream, to see if there's another angle to this problem. As a new observation to this problem, I noticed that Psalm seems to accept the type when keys don't form an usesAStringKeyedCollection(new ArrayCollection(['foo', 'bar'])); // doesn't work
usesAStringKeyedCollection(new ArrayCollection([0 => 'foo', 1 => 'bar'])); // doesn't work
usesAStringKeyedCollection(new ArrayCollection([0 => 'foo', 2 => 'bar'])); // works
usesAStringKeyedCollection(new ArrayCollection(['foo'])); // works
usesAStringKeyedCollection(new ArrayCollection([0 => 'foo'])); // works
usesAStringKeyedCollection(new ArrayCollection([1 => 'foo'])); // works @MatteoFeltrin I think we should bring it up in @vimeo/psalm |
Thanks @Ocramius , it's more clear now, and indeed, let's ask upstream before changing too many things here. |
fixes #11451