Skip to content
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

Infer getArrayResult #593

Draft
wants to merge 2 commits into
base: 1.4.x
Choose a base branch
from
Draft

Infer getArrayResult #593

wants to merge 2 commits into from

Conversation

ondrejmirtes
Copy link
Member

No description provided.

@VincentLanglet
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 ?

@janedbal
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)

@VincentLanglet
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants