Incorrect handling of bind with params

[vadimonus]

Describe the bug
Psalm can die when Laravel application has binds with parameters.

Impacted Versions
Any laravel version, psalm-plugin-laravel 1.x, 2.x

Additional context
If application has service provider with bind with params

$this->app->bind(SomeInterface::class, function ($app, $params) {
    $someParam = $params['some_param'];
    return $something_that_depends_on_param;
});

and call

$var = app(SomeInterface::class, ['some_param' => 'some_value'])

it will lead to ErrorException Undefined index: some_param, that will kill psalm.

That's because \Psalm\LaravelPlugin\Util\ContainerResolver::resolveFromApplicationContainer do not takes into account, that make method has second param.

Fix
Currently resolveFromApplicationContainer catches only BindingResolutionException | ReflectionException, it should catch Throwable, to be protected from all possible errors.

When Laravel resolves bind with params, it does not cache result, because it can depend on params, so every call to make with params can produce different results. As far it's not possible to simply pass params to container to get real type, resolvePsalmTypeFromApplicationContainerViaArgs should return null, when count($call_args) !== 1.

[alies-dev]

Hey @vadimonus

Thank you for the report!

Currently resolveFromApplicationContainer catches only BindingResolutionException | ReflectionException, it should catch Throwable, to be protected from all possible errors.

I'm not sure what do you mean. I checked it, and it's fixed some time ago:

#228

And it's fixed on 2.x branch (namely v2.0.1). e60d2b7

Or do I misunderstand something?

[vadimonus]

@alies-dev , thnk you for your reply.

#228 is only the sipliest part of solution, and does not works for 1.x branch. I do no think it's too hard to backport it.

The main problem is that you should not use resolveFromApplicationContainer at all for types resolution.
Service provider can have complex logic of resolving type. Cases are:

• contextual binding. with contextual binding you can resolve to different types, depending of caller. currently plugin does not pass context info when resolving, so we cannot be sure that it resolves to right class. Then it caches first responce. So it will lead to incorrect type resolution in all calles (may be only except first).
• binding with params (as described in this issue). When laravel uses binding without params, it can cache binding result in contatiner. In this case caching result in plugin can be quite ok. When binding with params, laravel never caches result, and can produce new type every time. So caching first resolution in plugin is bad idea. And calling container from plugin without params, when app() was called with params, is bad idea too.
• binding depending on configs. Service provider can resolve to different classes in local and in production depending on settigns. You'll run psalm in local environment, thinking it will help you to avoid errors in production, but it's not so, because local psalm will use local resolution (that may be mocks), that can be far away from production.

So type resolution with resolveFromApplicationContainer gives more problems with false resolutions, than not resolving at all. The only true logic that we have about resolution with app() function:

• if you resolve interface, you will receive instance of that interface.
• if you resolve class, you will receive that class or its' child.
  This is the same as Mockery plugin does, it does not require container calls.

Also, if you receive not class string, you can try resolve it with \Illuminate\Foundation\Application::registerCoreContainerAliases(). But this is already done good enough by barryvdh/laravel-ide-helper when generating meta, so we do not need to do this second time in plugin.

So i think:

• static analysis is about analysiss without calling code. So callign app container for resolution is a pattern violation. This narrows the range of possible types and leads to incorrect conclusions.
• plugin need some config for disabling resolveFromApplicationContainer. For backward compatibility it should be switched on in current version, with switching off in future major release.
• in future major release plugin resolveFromApplicationContainer method should be marked as deprecated, beacuse it can give too much false types resolution, with recommendation not to use it.

[alies-dev]

Hey @vadimonus
Thank you for your detailed reply

Firstly, on 1.x it's also "fixed": https://github.com/psalm/psalm-plugin-laravel/blob/1.x/src/Util/ContainerResolver.php (incl. version 1.6.2 and later: https://github.com/psalm/psalm-plugin-laravel/blob/v1.6.2/src/Util/ContainerResolver.php)

Secondly, Laravel has a lot of magic, and the way Larastan and this plugin work to find more type info - violate static analysis principle and run some code. I do agree, ideally we should have parameters to enable dangerous actions and when more resources (more maintainers) for this plugin -- we can do it. But so far resources are limited, and it's better to focus on the main, it's listed on the README.md. But any contributions to the plugin are always welcome 👍

[vadimonus]

@alies-dev ,
I'm trying to figure out how it happened that the problem was already fixed, but I created the task.

Since version 1.5 of plugin, composer.json requires "illuminate/*": "^6.0 || ^8.0", skipping Laravel 7. And i encountered this problem when running plugin on laravel 7, and plugin 1.4.x, because i cannot use higher plugin versions with it.

Do you remember, why 7 version was skipped in composer.json?
Can we backport this patch to 1.4 too?

[alies-dev]

@vadimonus

I was not a contributor to this package at the moment when maintainers dropped L7 support. I see it's done within this commit: d475892

The commit body is:

chore: drop support for Laravel 7
Laravel 7 has reached its end-of-life.

I created a new branch 1.4.x and backported two critical fixes. Can you please test this branch over your project and then share your feedback here? If the fix works fine, I'll release it as a new 1.4.10 version.

[vadimonus]

Yes, but it will take couple of days

[alies-dev]

@vadimonus
thank you, I'll wait, it's not urgent. Thank you!

[alies-dev]

Hey @vadimonus
Do you have any news?

[vadimonus]

@alies-dev , sorry, didn't have enough free time.
composer require psalm/plugin-laravel:^1.4.10 gived me dev version Downloading psalm/plugin-laravel (1.4.x-dev 018c576).
It finished without error, where 1.4.9 previously crashed

[alies-dev]

@vadimonus
thank you! Then, I'm closing the issue. Thank you for reporting!

[vadimonus]

@alies-dev , 1.4.10 does not have right requirements in composer.json, it skips laravel 7. Will you release 1.4.11 with requires same as v1.4.9?

[alies-dev]

hey @vadimonus

Done, I just released it as 1.4.11. What I released as 1.4.10 was a wrong branch. But not it should be fine. The changelog is very clear: v1.4.9...v1.4.11