zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
607 stars 62 forks source link

V2 and phpstan #623

Open norkunas opened 2 weeks ago

norkunas commented 2 weeks ago

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:

nikophil commented 2 weeks ago

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 commented 2 weeks ago

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

norkunas commented 2 weeks ago

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

nikophil commented 2 weeks ago

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:)

norkunas commented 2 weeks ago

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 :)