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

FunctionStorage was not initialized with FunctionParamsProviderInterface #7450

Closed
klimick opened this issue Jan 20, 2022 · 27 comments
Closed

Comments

@klimick
Copy link
Contributor

klimick commented Jan 20, 2022

klimick@1d988ad

After #7417 was merged I tried to make a plugin for variadic pipe function (This may look tricky).

It would even work but FunctionStorage is not created for functions that are processed by FunctionParamsProviderInterface.
Without FunctionStorage will not created TemplateResult that matters for argument types inference.

After a "quick fix" I caught failing tests.
Can you answer the question why is FunctionStorage is not created?

@psalm-github-bot
Copy link

Hey @klimick, can you reproduce the issue on https://psalm.dev ?

@orklah
Copy link
Collaborator

orklah commented Jan 21, 2022

I don't understand what FunctionStorage you're refering to, can you give more details?

@klimick
Copy link
Contributor Author

klimick commented Jan 21, 2022

if (isset($function_call_info->function_storage->template_types)) {
$template_result = new TemplateResult($function_call_info->function_storage->template_types ?: [], []);
}
ArgumentsAnalyzer::analyze(
$statements_analyzer,
$stmt->getArgs(),
$function_call_info->function_params,
$function_call_info->function_id,
$function_call_info->allow_named_args,
$context,
$template_result
);

When FunctionCallAnalyzer analyzes call of regular function, the TemplateResult is created and passed to the ArgumentsAnalyzer.

Inside of the ArgumentsAnalyzer the TemplateResult needs for generics inference:

map([1, 2, 3], fn($i) => [$i]) // $i is 1|2|3, because it was infer by previous arg.

For functions processed via FunctionParamsProviderInterface functions storage will never created:

