Larastan: Take advantage of `@mixin` in Laravel's code itself

[ondrejmirtes]

Hi,
PHPStan's dev-master now supports @mixin annotation (https://github.com/phpstan/phpstan-src/commit/b02ee1444440ed995c17171c456feec2c56e0cb2). I've been told that Laravel's codebase uses this annotation heavily, so maybe Larastan could take advantage of it. Maybe we could delete some of Larastan's custom code if the only thing it does is describing Laravel magic that's also described by the @mixin annotation.

What I'd like to see happen:

1) Require phpstan/phpstan: ^0.12.20 (soon to be released, in a matter of days) so that people that would update Larastan but not PHPStan are not left without this functionality.
2) Go through extensions that describe Laravel's code that uses @mixin and try to delete them/reduce them.

(If you don't find any code in Larastan that can be deleted thanks to @mixin support, then ^0.12.20 doesn't have to be required.)

Thank you!

[ondrejmirtes]

Also, one more idea - it's also possible that Larastan's describes some code from Laravel that would justify adding @mixin annotation to Laravel itself. In that case, you can just create a stub file for that class in Larastan with @mixin and it would work.

[nunomaduro]

If I remember correctly, we are already using those mixings somewhere on the larastan codebase. We need to check.

[ondrejmirtes]

It might be possible to simplify that code, or maybe somehow re-use this extension: https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/Mixin/MixinMethodsClassReflectionExtension.php

[canvural]

Actually we were doing this manually. Here: https://github.com/nunomaduro/larastan/blob/master/src/Methods/Pipes/Mixins.php Looking for @mixin and @see in the class docs, and searching the missing method in those classes.

So, I guess we can remove that. I'll look into it :+1:

[canvural]

laravel-ide-helper package adds @mixin \Eloquent to the models docblock when you generate ide helper files.

\Eloquent is a class alias to Illuminate\Database\Eloquent\Model So basically it is adding a mixin for the class' actual parent. This is to have some auto-completion in the IDE.

And I found out this kinda breaks Larastan's extensions. Because when a missing method is found PHPStan will try to look for that method in \Eloquent because of the @mixin. And when our extensions run they are getting the Illuminate\Database\Eloquent\Model as the class reflection. So they are producing the wrong return types. For example we have an extension to determine the return type of find method on the model. If the argument is iterable a collection should be returned, model otherwise. But with @mixin \Eloquent added, we don't know the real model name. We just have Model

But also laravel-ide-helper can be configured to write the mixin inside of the _ide_helper.php file. So I'd say we don't need to fix or workaround for this. We just need to notify people about this in upgrade guide or readme.

[ondrejmirtes]

Why is Larastan even reading anything that laravel-ide-helperproduces? It should read the application source code written by its developers, no?

[canvural]

laravel-ide-helper modifies the model files written by the developers. It adds the @mixin to the models php docblocks.

[canvural]

But like I said it can be configured to write it to a dedicated file. _ide_helper_models.php In that case it'll not conflict with Larastan/PHPStan.

[ondrejmirtes]

Can you explain the thing in https://github.com/nunomaduro/larastan/issues/555#issuecomment-624297770 in a bit more detail? What class has @mixin, what PHPStan sees vs. what it should see instead, what ClassReflection reports... maybe I can think of a solution, but I didn't get it what's exactly wrong from the comment. I'm not sure about the relationships between the classes.

[ondrejmirtes]

1) I could add an exclude option to the mixin extension that would Larastan set.
2) Maybe I’ll figure out how to get the info we need even in the current system.

[canvural]

/**
 * @mixin \Eloquent
 */
class User extends \Illuminate\Database\Eloquent\Model
{}

This is an Eloquent model that the developer writes. @mixin \Eloquent part, might be added by the laravel-ide-helper package.

And \Eloquent is just a class alias to Illuminate\Database\Eloquent\Model

When we analyze User::find(1), MixinMethodsClassReflectionExtension runs and here $type is \Eloquent object type. And PHPStan loops thorough all extensions to find if the extension has that method and gets it if it finds. The problem is Illuminate\Database\Eloquent\Model is passed to the extensions as class reflection and not the App\User

Which is, expected :sweat_smile:

Now going through the code I realized maybe we can fix it in the extensions. Tell it to not work if the given class reflection is Illuminate\Database\Eloquent\Model Some of the extensions are already doing this. Like this, but found we don't do it for https://github.com/nunomaduro/larastan/blob/master/src/Methods/Pipes/Mixins.php

[canvural]

I just need to spend more time on this. Looks like it is Larastan's problem for now. Sorry to bother :sweat_smile:

[ondrejmirtes]

I'm not sure how https://github.com/nunomaduro/larastan/blob/master/src/Methods/Pipes/Mixins.php works because it doesn't look like a traditional PHPStan extension, but I'm quite certain that the EloquentModel exclusion now needs to happen on PHPStan's side because it now handles @mixin itself.

[canvural]

https://github.com/nunomaduro/larastan/commit/bd84cae67f8c947e17e563c757858c9db72ebeab this seems to fix the issue. And everything else works as expected.

[canvural]

Looks like it still can cause issues, like #565 @ondrejmirtes Is it possible to have a blacklist of classes that should not be inspected in @mixin ?

[ondrejmirtes]

@canvural Definitely, do you have time to contribute that to phpstan-src or shoud I do it?

[canvural]

@ondrejmirtes I can try to do it. Only MixinMethodsClassReflectionExtension and MixinPropertiesClassReflectionExtension should be changed I guess. And a new config parameter for users to fill in?

[ondrejmirtes]

Yes, I'd call it mixinExcludeClasses. It doesn't even have to be documented :)