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

Use argument types as parameter types for inline closures (#7798) #1628

Merged
merged 1 commit into from Aug 17, 2022

Conversation

leongersen
Copy link
Contributor

@leongersen leongersen commented Aug 16, 2022

Implements #7798. Thank you for your advise, it was very helpful.

The relevant code for this in NodeScopeResolver is this but you won't have to touch that for this feature request, it works in a general sense:

if ($passedToType instanceof UnionType) {
$passedToType = TypeCombinator::union(...array_filter(
$passedToType->getTypes(),
static fn (Type $type) => $type->isCallable()->yes(),
));
}
$callableParameters = null;
$acceptors = $passedToType->getCallableParametersAcceptors($scope);
if (count($acceptors) === 1) {
$callableParameters = $acceptors[0]->getParameters();
}

This unfortunately is not used for this case; $passedToType is only passed to processClosureNode when handling a closure as a function argument:

if ($arg->value instanceof Expr\Closure) {
$this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $context);
$result = $this->processClosureNode($arg->value, $scopeToPass, $nodeCallback, $context, $parameterType ?? null);
} elseif ($arg->value instanceof Expr\ArrowFunction) {

return $this->processClosureNode($expr, $scope, $nodeCallback, $context, null);
} elseif ($expr instanceof Expr\ClosureUse) {

Because the closure is never the argument (it is the name of the FuncCall), this case is slightly different than for array_map, as the Expr\Closure is never handled by ParametersAcceptorSelector::selectFromArgs (only its ClosureType is).

Please let me know if I misunderstood.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This is really nicce and almost perfect! The only thing missing is that you should also test and make sure it works for arrow functions :) Please add another gathered file in NodeScopeResolverTest, but only for PHP_VERSION_ID >= 70400.

@leongersen
Copy link
Contributor Author

Thanks for the feedback, much appreciated. I've now added support and tests for arrow functions as well.

I've duplicated the logic and added another NodeVisitor, as processArrowFunctionNode duplicates logic from processClosureNode. If you'd like it abstracted I can of course do that.

@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants