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

[MatteoFeltrin]

fixes https://github.com/doctrine/orm/issues/11451

[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 :thinking:

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.