Fix branching querybuilder type inference

[VincentLanglet]

Hi,

I got an issue with a conditional query like

$qb = $em->createQueryBuilder();
if ($bool) {
    $qb->select('m.intColumn');
} else {
    $qb->select('m.intColumn', 'm.stringNullColumn');
}
$result = $qb->from(Many::class, 'm')->getQuery()->getResult();

which was considering to always returning array<array{intColumn: int}> which is wrong.

After debugging, I thought the issue was coming from TypeCombinator::union cf phpstan/phpstan#10694
After more debugging, I find out the behavior was different for covariant template or invariant template
https://phpstan.org/r/79c3c099-5d1d-4e36-9cf3-d6028d68415d

So I think the template for Query should not be covariant.

The covariant was added by you @ondrejmirtes in ec49b7a ; do you remember why ?

Thanks

[ondrejmirtes]

No, the problem is in https://github.com/phpstan/phpstan-doctrine/blob/1.4.x/src/Type/Doctrine/QueryBuilder/QueryBuilderType.php - it should override the isSuperTypeOf method and implement it correctly.

[VincentLanglet]

No, the problem is in 1.4.x/src/Type/Doctrine/QueryBuilder/QueryBuilderType.php - it should override the isSuperTypeOf method and implement it correctly.

I'm not sure to understand why it's QueryBuilderType. I tried to debug more and it's not called.

But after more debugging, I find out that during the execution of TypeCombinator::union(...$resultTypes);,
that this call
https://github.com/phpstan/phpstan-src/blob/952b313e94151d5e942e4f4577d42dd2c60d76b9/src/Type/TypeCombinator.php#L426
is changing the type from QueryType to GenericObjectType and then the issue occurs because of the call GenericObjectType::isSuperTypeOf when QueryType::isSuperTypeOf works fine.

As shown in my latest commit the testConditionalAddSelect pass with the override of changeSubtractedType.

This seems correct to me but some others tests are failing https://github.com/phpstan/phpstan-doctrine/actions/runs/8233591779/job/22513364391?pr=552:

• QueryBuilderDqlRuleSlowTest::testRuleBranches: one error was commented (

  phpstan-doctrine/tests/Rules/Doctrine/ORM/QueryBuilderDqlRuleSlowTest.php

  Lines 112 to 115 in 9c4935f

  	/*[
  	'QueryBuilder: [Semantical Error] line 0, col 93 near \'t.id = 1\': Error: \'t\' is not defined.',
  	90,
  	],*/

  ), seems like now I get it back so I think it's for the good. But I also get one line 107 which seems ok te me.
• QueryBuilderDqlRuleTest::testRuleBranches: same
• QueryBuilderDqlRuleTest::testBranchingPerformance: got two times the same message instead of one. I assume a array_unique will be necessary in the Rule.
• ReturnQueryBuilderExpressionTypeResolverExtensionTest::testFileAsserts got the "weird" type Doctrine\ORM\Query<null, QueryResult\Entities\Many>|Doctrine\ORM\Query<null, QueryResult\Entities\Many>. I think it's because the two query have different DQL but same results. Maybe QueryType::equals/QueryType::isSuperTypeOf need to be changed now.

WDYT @ondrejmirtes, do you agree with this analyse/new direction ?

[VincentLanglet]

The only failing test is the following one:

$branchingQuery = $this->getBranchingQueryBuilder($em)->getQuery();
assertType('Doctrine\ORM\Query<null, QueryResult\Entities\Many>', $branchingQuery);

I get

'Doctrine\ORM\Query<null, QueryResult\Entities\Many>|Doctrine\ORM\Query<null, QueryResult\Entities\Many>'

now.

This is kinda true because, if I call getDQL() on $branchingQuery we're getting an union type

$branchingQueryDQL = $this->getBranchingQueryBuilder($em)->getQuery()->getDQL();
assertType('\'SELECT m FROM QueryResult\\Entities\\Many m\'|\'SELECT m FROM QueryResult\\Entities\\Many m WHERE m.intColumn = 1\'', $branchingQueryDQL);

Should we:

• Update the test to assertType 'Doctrine\ORM\Query<null, QueryResult\Entities\Many>|Doctrine\ORM\Query<null, QueryResult\Entities\Many>' ?
• do a PR on phpstan-src to simplify display of type when possible (the internal PHPStan type could be 'Doctrine\ORM\Query<null, QueryResult\Entities\Many>|Doctrine\ORM\Query<null, QueryResult\Entities\Many>' but the display one 'Doctrine\ORM\Query<null, QueryResult\Entities\Many>').
• something else ?

[VincentLanglet]

Now phpstan/phpstan-src#2973 is merged, I just need to wait for the next release

[ondrejmirtes]

Released now https://github.com/phpstan/phpstan/releases/tag/1.10.63, please update it here

[VincentLanglet]

Released now phpstan/phpstan@1.10.63 (release), please update it here

Done.
I still had to update this line https://github.com/phpstan/phpstan-doctrine/pull/552/files#diff-60cf06ba47535191938faa998f36489178d29d99d8b2cbf39fac343871bec885R29

[ondrejmirtes]

Thank you.