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

V2 and phpstan #623

Closed
norkunas opened this issue Jun 10, 2024 · 8 comments
Closed

V2 and phpstan #623

norkunas opened this issue Jun 10, 2024 · 8 comments

Comments

@norkunas
Copy link
Contributor

norkunas commented Jun 10, 2024

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 😑

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 😑

@nikophil
Copy link
Member

nikophil commented Jun 10, 2024

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 🤷
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 😅 and we'll see what to do... (maybe the solution would be to modify this return type with rector rules)

@nikophil
Copy link
Member

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
Copy link
Contributor Author

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

@nikophil
Copy link
Member

wow sorry, I haven't seen that

big 🤦 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 😬

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

@norkunas
Copy link
Contributor Author

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

@svianney
Copy link

svianney commented Sep 11, 2024

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

@nikophil
Copy link
Member

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

@nikophil
Copy link
Member

I'm closing this, since #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

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

No branches or pull requests

3 participants