bug: Model::find() returns incorrect type

[kenjis]

PHP Version

8.1

PHPStan CodeIgniter Version

v1.4.2.70400

PHPStan Version

1.10.39

What happened?

 ------ ------------------------------------------------------------------------------ 
  Line   app/Models/ThreadModel.php                                                    
 ------ ------------------------------------------------------------------------------ 
  177    Cannot access property $thread_count on array<int, App\Entities\Category>.    
  177    Cannot access property $thread_count on array<int, App\Entities\Category>.    
  178    Cannot access property $post_count on array<int, App\Entities\Category>.      
  183    Cannot access property $id on array<int, App\Entities\Category>.              
  187    Cannot access property $last_thread_id on array<int, App\Entities\Category>.  
  188    Cannot access property $last_thread_id on array<int, App\Entities\Category>.  
  195    Cannot use -- on array|bool|float|int|object|string|null.                     
 ------ ------------------------------------------------------------------------------ 

https://github.com/kenjis/forum-example/blob/37a339f050ee2c40ed452e6570cef188dc0f90a0/app/Models/ThreadModel.php#L173-L195

Minimum Reproduction Script

git clone https://github.com/kenjis/forum-example --branch=add-phpstan-codeigniter
cd forum-example/
composer install
vendor/bin/phpstan

Expected Output

No errors on ThreadModel.

[paulbalandan]

Except for the last error, the errors are coming because PHPStan does not know what is the type of $data['data']['category_id']. Doing a dump of its type:

diff --git a/app/Models/ThreadModel.php b/app/Models/ThreadModel.php
index fac36b3..74d0dfc 100644
--- a/app/Models/ThreadModel.php
+++ b/app/Models/ThreadModel.php
@@ -172,6 +172,8 @@ class ThreadModel extends Model

         $categoryModel = model(CategoryModel::class);

+        \PHPStan\dumpType($data['data']['category_id']);
+
         // Update stats for new category
         $newCategory = $categoryModel->allowCallbacks(false)->find($data['data']['category_id']);
         $newCategory->thread_count++;

will give :175 Dumped type: mixed

It needs to know that the category_id index is either string or null. You can solve this by setting up an array shape to the parameter $data. Or do an assert call on category_id to make sure it is int|string.

[paulbalandan]

Assuming you did an assert call instead to is_int.

diff --git a/app/Models/ThreadModel.php b/app/Models/ThreadModel.php
index fac36b3..e5b8711 100644
--- a/app/Models/ThreadModel.php
+++ b/app/Models/ThreadModel.php
@@ -171,6 +171,8 @@ class ThreadModel extends Model
         }

         $categoryModel = model(CategoryModel::class);
+        
+        assert(is_int($data['data']['category_id']));

         // Update stats for new category
         $newCategory = $categoryModel->allowCallbacks(false)->find($data['data']['category_id']);

The errors now would be:

------ -----------------------------------------------------------
  Line   app\Models\ThreadModel.php
 ------ -----------------------------------------------------------
  :179   Cannot use ++ on array|bool|float|int|object|string|null.
  :197   Cannot use -- on array|bool|float|int|object|string|null.
 ------ -----------------------------------------------------------

At the moment, this extension does not yet understand the casts of Entities.

[kenjis]

Why does it assume array<int, App\Entities\Category> when $id is mixed?

[paulbalandan]

Because of these lines:

phpstan-codeigniter/src/Type/ModelFindReturnTypeExtension.php

Lines 87 to 97 in d4f77ab

	if ($idType->isNull()->yes()) {
	return $this->getTypeFromFindAll($methodReflection, $methodCall, $scope);
	}
	if ($idType->isInteger()->yes() || $idType->isString()->yes()) {
	$classReflection = $this->getClassReflection($methodCall, $scope);
	return TypeCombinator::addNull($this->modelFetchedReturnTypeHelper->getFetchedReturnType($classReflection, $methodCall, $scope));
	}
	return $this->getTypeFromFindAll($methodReflection, $methodCall, $scope);

