Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Infer getArrayResult #593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ondrejmirtes wants to merge 2 commits into 1.4.x
base: 1.4.x
Choose a base branch
Loading
from get-array-result
Draft

Infer getArrayResult #593

ondrejmirtes wants to merge 2 commits into 1.4.x from get-array-result

Conversation

Copy link
Member

@ondrejmirtes ondrejmirtes commented Jun 30, 2024

No description provided.

Copy link
Contributor

Hi, maybe it's better to continue the discussion #592 (comment) here (cc @janedbal)

There is a lot of failure on this draft but if I understand correctly the one exposed by your reproducer is

70) PHPStan\Type\Doctrine\QueryBuilderReproductionsTest::testFileAsserts with data set "/home/runner/work/phpstan-doctrine/phpstan-doctrine/tests/Type/Doctrine/data/queryBuilderReproductions.php:28" ('type', '/home/runner/work/phpstan-doc...ns.php', 'list<array{id: int}>', 'list<array{id: int|null}>', 28)
Expected type list<array{id: int}>, got type list<array{id: int|null}> in /home/runner/work/phpstan-doctrine/phpstan-doctrine/tests/Type/Doctrine/data/queryBuilderReproductions.php on line 28.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'list<array{id: int}>'
+'list<array{id: int|null}>'

which is similar to the sentence

I'd love if we could make phpstan-doctrine understand that where/andWhere can make a selected field non-nullable for example.

I'm sure you've already noticed that HYDRATE_OBJECT already infer as list<array{id: int|null}>.

For the where situation I had last time a "solution" to use a BenevolentUnion #447 ; I dunno if the PR handled the IDENTITY/join issues but it may be possible to do it.
Maybe this could be a good first step ?

This would allow HYDRATE_OBJECT to be inferred as list<array{id: __benevolent<int|null>}>, and avoid some false positive. WDYT ? Or do you (@ondrejmirtes or @janedbal) see a way to understand when a nullable field is not null ?

Copy link
Contributor

janedbal commented Jul 1, 2024

My point of view:

  • It should be possible to implement proper deduction of "this field is not null due to this WHERE" by traversing the DQL AST (big effort tho)
    1. benevolent union is smart hack to avoid some issues but (ii)
    2. phpstan-doctrine will never have 100% accurate types for all codebases, thus will always need inline vardocs or other methods to adjust those types
      • (e.g. when you pass QB to another method, you have no idea what happens there)

Copy link
Contributor

If I update the "failing test" you added @ondrejmirtes by changing getArrayResult to getResult

$result = $this->entityManager->createQueryBuilder()
			->select('DISTINCT IDENTITY(oi.product) AS id')
			->from(OrderItem::class, 'oi')
			->join('oi.product', 'p')
			->getQuery()
			->getResult(); // or getArrayResult

-> with getResult it's inferred as list<array{id: int|null}>.
-> with getArrayResult is inferred as array.

If the fact that getArrayResult is not inferred as list<array{id: int}>, shouldn't it have been a blocker for getResult too ?

Currently I solve my issues with changing every call to getArrayResult in my code base to getResult in order to have PHPStan inferring the code ; but I would expect to have at least the same behavior than getResult.

janedbal reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /