Get AST data from `$queryReflection->getResultType`

[jakubvojacek]

Hello @staabm

creating an issue instead of PR since I do not know how to tackle exactly this one (its related to dibi only I guess).

AST is great since it can tell me the SELECT count(*) from ada will always return one column, minimum 0. I often use code like

$count = $this->database->fetchSingle('SELECT count(*) from ada');

now phpstan will think that $count is nullable integer because of https://github.com/staabm/phpstan-dba/blob/main/src/Extensions/DibiConnectionFetchDynamicReturnTypeExtension.php#L120 but in fact its always 0...max integer and never null

But I am not sure how could I update DibiConnectionFetchDynamicReturnTypeExtension, when to use TypeCombinator::addNull and when not. I am not sure if there are other functions apart from count(*) that will always return row even when the table is empty.

If I had info in the resultType that count was used, I could remove TypeCombinator::addNull, something like

if ($resultType->getMethod() === \SqlFtw\Sql\Expression\BuiltInFunction::COUNT)  dont add null

How do you think I could achieve this?

[staabm]

interessting stuff... the addNull is there because you added it with https://github.com/staabm/phpstan-dba/pull/478 for the case of aggregate functions, to enforce it can get null :)

I need to think about it

[staabm]

I wonder whether these addNull's are correct or whether we should leave this decision to the SQL AST type inference

[jakubvojacek]

the AST cannot know this I think. fetchSingle will return first column from first row.

For example using your database structure.

$this->database->fetchSingle('select adaid from ada')

AST will tell me that adaid is int<0,max> and never null, right? But fetchSIngle will correctly return int<0,max>|null because it will be null when ada is empty.

But in case

$this->database->fetchSingle('select count(*) from ada')

right now it will return int|null but in reality it should never be null

[staabm]

shouldn't the dibi api then also return null for $this->database->fetchSingle('select count(*) from ada') when ada is empty? (I am asking stupid questions since I never used dibi)

[jakubvojacek]

no, because it will never be null, it will be some number (the count), in case of empty table 0

[jakubvojacek]

the motivation behind this is that I started using new phpstan rules https://github.com/shipmonk-rnd/phpstan-rules, ru and it now throws phptan error that I am comparing int|null > int in code

$count = $this->database->fetchSingle('select adaid from ada');

if ($count > 0) {
    ....
}

[staabm]

I see thx.

per your comment

$count = $this->database->fetchSingle('select adaid from ada');
if ($count > 0) {
    ....
}

can return null, so it will not help you against the error the shipmonk rule is reporting.

it would only help for similar logik with a different query (like your count case):

$count = $this->database->fetchSingle('select count(adaid) from ada');
if ($count > 0) {
    ....
}

right?

[jakubvojacek]

yes, my bad, was copying the query and chose the wrong one. should have been the count query

[staabm]

do other apis like mysqli, mysql, pdo etc also return null when 'select adaid from ada' is used on a empty table?

[jakubvojacek]

i do not work with other apis but I havent seen a method that works like fetchSingle - that returns only the first column from the first row only. They usually have methods to fetch row, fetch assoc but not fetchOneSingle field.

[staabm]

After my first morning coffee, I think there is an easy solution:

within the class which implements your database access implement

public function fetchCount($query): int {
  $count = $this->fetchSingle($query);
  if ($count === null) {
    throw new RuntimeException('count() can never be null');
  }
  return $count;
}

and use it instead of what you have right now:

$count = $db->fetchCount('select count(adaid) from ada');

I don't think we can solve it in phpstan-dba itself. I will close the issue now. feel free to comment/discuss further though.

[jakubvojacek]

well there certainly are some dirty workaround that would solve it phpstan-dba, first one coming to mind

--- a/vendor/staabm/phpstan-dba/src/Extensions/DibiConnectionFetchDynamicReturnTypeExtension.php
+++ b/vendor/staabm/phpstan-dba/src/Extensions/DibiConnectionFetchDynamicReturnTypeExtension.php
@@ -88,7 +88,7 @@ final class DibiConnectionFetchDynamicReturnTypeExtension implements DynamicMeth
                 return null;
             }

-            $fetchResultType = $this->reduceResultType($methodReflection, $resultType);
+            $fetchResultType = $this->reduceResultType($methodReflection, $resultType, $queryString);
             if (null === $fetchResultType) {
                 return null;
             }
@@ -106,7 +106,7 @@ final class DibiConnectionFetchDynamicReturnTypeExtension implements DynamicMeth
         return null;
     }

-    private function reduceResultType(MethodReflection $methodReflection, Type $resultType): ?Type
+    private function reduceResultType(MethodReflection $methodReflection, Type $resultType, string $queryString): ?Type
     {
         $methodName = $methodReflection->getName();

@@ -117,6 +117,10 @@ final class DibiConnectionFetchDynamicReturnTypeExtension implements DynamicMeth
         } elseif ('fetchPairs' === $methodName && $resultType instanceof ConstantArrayType && 2 === \count($resultType->getValueTypes())) {
             return new ArrayType($resultType->getValueTypes()[0], $resultType->getValueTypes()[1]);
         } elseif ('fetchSingle' === $methodName && $resultType instanceof ConstantArrayType && 1 === \count($resultType->getValueTypes())) {
+            if (str_starts_with($queryString, 'select count(')) {
+                return $resultType->getValueTypes()[0];
+            }
+
             return TypeCombinator::addNull($resultType->getValueTypes()[0]);
         }

but its too hacky to be merged, I guess :)

[staabm]

but its too hacky to be merged, I guess :)

absolutely. ;)

and my above example is pretty clean on your end

[jakubvojacek]

yes i guess i will do that since I use my fork of dibi anyway. But might have to do custom phpstan rule that will check query and if it starts with select count(, it will scream if used with fetchSingle instead of fetchCount (since I know I'd forget to use it sometimes)

Ok thanks :+1:

[staabm]

phpstan will cry at you because of the NULL value anyway - but sure why not :)