foundry icon indicating copy to clipboard operation
foundry copied to clipboard

V2 and phpstan

Open norkunas opened this issue 1 year ago • 5 comments

I have another problem now that blocks from completely migrating to v2. And there is nothing about this in upgrade guide.

In a factory I have protected function defaults(): array and phpstan complains:

  22     Method App\Foundry\Factory\MyFactory::defaults() return type has no value type specified in iterable type array.                                                                     
         🪪  missingType.iterableValue                                                                                                                                                            
         💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type                                                                                                
  22     Return type (array) of method App\Foundry\Factory\MyFactory::defaults() should be covariant with return type (array<string, mixed>|(callable(int): array<string, mixed>)) of method  
         Zenstruck\Foundry\Factory<App\Entity\MyEntity&Zenstruck\Foundry\Persistence\Proxy<App\Entity\MyEntity>>::defaults()                                                                          
         🪪  method.childReturnType

If I add /** @return array<mixed> */ above defaults method, I get:

  25     Return type (array) of method App\Foundry\Factory\MyFactory::defaults() should be covariant with return type (array<string, mixed>|(callable(int): array<string, mixed>)) of method  
         Zenstruck\Foundry\Factory<App\Entity\MyEntity&Zenstruck\Foundry\Persistence\Proxy<App\Entity\MyEntity>>::defaults()                                                                          
         🪪  method.childReturnType  

if I update return typehint for defaults method to : array|callable to be consistent with Zenstruck\Foundry\Factory, then I get:

  22     Method App\Foundry\Factory\MyFactory::defaults() never returns callable(int): array<string, mixed> so it can be removed from the return type.  
         🪪  return.unusedType 

If I add @phpstan-import-type Attributes from Factory and /** @return Attributes */, then I get:

  28     Method App\Foundry\Factory\MyFactory::defaults() return type has no value type specified in iterable type array.                                                                     
         🪪  missingType.iterableValue                                                                                                                                                            
         💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type                                                                                                
  28     PHPDoc tag @return with type array<string, mixed>|(callable) is not subtype of native type array.                                                                                        
         🪪  return.phpDocType                                                                                                                                                                    
  28     Return type (array) of method App\Foundry\Factory\MyFactory::defaults() should be covariant with return type (array<string, mixed>|(callable(int): array<string, mixed>)) of method  
         Zenstruck\Foundry\Factory<App\Entity\MyEntity&Zenstruck\Foundry\Persistence\Proxy<App\Entity\MyEntity>>::defaults()                                                                          
         🪪  method.childReturnType  

What's the way here to fix this? If return type is defined in the parent interface/class then it should be inferred, at least that works with any other part of the code in our app and phpstan doesn't complain, but now we are here :expressionless:

Edit: ok found that it works if defining return type as array<string, mixed>, but that's just nonsense to add this to 100+ factories :expressionless:

norkunas avatar Jun 10 '24 12:06 norkunas

Hey @norkunas

thank you for this report.

but that's just nonsense to add this to 100+ factories

you're absolutely right, we're gonna find a solution!

After some times scratching my head looking for an explanation, I think I've found the problem: https://phpstan.org/r/a7f5de6f-afaf-4972-8d51-a6932bfd9cc8

It seems that PHPStan "lose" the inherited array definition if the return type is changed :shrug: so I think that it thinks the prototype of the method is array<array-key, mixed> which is not covariant with array<string, mixed>

I think as an easy fix, you could replace all return types from array to array|callable. Would you try and tell me if it works for you?

I've asked on PHPStan's GitHub if this is a bug or a feature :sweat_smile: and we'll see what to do... (maybe the solution would be to modify this return type with rector rules)

nikophil avatar Jun 10 '24 16:06 nikophil

ok so this is a bug in PHPStan... I think the simplest solution is to add return type array|callable everywhere. I'll add a rector rule for this

nikophil avatar Jun 11 '24 08:06 nikophil

But then if you don't return a callable, phpstan complains also and says to narrow type, this is included in issue description :)

norkunas avatar Jun 11 '24 09:06 norkunas

wow sorry, I haven't seen that

big :facepalm: here

that's strange it did not occurred to me... is that coming from level 9? (I'm only using level 8 on my projects)

I think we're stuck for now and I hate this :grimacing:

I got plans to create a PHPStan extension for Foundry where we can fix this, but I don't know when I'll got enough time. (And I expect Psalm users to show up, with some related problems :shrug:)

nikophil avatar Jun 11 '24 09:06 nikophil

Yup, max level/bleeding edge :)

I think we're stuck for now and I hate this 😬

I just added everywhere @return array<string, mixed> for now, so no rush :)

norkunas avatar Jun 11 '24 09:06 norkunas

Hi,

I'm not sure if I should open a new issue but since v2.0.8 I've encountered problems with Psalm and the fact that the createOne methods is defined like this

    /**
     * @param Attributes $attributes
     *
     * @return T
     */
    final public static function createOne(array|callable $attributes = []): mixed
    {
        return static::new()->create($attributes);
    }

I use the factory normally like this:

EndConversationNodeFactory::createOne([
    'script' => $script,
]);

and the error I'm getting is

InvalidArgument - src/DataFixtures/AppFixtures.php:126:78 - Argument 1 of App\Foundry\Factory\Node\EndConversationNodeFactory::createOne expects Zenstruck\Foundry\Persistence\Attributes, but array{script: Zenstruck\Foundry\Persistence\Proxy<T:Zenstruck\Foundry\Persistence\PersistentProxyObjectFactory as object>} provided (see https://psalm.dev/004)

So Psalm doesn't expects me to put an array as an argument, and I'm not sure how to fix this or even if I can fix this. So everywhere in my codebase where I pass an array as attributes of ::createOne or ::new methods, I get this Psalm error. For now I've ignored 2.0.8+ versions but I'd really like to tackle this and resume the upgrade of this wonderful package.

Can you help me on this matter please ? If will be happy to provide you with more information if needed.

EDIT: I use Psalm 5.26.1 btw

svianney avatar Sep 11 '24 07:09 svianney

@svianney would you mind opening an issue related to your problem, please?

nikophil avatar Sep 11 '24 08:09 nikophil

I'm closing this, since https://github.com/zenstruck/foundry/issues/701 has been opened, and we are stuck with a PHPStan's bug.

I might create a PHPStan extension some day, which will fix the problem, and this already has its own issue

nikophil avatar Oct 18 '24 15:10 nikophil