This pr still contains the preanalysed version of the function params hook. Is it the idea to keep it there since we can now use this new hook?

I would suggest to remove old hook in favor new one.
In my opinion old hook is unuseful against new hook. New hook can do the same and more.

Guys, it's amazing to read your comments and the proactivity ! Even if I don't understand anything, I'm pretty sure you're going to make something great that I really want to use ASAP !

I would suggest to remove old hook in favor new one.

Would old plugins be easy to migrate? If we do it, it's the right moment, we just removed the old plugins interfaces in Psalm 5, it's possible a lot of plugins didn't migrate already

I would suggest to remove old hook in favor new one.

Would old plugins be easy to migrate? If we do it, it's the right moment, we just removed the old plugins interfaces in Psalm 5, it's possible a lot of plugins didn't migrate already

It depends which ones are gonna be removed.
The function return type hooks are being used quite often in our plugin and still make sense. You don't always want to build up the full function storage.

The one for function parameters I don't think is being used commonly. Because you can't really do that much with it at this moment. But I can only speak for my own plugins :)

Current hooks are compatible with the once for methods as well. There we don't have this new hook.

It depends which ones are gonna be removed.

I suggest to remove FunctionParamsProviderInterface and MethodParamsProviderInterface.
As I understand: It provides dynamic signature using raw arg nodes.
With (Function|Method)ParamsProviderInterface + (Function|Method)ReturnTypeProviderInterface we can organize function that return type depends from input args. But it can't provide templates. It can't cover hard to infer functions (native array_map, pipe, curry and other).

On the other side, new hook is very low level. Writing plugin for custom_array_map was not easy as it might seem.
For functions without template params we can leave FunctionParamsProviderInterface and MethodParamsProviderInterface.

It depends which ones are gonna be removed.

I suggest to remove FunctionParamsProviderInterface and MethodParamsProviderInterface. As I understand: It provides dynamic signature using raw arg nodes. With (Function|Method)ParamsProviderInterface + (Function|Method)ReturnTypeProviderInterface we can organize function that return type depends from input args. But it can't provide templates. It can't cover hard to infer functions (native array_map, pipe, curry and other).

On the other side, new hook is very low level. Writing plugin for custom_array_map was not easy as it might seem. For functions without template params we can leave FunctionParamsProviderInterface and MethodParamsProviderInterface.

It might make sense to keep them, for making the upgrade traject easier for maintainers as well.
It gives integrations the flexibility of either taking the more simple plugin approach or have full control with the dynamic one.

Ok then let's keep other hooks for now.

Before I dive too deep in the code, I have a few questions:

• does it solves the issue that was created by calling ArgumentsAnalyzer twice?
• can you add some documentation for the new plugin hook and specify when to use the old ones and the new ones
• Is ArrayMapStorageProvider just for test purposes? Is it normal it targets custom_array_map only?

• does it solves the issue that was created by calling ArgumentsAnalyzer twice?

I've removed double args analysis just now. This can lead to many problems. If you have not yet understood what the problems are, I can try to describe them.

• can you add some documentation for the new plugin hook and specify when to use the old ones and the new ones

Of course I can. When PR will ready.
I think we should have hook like this but for class methods. i.e. MethodDynamicStorageProvider. What do you think?

• Is ArrayMapStorageProvider just for test purposes? Is it normal it targets custom_array_map only?

Currently just for test. I planned to use it as example for documentation.
If you count it too complicated example I will try to write easy one.

I removed internal classes usage and tried to make a public api instead.

• ArgTypeInferer: can use we need to infer argument type
• DynamicTemplateProvider: template creator. Not all plugin users know about $defining_class and what value it should be. And hook can works with many function that enumerated in getFunctionIds. It's a little bit difficult pass the right value to 'TTemplateParam'. So creator can create well formed TTemplateParam.
• DynamicFunctionStorage: I've make it because in the FunctionStorage there are useless properties for hook (at my view point)

What do you think? @veewee @orklah

Before I dive too deep in the code, I have a few questions [...]

Also, how does it interact with cache?

Also, how does it interact with cache?

I didn't quite understand what you mean.

Dynamically created storage saves only in the FunctionDynamicStorageProvider in $dynamic_storages.
Because there is cases where FunctionDynamicStorageProvider should be called twice for the same expression.
At the second call FunctionDynamicStorageProvider returns already computed storage.

As I know Psalm treats each closure as unique and create for them individual storage. Does Psalm cache it?
Dynamically analyzed function has next id:

$dynamic_storage_id = strtolower($statements_analyzer->getFilePath())
            . ':' . $stmt->getLine()
            . ':' . (int)$stmt->getAttribute('startFilePos')
            . ':dynamic-storage'
            . ':-:' . strtolower($function_id);

Id from FunctionDynamicStorageProvider has similar to closure id.

These two calls will be analyzed individually and each call will have their id:

custom_array_map(
   fn($i) => $i + 1,
   [1, 2, 3]
);
custom_array_map(
   fn($i) => (string) $i,
   fn($i) => $i + 1,
   ['1', '2', '3']
);

There is sense to cache it? And if there is. How?

I didn't quite understand what you mean.

I meant the file cache Psalm uses to speed up things on subsequent runs. Normally FunctionStorage instances are cached as part of FileStorage. Having looked at the proposed changes once again, I think the answer is 'it doesn't interact with cache in any way, as it's used during call analysis'. Also, it appears it would shadow the FunctionStorage Psalm may generate during the scanning phase. It would be nice to have this documented in the source code itself - say, in DynamicFunctionStorageProvider docblock.

Also, would you mind renaming all occurrences of function dynamic storage to dynamic function storage? I believe it would read better that way.

Thanks!

Excellent work, thanks !

Great job!

Will be released in v5?

Yeah, this has been targeted on master branch, so with V5 release