larastan / larastan

⚗️ Adds code analysis to Laravel improving developer productivity and code quality.
MIT License
5.59k stars 422 forks source link

Description of relationships #2071

Open choco-cat opened 1 month ago

choco-cat commented 1 month ago

Description

Hi. How to describe relationships correctly? To my description

    /**
     * @return BelongsTo<User, Rate>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

an error occurs

 Method App\Rate\Models\Rate::user() should return Illuminate\Database\Eloquent\Relations\BelongsTo<App\Rate\Models\Rate<App\User\Models\User>> but returns
         Illuminate\Database\Eloquent\Relations\BelongsTo<App\User\Models\User, $this(App\Rate\Models\Rate)>.

Laravel code where the issue was found

    /**
     * @return BelongsTo<PariUser, Rate>
     */
    public function pariUser(): BelongsTo
    {
        return $this->belongsTo(PariUser::class);
    }
calebdw commented 1 month ago

This should be:

    /**
     * @return BelongsTo<User, $this>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }
calebdw commented 1 month ago

@canvural, I had new docs in #1990 that didn't get merged, can you cherry-pick that? 17bce5f783f19258312aaf8a45e3cbbec335b144

szepeviktor commented 1 month ago

Or just avoid the @return tag.

calebdw commented 1 month ago

Or just avoid the @return tag.

The @return is useful for IDE autocompletion on the method call---and you can avoid a costly model file parse if you include them

koraga commented 1 month ago

This should be:

    /**
     * @return BelongsTo<User, $this>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

Same error:

    /**
     * @return BelongsTo<User, $this>
     */
    public function third(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }
Method App\Rate\Models\Rate::third() should return Illuminate\Database\Eloquent\Relations\BelongsTo<App\Admin\Models\User, App\Rate\Models\Rate> but   
         returns Illuminate\Database\Eloquent\Relations\BelongsTo<App\Admin\Models\User, $this(App\Rate\Models\Rate)>.                                          
         💡 Template type TDeclaringModel on class Illuminate\Database\Eloquent\Relations\BelongsTo is not covariant. Learn more:                               
            https://phpstan.org/blog/whats-up-with-template-covariant   
calebdw commented 1 month ago

Make sure to clear any cache, phpstan clear-result-cache

encodiaweb commented 1 month ago

Make sure to clear any cache, phpstan clear-result-cache

Thanks for this tip, but unfortunately, it didn't solve. Same errors as @koraga

koraga commented 1 month ago
    /**
     * @return BelongsTo<User, covariant $this>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

There is no error if I add covariant

calebdw commented 1 month ago

Adding covariant is not the answer

This was working on #1990 (and it is working on my fork). Something likely was messed up when cherry-picking #1990

calebdw commented 1 month ago

What does the full Model look like? By any chance are they final?

koraga commented 1 month ago

What does the full Model look like? By any chance are they final?

Yes, all my models are final

calebdw commented 1 month ago

Yes, all my models are final

Ah, that's it---I can replicate it now. This looks like a PHPStan bug, I'll open an issue over there

davideprevosto commented 1 month ago

My Models are not final, but I am getting the same issue.

calebdw commented 1 month ago

@davideprevosto, then there's also a bug in Larastan because non-final models work just fine on my fork

NickSdot commented 1 month ago

Same issue here. Cache cleared etc. On my end it is not related to final/non-final. Caleb, I tried your fork and have the same result. Worth to mention that this is old code that used to work without reports.

/** @return BelongsTo<\App\Models\Company, \App\Models\MultisiteInquiry> */
public function company(): BelongsTo
{
    return $this->belongsTo(Company::class, 'company_id');
}
phpstan: Method App\Models\MultisiteInquiry::company() should return
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, App\Models\MultisiteInquiry> but returns
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, $this(App\Models\MultisiteInquiry)>.

Seems to me like it is a regression in Larastan 2.9.9.

Works? PHPStan Larastan
1.12.5 2.9.8
1.12.6 2.9.8
1.12.5 2.9.9
1.12.6 2.9.9
calebdw commented 1 month ago

@NickSdot,

This error is correct, your generic should be:

/** @return BelongsTo<\App\Models\Company, $this> */
NickSdot commented 1 month ago

@NickSdot,

This error is correct, your generic should be:

/** @return BelongsTo<\App\Models\Company, $this> */

Thanks @calebdw. Not trying to argue, but please allow me to double confirm:

1) Is it really intentional that a patch release makes my previouly working doc blocks to errors now? There is a ton of code on GitHub which is not using this/self, and it very likely worked before 2.9.9 (as it did in our projects).

2) In a Laravel model, where we define the relation, $this === App\Models\MultisiteInquiry. Why would one work, and one not? Mind to elaborate on the technical difference for SA? I am having a hard time to see what is the added value of this breaking change.

3) Using $this in a final class results in the below, which is what you reported here. So it definitely didn't work until now, which is indicating a new behaviour. However, the model FQN worked just fine until the last release.

phpstan: Method App\Models\MultisiteInquiry::company() should return
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, App\Models\MultisiteInquiry> but returns
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, $this(App\Models\MultisiteInquiry)>.
calebdw commented 1 month ago

@NickSdot,

Yes it is intentional, and $this vs self matters when you have models that extend other models to insure the generics have the right model (the child and not the parent that the relation is defined on). This change actually fixed several bugs related to Model inheritance and as PHPStan has stated, sometimes fixing bugs / causing the code to be smarter causes new errors to be revealed.

Using $this is the most correct as the framework literally passes the $this instance into the Relation constructor: https://github.com/laravel/framework/blob/d2895c733d738200964c6209714e37c20f568789/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L111

NickSdot commented 1 month ago

@NickSdot,

Yes it is intentional, and $this vs self matters when you have models that extend other models to insure the generics have the right model (the child and not the parent that the relation is defined on). This change actually fixed several bugs related to Model inheritance and as PHPStan has stated, sometimes fixing bugs / causing the code to be smarter causes new errors to be revealed.

Using $this is the most correct as the framework literally passes the $this instance into the Relation constructor:

https://github.com/laravel/framework/blob/d2895c733d738200964c6209714e37c20f568789/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L111

Thank you for taking the time to explain! I get the intention.

Would have been nice if there would have been a fade-out phase until the next major were both the most correct $this, and the previous FQN approach would have worked in parallel. However, I know there is never a right time.

The above just as an aside. I appreciate all your hard work here, and in other repos. Thanks.🤝

calebdw commented 1 month ago

There was talk about releasing this in v3, but we're going to release v3 when PHPStan is bumped to v2 so it was just decided to go ahead and release this to support Laravel >11.15

both the most correct $this, and the previous FQN approach would have worked in parallel.

This isn't as easy to do as it sounds. The dynamic return type extension has to return one value so it's kind of one or the other without a ton of complicated checks

he above just as an aside. I appreciate all your hard work here, and in other repos. Thanks.🤝

Thank you! You are too kind---I'm always happy to help :smile:

kayw-geek commented 1 month ago
 Line   app/Models/Support/FooTrait.php (in context of class App\Models\Bar)                                                              
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------- 
  90     Method App\Models\Bar::getDate() should return Illuminate\Database\Eloquent\Collection<int, App\Models\Bar> but  
         returns Illuminate\Database\Eloquent\Collection<int, static(App\Models\Bar)>.  

The static method getDate() is declared in the FooTrait located in app/Models/Support/FooTrait. It returns a static::query(), and using this FooTrait in a final class will trigger the aforementioned error.

calebdw commented 1 month ago

@kayw-geek,

There's an open issue at PHPStan. In the meantime you can either: 1) ignore the error, or 2) remove final from the model in question