-
Notifications
You must be signed in to change notification settings - Fork 447
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
Add new *ParameterClosureTypeExtension #3089
Add new *ParameterClosureTypeExtension #3089
Conversation
b53f40c
to
187e074
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's solve the names as the last thing, now is time to solve the fundamentals :)
|
||
public function isFunctionSupported(FunctionReflection $functionReflection): bool; | ||
|
||
public function getTypeFromFunctionCall(FunctionReflection $functionReflection, FuncCall $functionCall, Scope $scope): ?Type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this method signature is not sufficient because the parameter where the closure is passed to is also significat, right? So we also need to know ParameterReflection
here. Also probably add it to the *Supported
method too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this method signature is not sufficient because the parameter where the closure is passed to is also significat, right?
hmm, not sure about that. We want to overwrite that parameter by supplying our own. And for my use case I did not need it. The test case I wrote here is basically the use case I needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say you have a method call with three parameters. Each parameter is a callable. You want this extension to override each callable parameter with its own type.
Your current design only works for methods with a single callable parameter, not multiple. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh yeah, now I understood. We want to be able to select which parameter we want to modify, right? By checking the parameter name or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ea44946
src/Analyser/NodeScopeResolver.php
Outdated
@@ -3928,6 +3930,49 @@ private function processClosureNode( | |||
?Type $passedToType, | |||
): ProcessClosureResult | |||
{ | |||
if ($stmt instanceof Node\Stmt\Expression && $stmt->expr instanceof Expr\CallLike) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to work for nested expressions to so to eliminate everything that isn't Node\Stmt\Expression
is not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to do the same thing for processArrowFunctionNode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to work for nested expressions to so to eliminate everything that isn't Node\Stmt\Expression is not ideal.
How to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my other suggestion (to move to processArgs) will take care of this too.
src/Analyser/NodeScopeResolver.php
Outdated
@@ -3928,6 +3930,49 @@ private function processClosureNode( | |||
?Type $passedToType, | |||
): ProcessClosureResult | |||
{ | |||
if ($stmt instanceof Node\Stmt\Expression && $stmt->expr instanceof Expr\CallLike) { | |||
if ($stmt->expr instanceof FuncCall && $stmt->expr->name instanceof Name && $this->reflectionProvider->hasFunction($stmt->expr->name, $scope)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could get rid of these if
statements if this logic was implemented in processArgs
(where processClosureNode
is called) it could be simplified a lot. You wouldn't have to look for the right reflection again, because it's available there as $calleeReflection
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I can try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it simplified a lot. But the test case with calling static method on a class string doesn't work. And maybe it doesn't need to work 🤷🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not surprised by that. $calleeReflecrion is probably null for that case now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it for now.
Also I forgot to mention, there was no easy way to access the expression in processArgs
, so had to refactor a bit so it accepts the expression (CallLike) now and extracts the args inside.
134fb44
to
3b37e60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine by me 👍
Let's think about the name:
- I think we can omit "changing". All extensions "change" something.
- I think the extension names should include "parameter".
So something like this:
- (Function|Method|StaticMethod)ParameterClosureTypeExtension (it communicates that it's about a function parameter and its type when you pass in closure)
Maybe we can also prefix the name with "Dynamic" but it might not be necessary.
Let me know your ideas :)
I can't find anything better than
Do you have any thoughts for this one also? |
Yeah, "dynamic" is not necessary. I'm fine with the current logic, if someone is going to need adjusting, we can change it in the future. I'm going to merge it after the rename, thanks :) |
I'm on my phone and remembered one more thing - make sure the extension interfaces are marked with |
Thank you! You can now require |
Yeah I already tried it with downloading the PHAR file from the PR 😄 And tweeted the results. Everything looks good so far. I'll do more testing. Thanks for the idea and your help! 😊 |
Alright 😊 Since there already has been a release today, I'm going to release 1.11.3 at some point next week (while in Berlin at IPC). |
Sure. No rush from my side. |
Hello 👋🏽
This PR adds new
ClosureTypeChangingExtension
extension that enables changing the closure type on the receiving side when necessary.There are couple of issues I want solve before merging it though so I'm making a draft first:
Dynamic
prefix in other extensions, not sure if we should use it here too.