if ($codebase->functions->params_provider->has($function_call_info->function_id)) {
$function_call_info->function_params = $codebase->functions->params_provider->getFunctionParams(
$statements_analyzer,
$function_call_info->function_id,
$args,
null,
$code_location
);
}
if ($function_call_info->function_params === null) {
if (!$function_call_info->in_call_map || $function_call_info->is_stubbed) {
try {
$function_call_info->function_storage = $function_storage = $codebase_functions->getStorage(
$statements_analyzer,
strtolower($function_call_info->function_id)
);

If function storage will never created then TemplateResult will never be passed to ArguementsAnyalizer.
So the question. Why FunctionStorage doesn't creates for functions handled via FunctionParamsProviderInterface?

@orklah
Copy link
Collaborator

orklah commented Jan 21, 2022

Can you check if veewee's fix solves your issue?

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

Can you check if veewee's fix solves your issue?

It doesn't solve my issue.
I think that double running ArgumentsAnalyzer wasn't the best idea.
This causes callables without arg type hints will be inferred as mixed.

/** @psalm-trace $_ */
$_ = pipe(
    getList(), // list<string>
    map(function ($a) { // $a should be string but mixed
        return new Box(stringToInt($a));
    }),
    map(function ($box) { // $box should be Box<string> but mixed
        /** @psalm-trace $box */;
        return ['Val' => $box->a];
    }),
);

// ERROR: MixedArgument
// at /home/klimick/PhpstormProjects/other/psalm/pipe-example.php:100:36
// Argument 1 of stringToInt cannot be mixed, expecting string (see https://psalm.dev/030)
//         return new Box(stringToInt($a));
//
//
// ERROR: Trace
// at /home/klimick/PhpstormProjects/other/psalm/pipe-example.php:103:33
// $box: mixed (see https://psalm.dev/224)
//         /** @psalm-trace $box */;
//
//
// ERROR: MixedPropertyFetch
// at /home/klimick/PhpstormProjects/other/psalm/pipe-example.php:104:26
// Cannot fetch property on mixed var $box (see https://psalm.dev/034)
//         return ['Val' => $box->a];

Even if I specify type hints this affects inference of templates:

/** @psalm-trace $_ */
$_ = pipe(
    getList(),
    map(function (string $a) {
        return new Box(stringToInt($a));
    }),
    map(function (Box $box) {  // $box should be Box<string> but just Box
        /** @psalm-trace $box */;
        return ['Val' => $box->a];
    }),
);

// ERROR: Trace
// at /home/klimick/PhpstormProjects/psalm/pipe-example.php:103:33
// $box: Box (see https://psalm.dev/224)
//         /** @psalm-trace $box */;

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

But if I comment re-running of the ArgumentsAnalyzer my plugin works as I expect.

ArgumentsAnalyzer::analyze(
$statements_analyzer,
$stmt->getArgs(),
$function_call_info->function_params,
$function_call_info->function_id,
$function_call_info->allow_named_args,
$context
);

@veewee Have you checked how your pipe infers template types?

For example:

/**
 * @template A
 * @template B
 *
 * @param callable(A): B $_ab
 * @return callable(list<A>): list<B>
 */
function map(callable $_ab): callable
{
    throw new RuntimeException('???');
}

/**
 * @return list<int>
 */
function getList(): array
{
    return [];
}

/**
 * @template T
 * @psalm-immutable
 */
final class Box
{
    /** @var T */
    public $prop;

    /**
     * @param T $a
     */
    public function __construct($a)
    {
        $this->prop = $a;
    }
}


// Inferred type is list<Box<mixed>>
// Without double run ArgumentsAnalyzer is list<Box<int>>
$result = pipe(
    getList(),
    map(function (int $a) {
        return new Box($a + 1);
    }),
    map(function (Box $a) {
        return new Box($a->prop + 1);
    })
);

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

It seems first running of the ArgumentsAnalyzer analyze anonymous functions and second running will not overwrite already analyzed information.

First running also may raise issues that second doesn't raise.

@veewee
Copy link
Contributor

veewee commented Jan 22, 2022

I've tested the example above with my plugin (with closures instead of callables though)

Test:
https://github.com/veewee/pipe-plugin/blob/main/tests/templated.php

Result:

ERROR: Trace - tests/templated.php:54:26 - $pipe: Closure(list<int>):list<Box<mixed>> (see https://psalm.dev/224)
/** @psalm-trace $pipe */;


ERROR: Trace - tests/templated.php:55:28 - $result: list<Box<mixed>> (see https://psalm.dev/224)
/** @psalm-trace $result */;

The templated value of the item inside Box gets lost and becomes mixed.
If I remove the added analyse function, I get the same result.
So I guess this was something that was not possible yet before as well?

Can you share me your function + plugin maybe, so that I can take a look in there?

I do notice that I did not add any template map to the arguments analyzer here:

https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php#L565-L572

Adding it with $statements_analyzer->getTemplateTypeMap() does not change the outcome though.
Not sure it should be added.

This additional analyzer call is only ran for the pipe function, since the other functions dont have any plugin. So I don't think this is a big issue. It only analuzes twice for the pipe function itself: The first time with the guessed types, the second time with the improved types.
This means it might be something in your plugin that makes the templates work?

Not sure if it is related, but callables don't play well with generic types either.
See #7244

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

P.S.S Before #7394 my plugin doesn't work because FunctionStorage was not create for function processed via FunctionParamsProviderInterface (problem described above)

@veewee
Copy link
Contributor

veewee commented Jan 22, 2022

So after #7394, the storage you require is available now?

The big difference in our versions is that I tried parsing the argument types and you try to link a predefined set of template parameters instead.

So the issue I have, is that those argument types were not available - and now are.
The issue you had, was that the function storage for the template resolution was not available?

But if you comment out the analyzer, it won't be available again?

I blindly expected the types to have the correctly resolved templates after argument analysis. That way you don't need those nested template resolutions and you could use the argument types to determine the different stages. My best guess it that the reason for this, is that closures and templates don't work well together (#7244)

If you skip the analysis, as you did, the argument type information is not available in front of triggering the plugin.

I am not sure what the best direction would be here for both our approaches?

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

But if you comment out the analyzer, it won't be available again?

After #7394. FunctionStorage is available and TemplateResult too.
Even If I comments ArgumentsAnalyzer:

https://github.com/klimick/psalm/blob/d814bdf011dd08427206af5cc8204133cc5bc0b7/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php#L564-L574

The big difference in our versions is that I tried parsing the argument types and you try to link a predefined set of template parameters instead.

Pipe arguments should be analyzed sequentially:

callable(A): B
callable(B): C

The true B type we can knows after we analyze first callable.
For this reason (you try analyse all args) example with Box<A> is Box<mixed> in your case.

As I understand my way called Contextual type inference.
It infers types using previous inferred types.
The first time I saw this in typescript: https://www.typescripttutorial.net/typescript-tutorial/typescript-type-inference/
(Sorry can't leave hyperlink. Find by "Contextual typing")

I'm not sure that we can do it somehow else.
By this reason I've used predefined templates + FunctionParamsProviderInterface hook.
This trick allows to mimic variadic (in my example limited to 12 args) arguments with templates.

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

Your changes from #7394 was merged to master. It's not too late to remove pre-analysis in favor to the plugin with contextual inference. What do think @veewee ?

@veewee
Copy link
Contributor

veewee commented Jan 22, 2022

The way I see it, there are 2 issues:

  1. The closures/callables don't return the resolved templates
  2. Your added higher order templating logic is not triggered when analyzing twice

Problem 1:

Since the context is known within the pipe due to the order of functions, it could be possible to resolve the templates correctly by only reading the arguments it has:

pipe(
    map(function (int $a) {
        return new Box($a + 1);
    }),
    map(function (Box $a) {
        return new Box($a->prop + 1);
    })    
)

In this case:

  • Param 1 : It should know based on function 1 that input is list<int> and output is list<Box<int>>. If the callables resolved tempaltes correctly, you should know this in the parent 'pipe' function right?
  • Param 2 : It should know based on function 1 that input is list<Box<int>> and output is list<Box<int>>. If the callables resolved tempaltes correctly, you should know this in the parent 'pipe' function right?

So if the closures would resolve the correctly resolved template types, that fixes the contextual issue.

(That's basically how my version of the plugin deals with this issue: https://github.com/veewee/pipe-plugin/blob/6b47861e85060056b0c829f49cabbbb95673ebc9/src/Psalm/PipeArgumentsProvider.php#L43-L69)

Problem 2: If we could fix problem 1, we might not need to fix problem 2. Because that would mean types are resolved correctly as well.

Adding 12 templates feels a bit clumsey - similar like the typescript overloading approaches.

About reverting the code: Not sure ... The additional analysis has a different purpose.
I am also planning to create a plugin for a curry function, in which I do need the argument types in advance to calculate the types you can pass to both the curry function and the resulting closure.
That won't be possible anymore in that case.

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

Or if type pre-analysis for args in the FunctionParamsProviderInterface is useful it should be optional:

final class SomeFunctionParamsProvider implements FunctionParamsProviderInterface
{
    // new method that determine pre-analysis running
    public static function preAnalyseArgTypes(): bool
    {
        return false;
    }

    /**
     * @return array<lowercase-string>
     */
    public static function getFunctionIds(): array
    {
        // ...
    }

    /**
     * @return ?array<int, FunctionLikeParameter>
     */
    public static function getFunctionParams(FunctionParamsProviderEvent $event): ?array
    {
        // ...
    }
}

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

Adding 12 templates feels a bit clumsey - similar like the typescript overloading approaches.

You can add 24 templates. 48 templates. 94 templates. In real life, hardly anyone will make a pipeline of 12 calls.
No matter how many templates you add, it shouldn't be considered clumsy. It will be in vendor code and invisible in userland code.

P.S. typescript can handle curry, compose, pipe and other variadic arg functions. For example curry: https://github.com/type-challenges/type-challenges/issues?q=label%3A462+label%3Aanswer

I am also planning to create a plugin for a curry function

You can use FunctionReturnProviderInterface hook. In this hook all arg types are available and you can rewrite return type. You can throws any issues using available type information.

@veewee
Copy link
Contributor

veewee commented Jan 22, 2022

Or if type pre-analysis for args in the FunctionParamsProviderInterface is useful it should be optional:

Adding a method to toggle how it works doesn't seem like a good idea to me. Since problem 1 will still be there.
Something is not running good ATM for sure. But covering it up with this method I wouldn't personnaly do :)

You can add 24 patterns. 48 templates. 94 templates. In real life, hardly anyone will make a pipeline of 12 calls.
No matter how many templates you add, it shouldn't be considered clumsy. It will be in vendor code and invisible in userland code.

True ... stiil clumsy :)

You can use FunctionReturnProviderInterface hook. In this hook all arg types are available and you can rewrite return type. You can throws any issues using available type information.

True, but it is only for determing the return type. Whereas you should use FunctionParamsProviderInterface to check the initially provided params. E.g.:

curry(array_map(...), 'wrong');
                       ^^^^^^

I'm logging off for the day.

Maybe @orklah can moderate from his point of view?

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2022

Honestly, you're both way above me on those concepts... I can't give advices here unfortunately

@veewee
Copy link
Contributor

veewee commented Jan 22, 2022

Ok, I'm gonna look into problem 1 next week: the templated callable does not get resolved issue.

Since it might end up being a workable fix for @klimick as well.

@klimick
Copy link
Contributor Author

klimick commented Jan 22, 2022

keep it in mind @veewee

  1. Recently added high order func arg inference can work only with functions that have the fixed signatures.
    i.e. there is map:
/**
 * @template A
 * @template B
 *
 * @param callable(A): B $_ab
 * @return callable(list<A>): list<B>
 */
function map(callable $_ab): callable
{
    throw new RuntimeException('???');
}

We know that map have one arg of type callable(A): B and return type callable(list<A>): list<B>.
It can be easily use for contextual inference.

  1. You're going to write a plugin for the curry.
    Plugin is meant that function will have the dynamic signature.
    It means high order func arg inference won't work correctly for this case.
    We don't know how many templates we have, how many args and return type.
    When we will know all type information from a plugin it will be late.
    Arg analysis will have been finished after HOF arg inference logic.
    I have no idea how to improve HOF arg inference for dynamic signatures.

P.S "work correctly" meant:

  1. inference of functions without explicit type hint fn($i) => $i
  2. inference of arg template fn(Box $i) => $i where Box have one template T

@veewee
Copy link
Contributor

veewee commented Jan 23, 2022

@klimick If I understand corectly, in Following example

curry(
    callable(T1, T2, T3): R,
    T1,
    T2
) : callable(T3) : R

The higher order logic won't be able to resolve T1 and T2 (and possibly R) correctly - but can resolve T3?

I am on board with your pipe approach at this moment.
And also think that the first argument analysis results in false positives - like it does here.
(Because the first time, it runs the analyzer with incorrect information, resulting in types not properly being detected and issue reports being generated right?)
Especially since the parsed argument types don't know about Box being generic etc in the example above.

However, how would je be able to detect the type of the first argument from a plugin POV in this case?
Given that the event args contain the AST expressions, and it can be pretty much anything: variable, first class callable, invokable class, ... There is no way of grabing the resulting type information, so you can't even know the argumens and return type of the original function in the curry example - without parsing everything again manually (which surely can't be the prefered way).

Maybe if we can find answers to the curry issue, we can find an agreement that has every case covered?
Is this something you looking into as well?

@orklah
Copy link
Collaborator

orklah commented Jan 23, 2022

Just chiming in, if the plugin hook is not positioned at the right time, the inference behind curry and pipe feature could be included in Core I think. You'll probably have your hands less tied like that

@veewee
Copy link
Contributor

veewee commented Jan 23, 2022

Ho would you see that @orklah ?

@orklah
Copy link
Collaborator

orklah commented Jan 23, 2022

Not sure, but doesn't the principles applied independant from any third-party tool? (I may be completely wrong here, feel free to ignore me 😄 )

@klimick
Copy link
Contributor Author

klimick commented Jan 23, 2022

Simplified example showing a flaw of argument pre-analysing:
Contextual type inference (works): https://psalm.dev/r/0b43a7300b
With pre-analyzed args (cannot works): https://psalm.dev/r/bca7840b9d
With pre-analyzed (works because closure typed explicitly): https://psalm.dev/r/83994466a4
Example where pre-analysis never can infer Box<T> correctly (we must type it explicitly): https://psalm.dev/r/f4f9502598

No idea how args pre-analysis can help.
i.e. it my vote against double arguments analysis.

But I'll think about how you can make both pipe and curry work.
Even without "clumsy" word))

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/0b43a7300b
<?php

/**
 * @template A
 * @template B
 *
 * @param list<A> $list
 * @param callable(A): B $ab
 * @return list<B>
 */
function map(array $list, callable $ab): array
{
	throw new RuntimeException('???');
}

/**
 * @return list<int>
 */
function getList(): array
{
	return [];
}

$result = map(getList(), fn($i) => [$i]);
/** @psalm-trace $result */;
Psalm output (using commit e0d3c3f):

INFO: Trace - 25:28 - $result: list<array{int}>
https://psalm.dev/r/bca7840b9d
<?php

/**
 * @template A
 * @template B
 *
 * @param list<A> $list
 * @param callable(A): B $ab
 * @return list<B>
 */
function map(array $list, callable $ab): array
{
	throw new RuntimeException('???');
}

/**
 * @return list<int>
 */
function getList(): array
{
	return [];
}

$ab = fn($i) => [$i];
$result = map(getList(), $ab);
/** @psalm-trace $result */;
Psalm output (using commit e0d3c3f):

INFO: MissingClosureParamType - 24:10 - Parameter $i has no provided type

INFO: MissingClosureReturnType - 24:7 - Closure does not have a return type, expecting array{mixed}

INFO: Trace - 26:28 - $result: list<array{mixed}>
https://psalm.dev/r/83994466a4
<?php

/**
 * @template A
 * @template B
 *
 * @param list<A> $list
 * @param callable(A): B $ab
 * @return list<B>
 */
function map(array $list, callable $ab): array
{
	throw new RuntimeException('???');
}

/**
 * @return list<int>
 */
function getList(): array
{
	return [];
}

$ab =
    /**
     * @param int $i
     * @return array{int}
     */
    fn($i) => [$i];

$result = map(getList(), $ab);
/** @psalm-trace $result */;
Psalm output (using commit e0d3c3f):

INFO: Trace - 32:28 - $result: list<array{int}>
https://psalm.dev/r/f4f9502598
<?php

/**
 * @template A
 * @template B
 *
 * @param list<A> $list
 * @param callable(A): B $ab
 * @return list<B>
 */
function map(array $list, callable $ab): array
{
	throw new RuntimeException('???');
}

/**
 * @template T
 */
final class Box
{
    /**
     * @param T $value
     */
	public function __construct(public mixed $value) { }
}

/**
 * @return list<Box<int>>
 */
function getList(): array
{
	return [];
}

$ab =
    /**
     * @param Box<int> $i
     * @return array{Box<int>}
     */
    fn(Box $i) => [$i];

$result = map(getList(), $ab);
/** @psalm-trace $result */;
Psalm output (using commit e0d3c3f):

INFO: Trace - 43:28 - $result: list<array{Box<int>}>

@veewee
Copy link
Contributor

veewee commented Jan 23, 2022

No idea how args pre-analysis can help. i.e. it my vote against double arguments analysis.

Indeed. Figured it out this morning as well, hence my previous comment: it is not a good solution.
So let's scrap that pre-analysis for now.

But I'll think about how you can make both pipe and curry work. Even without "clumsy" word))

Don't get me wrong here: I love the template based solution.
My comment was about the 12 hard coded and therefore limiting template params. Would be nice if there is a way in which you can declare dynamic templates on the function. (Haven't looked into that yet)

@klimick klimick closed this as completed Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants