Improve QueryResultDynamicReturnTypeExtension

[VincentLanglet]

Same as #453 (comment)

With the commit 1b0a1b0

I changed the strategy to be easily accepted and then easier to merge:

• If I can precisely infer the type, I return the type
• If not, I rely on the default return type rather than returning an half-precise type like array<mixed> => This way we have the same behavior than before and people doesn't get error moving from level 9 to 7-8.

[ondrejmirtes]

Alright, I'm gonna look at this again after #506 is merged.

[VincentLanglet]

From #453 (comment)

Real world project number 2: 21 new errors, some queries use getScalarResult and getResult(AbstractQuery::HYDRATE_SCALAR). In these cases the inferred type is mixed which should be improved. Example query (anonymized):

 $qb = $this->createQueryBuilder('a');
 $result = $qb
            ->select('a.one, a.two, a.three')
            ->andWhere($qb->expr()->in('a.four', $four))
            ->orderBy('a.date', 'ASC')
            ->getQuery()
            ->getResult(AbstractQuery::HYDRATE_SCALAR);

This is fixed by the commit 1b0a1b0

Real world project number 1: 48 new errors, most are "Casting to int something that's already int.". A few real bugs found. But the PR is not currently usable because COUNT(pr.id) AS count as inferred as int<0, max>|numeric-string which will be solved by #506.

I'm curious about this "bug" because COUNT(pr.id) AS count is supposed to be inferred as __benevolent(int<0, max>|numeric-string) in this PR. It's tested here:
https://github.com/phpstan/phpstan-doctrine/pull/520/files#diff-28394df5dba3e85fe86f9c8d9b05d6087fe897d0efa1d0c0ad3533a1f0252b99R381-R413

Did you kept any example of query with your issue @ondrejmirtes ?

[ondrejmirtes]

I'm sorry, I want to wait for #506 to be finished before trying #520 again.

[ondrejmirtes]

Please fix the conflicts.

[janedbal]

Are you sure this ever works? Because QueryBuilderGetQueryDynamicReturnTypeExtension::METHODS_NOT_AFFECTING_RESULT_TYPE contains all where methods.

And if you remove those, you will break codebases using dynamic approaches in those methods (if those have doctrine.reportDynamicQueryBuilders: true).

[ondrejmirtes]

Hey, I just merged the most important part of this PR: 5745ea6

Please let me know if I missed anything and open a new PR, thanks.

[janedbal]

One note here: are we sure TypedExpressions are not wrongly deduced with this? Because getSingleScalarResult is NOT using result set mapping and thus it never gets casted by the type. Maybe it even affects more than just TypedExpressions

[janedbal]

If more hydratation modes are now supported, I believe all those getXxxResult calls should be added to platform test and verified that it matches.

[VincentLanglet]

One note here: are we sure TypedExpressions are not wrongly deduced with this? Because getSingleScalarResult is NOT using result set mapping and thus it never gets casted by the type. Maybe it even affects more than just TypedExpressions

I dunno bout TypedExpressions but I just found a special case, and a pretty simple one. BigInt field on Dbal 3.

• HYDRATE_OBJECT and HYDRATE_ARRAY is returning a numeric-string value
• HYDRATE_SCALAR, HYDRATE_SINGLE_SCALAR and HYDRATE_SCALAR_COLUMN are returning an int value.

But since the query is already considered as a Query<null, array{id: numeric-string}> I'm not sure it will be easy to get back the int value for such hydration mode.

Should revert most of the PR (HYDRATE_SCALAR / HYDRATE_SINGLE_SCALAR / HYDRATE_SCALAR_COLUMN) to only keep the HYDRATE_ARRAY inference (cc @ondrejmirtes) or do you see a way to improve the dbal inference @janedbal ?

[janedbal]

I'd need to dig deeper into it, but basically, for those hydratation modes you need to use getDatabaseInternalTypeForDriver and not getWritableToPropertyType in the walker. So if we know the mode beforehand, it should be easy. If not, hard.

[VincentLanglet]

I'd need to dig deeper into it, but basically, for those hydratation modes you need to use getDatabaseInternalTypeForDriver and not getWritableToPropertyType in the walker. So if we know the mode beforehand, it should be easy. If not, hard.

I don't think we know the mode before because the order things are done for $qb->getQuery()->getResult($hydrationMode) is

• On the getQuery call, compute the Query type in QueryResultTypeWalker to hydrate the templates of the stub

/**
 * @template-covariant TKey The type of column used in indexBy
 * @template-covariant TResult The type of results returned by this query in HYDRATE_OBJECT mode
 */
abstract class AbstractQuery

• Then, on the getResult compute the result with the hydration mode and the Query<TKey, TResult> type already computed.

That's why it may require to have something like

/**
 * @template-covariant TKey The type of column used in indexBy
 * @template-covariant TResult The type of results returned by this query in HYDRATE_OBJECT mode
 * @template-covariant TResult2 The type of results returned by this query in HYDRATE_SCALAR mode
 */
abstract class AbstractQuery

which is not really satisfying

[janedbal]

Yeah, that is not very nice.

Btw, we are using only analysable methods since the query type inference was added and you actually dont need all the other methods. Everything can be replaced by the supported ones. We have this backed up by a rule. Thereotically, such rule can be even in phpstan-doctrine under some flag.

[janedbal]

Should revert most of the PR

So I'd say yes.

[VincentLanglet]

Btw, we are using only analysable methods since the query type inference was added and you actually dont need all the other methods. Everything can be replaced by the supported ones. We have this backed up by a rule. Thereotically, such rule can be even in phpstan-doctrine under some flag.

I don't understand

Should revert most of the PR

So I'd say yes.

Done #588

[janedbal]

I'm just saying that codebase can still have 100% type coverage in terms of DQLs even when some of the methods are not supported:

• getScalarResult or getArrayResult
  • replace by getResult
• getSingleScalarResult
  • replace by getSingleResult(Query::HYDRATE_OBJECT)['column']
• getSingleColumnResult
  • replace by getResult() + array_column

and pass Query::HYDRATE_OBJECT to getOneOrNullResult, getSingleResult, toIterable and you are safe.

[VincentLanglet]

The main method i use was getSingleColumnResult, I find silly to always have to add an extra array_column.

But yeah this can be avoided...

[janedbal]

Silly, but type-safe :)

[VincentLanglet]

I have trouble to understand why we have a such different behavior with hydration mode. I created doctrine/orm#11527 to know more about it if you're interested.

But I feel like you may already know the answer, or at least know way more things than me about this subject.

[janedbal]

If you look at AbstractHydrator descendants, you will se that some (e.g. SingleScalarHydrator) is not using parent's ResultSetMapping. And that is the reason why result types are different.