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

Can't add first class callable filter: No closure found #435

Closed
spaze opened this issue Aug 23, 2023 · 20 comments
Closed

Can't add first class callable filter: No closure found #435

spaze opened this issue Aug 23, 2023 · 20 comments

Comments

@spaze
Copy link
Contributor

spaze commented Aug 23, 2023

When adding a filter like:

$engine->addFilter('objectFilterFirstClassCallable', (new Filters())->objectFilter(...));

PHPStan returns "No closure found". I don't even know where to start so I'll just add a failing test (#436).

@MartinMystikJonas
Copy link
Collaborator

This looks like limitation of roave/betterreflection which is used to analyse closure created by first class callable. It tries to analyse given closure (param and return types) by parsing known location (file+line) of closure definition (it is part of closure "object") but looks only for normal closures not for first class callables.

Honestly I have no idea how to fix this.

@MartinMystikJonas
Copy link
Collaborator

I will check is there is any workaround for this used in PHPStan itself somewhere.

@MartinMystikJonas
Copy link
Collaborator

Reported issue to roave/betterreflection Roave/BetterReflection#1363

@ondrejmirtes
Copy link

Hey, please let me know the place in code where this problem occurs for you.

Personally I think if you did $scope->getType() instead of going through BetterReflection you should be able to avoid this problem.

@MartinMystikJonas
Copy link
Collaborator

MartinMystikJonas commented Nov 15, 2023

It happens here:

$args = $this->updateArgs(ReflectionFunction::createFromClosure($filter), $args);

We have Closure (real) created as first class callable which is extracted from Latte engine. And we need to know its signature to be able to preprocess code for analysis. Replace magic call by direct call to given method for which first class callable was created.

@ondrejmirtes
Copy link

we need to know its signature to be able to preprocess code for analysis

In which form do you need the signature? What about FunctionReflection/MethodReflection or ParametersAcceptor?

@MartinMystikJonas
Copy link
Collaborator

Well, class name + method name of first class callablectarget would be enough. Or we need types of all parameters and return type to create matching placeholder - MethodReflection should be enough.

Problem is we do not have scope from plave where closure is defined - we just get closure itself from latte engine.

@ondrejmirtes
Copy link

Why is this code in a node visitor and not done later where you Scope and other helpers available?

@MartinMystikJonas
Copy link
Collaborator

We compile template by latte, use these preprocessors to make it analysable and save it. Then we run analysis of this compiled template.

@ondrejmirtes
Copy link

Please show me these steps:

  1. Original latte template
  2. Latte template after transformation analysable by PHPStan (where the aforementioned filter is used and the AST is changed by this visitor)

@MartinMystikJonas
Copy link
Collaborator

MartinMystikJonas commented Nov 15, 2023

Original call to filter looks like this ($this->filters is jsut universal crate for filters, template have no idea what is in it at runtime)

($this->filters->upper)($title)

for static methods we transform it to:

\Nette\Utils\Strings::upper($title);

for non-static methods we use:

/** callable(string):string $__filter_upper */
// ...
$__filter_upper($title)

@ondrejmirtes
Copy link

Why don't you transform the filter call to:

((new Filters())->objectFilter(...))($whatIFilter)

That way you don't really need to care and transform the details of the call.

@ondrejmirtes
Copy link

Or maybe just do:

$__filter_upper = The\Callable::doFoo(...);

@ondrejmirtes
Copy link

Otherwise you could also do the job of collecting "this filter has this ParametersAcceptor signature" with a custom Collector (where you have all the PHPStan internals available) and then use that information later.

@MartinMystikJonas
Copy link
Collaborator

Problem is we have only Closure object we get from latte engine. It is created outside analysed files.

@MartinMystikJonas
Copy link
Collaborator

For your alternatives - in both cases we need to thow class name and method name Closure targets. But we cannot get it from Closure itslef.

@MartinMystikJonas
Copy link
Collaborator

MartinMystikJonas commented Nov 15, 2023

We also need to know if first param of target is FilterInfo because it is added to call magically by filter executor outside template

@ondrejmirtes
Copy link

I get it - so you don't see the AST from $engine->addFilter('objectFilterFirstClassCallable', (new Filters())->objectFilter(...));, you're trying to understand it and recreate from a Closure object.

The bug isn't in BetterReflection. You're calling ReflectionFunction::createFromClosure which is only for functions anyway, not methods.

Since you already have the Closure, I think you can just read the info manually by doing new \ReflectionFunction($closure) in the visitor, and read it your way. That should always work. And should be faster too.

@MartinMystikJonas
Copy link
Collaborator

Thanks for the tip, I will try it.

@MartinMystikJonas
Copy link
Collaborator

MartinMystikJonas commented Nov 15, 2023

Fixed by #450

@ondrejmirtes Thanks for good tip

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