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

[MatteoFeltrin]

fixes #11451

[greg0ire]

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.

[Ocramius]

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.

[greg0ire]

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.

[MatteoFeltrin]

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.

[greg0ire]

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

[MatteoFeltrin]

Not sure which is the best way to fix the Psalm error. Should we put another @template TKey of int over the $parameters property or should we move the templating to class level?

[Ocramius]

@MatteoFeltrin is it possible to declare the property as templated too? Property Doctrine\ORM\QueryBuilder::$parameters?

Moving at class level could be a bit of a mess 🤔

Alternatively, suppress inline, for now?

[greg0ire]

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 getParameters() returns, but I don't think it would be super helpful to anyone.

[MatteoFeltrin]

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

In 2.19 was already forcely templated with int, but it's only since version 3.1 that the mixed[] as argument was not accepted anymore.
Should I fix it from 2.19 or we can ignore it?

[MatteoFeltrin]

I have tried to add the templating to Property Doctrine\ORM\QueryBuilder::$parameters, the problem is that we have 2 different templates that may not be able to work togheter...

My templated property:

    /**
     * @template TKey of int
     *
     * @psalm-var ArrayCollection<TKey, Parameter>
     */
     private ArrayCollection $parameters;

May be, for example, a ArrayCollection<0-1, Parameter>

While the argument i'm passing to setParamters may be an ArrayCollection<0-8, Parameter>, so Psalm is, rightly, complaining of this:

$this->parameters with declared type 'Doctrine\Common\Collections\ArrayCollection<Doctrine\ORM\TKey, Doctrine\ORM\Query\Parameter>' cannot be assigned type 'Doctrine\Common\Collections\ArrayCollection<TKey:fn-doctrine\orm\querybuilder::setparameters as int, Doctrine\ORM\Query\Parameter>'

We may just suppress the error on setParameters as @Ocramius suggested, but I have tried to put the templating at class level on our company project where we use many times the QueryBuilder and I got no errors about missing templating...

@template-covariant doesn't seem to help

Will wait for your feed on how to proceed...

[greg0ire]

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
So maybe it's safe to ignore it. Using the templating on the class level, even if it raises no new issue, feels weird to me.

[MatteoFeltrin]

Hmmm, now that you pointed it out I noticed is a problem of PHPStan, not Psalm...
When i run it locally I get a slightly different message:

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>.
         💡 Template type TKey on class Doctrine\Common\Collections\ArrayCollection is not covariant. Learn more: https://phpstan.org/blog/whats-up-with-template-covariant

Let's just suppress it?

[greg0ire]

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 @psalm-external-mutation-free does not fix it: in my mind:

1. Psalm notices that it's not just int, but int<0,1> can be inferred.
2. It fears loosing its precious type information because setParameters would mutate the collection.
3. @psalm-external-mutation-free should be enough to ensure that's not the case.

[Ocramius]

@psalm-external-mutation-free is not related to this issue: the problem is not around mutation semantics, but just type inference.

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;

• in PHPSTan
• in Psalm

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 TKey (collections are invariant due to many issues around read/write semantics), where the more precise inferred type becomes a problem, since the type checker tries to assume invariance of the given type.

<?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']));

• in PHPStan
• in Psalm

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 int range:

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

[greg0ire]

Thanks @Ocramius , it's more clear now, and indeed, let's ask upstream before changing too many things here.

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[Ocramius]

This still depends on upstream: it's worth fixing, but unsure when that may happen.