Result infering: support indexBy and lists #384
Conversation
|
/cc @arnaud-lb Can you review this? Thank you :) |
src/Type/Doctrine/Query/QueryResultDynamicReturnTypeExtension.php
Outdated
Show resolved
Hide resolved
| doctrine: | ||
| objectManagerLoader: entity-manager.php | ||
| featureToggles: | ||
| listType: true |
There was a problem hiding this comment.
Not sure I can keep this here if we would merge this.
|
|
||
| private function getQueryIndexType(Type $queryType): Type | ||
| { | ||
| if (!$queryType instanceof GenericObjectType) { |
There was a problem hiding this comment.
I think this should rather be PHPStan\Type\Doctrine\Query\QueryType, but this was used below.
| parent::__construct('Doctrine\ORM\Query', [ | ||
| $resultType ?? new MixedType(), | ||
| $indexType ?? new MixedType(), | ||
| ]); |
There was a problem hiding this comment.
I think that $indexType can legitimately be mixed, depending on the type of the field. As an effect, getResult() would be typed as list<...>, which is not right.
Maybe we could introduce a subtype of Query<TResult>, like IndexedQuery<TIndex,TResult> ?
This would also avoid the BC break of adding a new type parameter to Query.
There was a problem hiding this comment.
Right now we're incompatible with Psalm #377 so I wouldn't avoid the change now.
There was a problem hiding this comment.
What about using some artificial custom type for this purpose? Like ListIndexMarkerType extends MixedType?
There was a problem hiding this comment.
I think I fixed this issue, hopefully.
Default value of QueryType is still MixedType. But only when we get valid answer from QueryResultTypeBuilder::getIndexType, we get either ListIndexMarkerType or the type of indexBy type (can be mixed). But the extension now infers lists only for ListIndexMarkerType so we should be getting list<T> only in those cases where it really passed the QueryResultTypeWalker without any issues.
There was a problem hiding this comment.
If we want to be compatible with Psalm, we need to put the TIndex param before TResult as well.
The artificially custom type is a good idea. Will the user be able to type a list Query in type annotations like in the following snippet ?
/** @return Query<ListQuery,Foo> */
function getQuery(): Query;There was a problem hiding this comment.
Maybe we could introduce a subtype of
Query<TResult>, likeIndexedQuery<TIndex,TResult>?
I'm withdrawing this suggestion, it wouldn't work.
After all we should probably add TKey, as we will have to do this change if we add full INDEX BY support anyway, and this makes us compatible with Psalm at the same time.
Users will have to change Query<Something> to Query<int,Something> for non-indexed queries or Query<mixed,Something> for indexed queries (and to something more specific than mixed if we add full INDEX BY support).
We use phpstan-doctrine infering quite lot in our codebase, but we never needed to use the generics ourselves. Do you @arnaud-lb use it in userland?
I used the Query<...> or AbstractQuery<...> types on only a few occasions, but it would have been very inconvenient if the result of some createQuery() call couldn't be accepted by any Query<...> type. It would be good enough if Query<mixed,Something> or Query<int,Something> accepts a QueryType that have TKey=ListIndexMarkerType (I don't know if it does currently, we may need to declare TKey as @template-covariant, which would be correct).
PHPDoc types are resolved in TypeNodeResolver, and plugins can implement TypeNodeResolverExtensions to add custom types: https://phpstan.org/developing-extensions/custom-phpdoc-types
Alternatively, we could bind TKey to the void type to denote a non-indexed Query. We bind TResult to void to denote non-SELECT queries already.
There was a problem hiding this comment.
Users will have to change Query to Query<int,Something> for non-indexed queries or Query<mixed,Something> for indexed queries (and to something more specific than mixed if we add full INDEX BY support).
I'm ok with this BC. I can swap the types. Will do that.
It would be good enough if Query<mixed,Something> or Query<int,Something> accepts a QueryType that have TKey=ListIndexMarkerType
As ListIndexMarkerType exteds MixedType, any key type accepts it, even Query<string, Something>. Maybe we could make ListIndexMarkerType extends IntegerType, that would be more precise and should handle your cases as described. Will do that.
Alternatively, we could bind TKey to the void type to denote a non-indexed Query. We bind TResult to void to denote non-SELECT queries already.
Personally, I see the custom type as better option than abusing existing type with some meaning.
Anything else that needs to be done before merging this?
There was a problem hiding this comment.
PHPDoc types are resolved in TypeNodeResolver, and plugins can implement TypeNodeResolverExtensions to add custom types: https://phpstan.org/developing-extensions/custom-phpdoc-types
I can add support of some list-marker phpdoc for the ListIndexMarkerType, but I think it might be confusing for people seeing it error messages. But the truth is that this "hidden" type is not ideal as well (it is outputted as any other mixed now).
There was a problem hiding this comment.
- Swapped TKey and TResult so that we are psalm-compatible
- ListIndexMarkerType extends IntegerType
There was a problem hiding this comment.
I'd love to finalize this. I'm just waiting for your answers / review. Thanks.
src/Type/Doctrine/Query/QueryResultDynamicReturnTypeExtension.php
Outdated
Show resolved
Hide resolved
|
I think you can rebase/retrigger the ci to fix the 8.2 builds |
Rebased |
|
I took this PR and reworked it with some changes according to my taste: 4490e56
Thank you :) |
4 participants
Closes #381