If the type is mixed, and since the parameter type of $id is array|int|string|null, the executed line will be the last line because the type will be simplified to array|null.

[kenjis]

At the moment, this extension does not yet understand the casts of Entities.

Why the casts of Entities that can be understood without specifying a type cannot be understood with specifying type with assert()?

[paulbalandan]

By default, PHPStan does not know what is the relation of $casts to the properties of Entities. We need to teach it that the content of $casts influence the type of the properties.

[kenjis]

Ah, by default PHPStan does not know the Entiy property types. I misunderstood it.

[kenjis]

The logic below is not correct after all in many cases. Because guessed mixed does not means it is really array.
This case codeigniter4projects/playground#218 (comment) is also an incorrect error.

phpstan-codeigniter/src/Type/ModelFindReturnTypeExtension.php

Lines 87 to 97 in d4f77ab

	if ($idType->isNull()->yes()) {
	return $this->getTypeFromFindAll($methodReflection, $methodCall, $scope);
	}
	if ($idType->isInteger()->yes() || $idType->isString()->yes()) {
	$classReflection = $this->getClassReflection($methodCall, $scope);
	return TypeCombinator::addNull($this->modelFetchedReturnTypeHelper->getFetchedReturnType($classReflection, $methodCall, $scope));
	}
	return $this->getTypeFromFindAll($methodReflection, $methodCall, $scope);

The last line should be the second return, because finding single record is the most use cases for find().
And if $id is surely an array, it should return the last return.

[paulbalandan]

If we'll make the last line as an if check to array, then we still need to account for all other cases.

Like, if passed id is not array|int|string|null does the framework throw an error so should the type be never?

[kenjis]

I mean like this.

            function (Type $idType, callable $traverse) use ($methodReflection, $methodCall, $scope): Type {
                if ($idType instanceof UnionType || $idType instanceof IntersectionType) {
                    return $traverse($idType);
                }

                if ($idType->isNull()->yes() || $idType->isArray()->yes()) {
                    return $this->getTypeFromFindAll($methodReflection, $methodCall, $scope);
                }

                if ($idType->isInteger()->yes() || $idType->isString()->yes()) {
                    $classReflection = $this->getClassReflection($methodCall, $scope);

                    return TypeCombinator::addNull($this->modelFetchedReturnTypeHelper->getFetchedReturnType($classReflection, $methodCall, $scope));
                }

                // We don't know the correct type, we assume it is a int or string.
                $classReflection = $this->getClassReflection($methodCall, $scope);

                return TypeCombinator::addNull($this->modelFetchedReturnTypeHelper->getFetchedReturnType($classReflection, $methodCall, $scope));
            }

[paulbalandan]

I think the current code is correct. Looking at the code of Model::doFind, the check is first see if id is array, or check if string or int, then it has an else encompassing all other cases which returns all records.

[kenjis]

See the product code. $newCategory is a single record, not an array.

[paulbalandan]

It's because at runtime the id resolves to either a string or null. However, phpstan does not run the code. Thus, it thinks that the id is mixed.

[kenjis]

Yes, but the mixed is incorrect, or at least thinking $newCategory is an array is wrong.
Aside from what the internal process is doing, this extension often reports false positive errors on find().

[MGatner]

I might be stating something stupidly obvious and not helpful but... wouldn't a conditional return on find() be enough to differentiate the two completely different uses?

https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types

[paulbalandan]

It wouldn't be easy. You would need to annotate that the each calling model returns a return type (array, Entity, custom object, stdClass). Maybe generics could do the job? Also, you need to note that when asArray() and asObject() is called the return type also changes.

[kenjis]

I already sent codeigniter4/CodeIgniter4#8187

[MGatner]

Thanks you two. I think all of the work going into this is telling what a mess Model/BaseModel is. 😓