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

Added support for first-class callables #7113

Merged
merged 3 commits into from Dec 10, 2021

Conversation

trowski
Copy link
Contributor

@trowski trowski commented Dec 9, 2021

Hopefully the approach here is appropriate. First-class callables are treated as a function call with a single placeholder argument, which necessitates special handling in the call analyzers to swap out the the return value to a closure and skip analyzing function arguments.

Closes #6412.

@@ -235,6 +243,45 @@ public static function analyze(
);

$config->eventDispatcher->dispatchAfterEveryFunctionCallAnalysis($event);

if ($is_first_class_callable) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you returning early here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about this, but it seemed the code below was generally analyzing a call to the given function, which isn't actually happening. Some guidance on what still needs to be called from the code below would be appreciated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you wouldn't go that far anyway, you have another if ($is_first_class_callable) just below that create the closure. I just found weird to return before that only when this condition if ($function_name instanceof PhpParser\Node\Name && $function_call_info->function_id) { is met and not otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path taken by if ($function_name instanceof PhpParser\Node\Name && $function_call_info->function_id) invokes FunctionCallReturnTypeFetcher::fetch, which returns a closure for first-class callables from named calls (e.g. strlen(...) or var_dump(...)). The if-statement below creates a closure for a first-class callable created from another expression, such as $closure(...) (see the test labelled FirstClassCallable:FromClosure).

Perhaps there is a more elegant way of doing this, but that's why the separation exists here.

@@ -235,6 +243,45 @@ public static function analyze(
);
Copy link
Collaborator

@orklah orklah Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the AfterEveryFunctionCallAnalysisEvent is the plugin hook. I feel like this should stay at the bottom of the Analyzer. Or maybe it should not be called at all when dealing with first class callable, not sure

EDIT: well, just saw this is currently not at the bottom of the function, I may be wrong here

@orklah
Copy link
Collaborator

orklah commented Dec 9, 2021

Wow, that's impressive work!

@weirdan weirdan self-requested a review December 9, 2021 21:52
@weirdan weirdan changed the title Add support for first-class callables Added support for first-class callables Dec 9, 2021
@weirdan
Copy link
Collaborator

weirdan commented Dec 9, 2021

Looks good to me in principle, but it seems it broke closure handling somewhere (hence all the failures).

Skipping checking args for first-class callables, but fixes handling of other methods.
@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Dec 10, 2021
@trowski
Copy link
Contributor Author

trowski commented Dec 10, 2021

@weirdan I reordered a couple of calls in ExistingAtomicMethodCallAnalyzer::analyze I didn't realize were dependent upon each other. After restoring the order and guarding a arg check call if the call being analyzed is a first-class callable, everything appears to be working as it should.

@orklah
Copy link
Collaborator

orklah commented Dec 10, 2021

Looks good! Let's see how this behave in the wild :D!

@orklah orklah merged commit 76bb8bc into vimeo:master Dec 10, 2021
@trowski trowski deleted the first-class-callables branch December 11, 2021 01:41
@orklah orklah mentioned this pull request Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closures for callables with myfunc(...)
3 participants