-
Notifications
You must be signed in to change notification settings - Fork 461
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 support for generic CallableType. #2938
Conversation
You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x. |
@@ -7,6 +7,11 @@ | |||
class TemplateTypeScope | |||
{ | |||
|
|||
public static function createWithAnonymousFunction(): self |
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.
not sure at all about this but wasn't sure of an alternative
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'd do the same thing here, if it doesn't work you have to debug why :)
The transformation happens in CallableType::traverse()
. So basically it should be automatic.
You should also pass TemplateTypeMap into ClosureType.
As an exercise you should make the CallableType constructor parameters required, and make sure they're passed in everywhere it's relevant. Most importantly in all new self(...)
instances inside the class.
What gives me a little pause is $this->resolvedTemplateTypeMap
. I'm not sure what should happen there. Maybe we can just continue using createEmpty
there.
The main workflow that influences generics is through ParametersAcceptorSelector, GenericParametersAcceptorResolver specifically. So the code that resolves these types will always go through there, so you can debug what's going on. |
I realized that ClosureType already supports generics (https://phpstan.org/r/eebaf81b-333c-4e96-97fb-801436690236) so if something does not work for CallableType you can debug what's going on in ClosureType and get inspired by that :) |
abaea93
to
f5d6c34
Compare
thanks for your wisdom there @ondrejmirtes it guided me to where to begin looking, and great tip about |
This pull request has been marked as ready for review. |
f5d6c34
to
e663838
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.
We also need to catch the things MethodTagValueNode catches for these types in these places:
- Method parameters
- Method
@param-out
- Method return types
- Function parameters
- Function
@param-out
- Function return types
- Property types
- Inline
@var
s - All the class-level PHPDocs like
@method
and@property
You can write the common logic in a rule helper that you'll inject into these rules:
* IncompatiblePhpDocTypeRule (covers everything for functions and methods)
- IncompatiblePropertyPhpDocTypeRule
- IncompatibleSelfOutTypeRule
- InvalidPhpDocVarTagTypeRule
- MethodTagTemplateTypeRule (maybe rename this one)
- a new rule for property tags - nothing is currently checked about them!
This could also be an opportunity to check more about @method
parameters and return types, like classes that do not exist (this is typically done in ExistingClassesIn*
rules), unresolvable types etc.
Do you want to do this here or in a follow-up PR? :) Thanks.
Also please inspect the failures in https://github.com/phpstan/phpstan-src/actions/runs/8031291020, they look very interesting! |
Oh sorry, most of them are fault of a previous PR :) |
yeah i think the rector ones are caused by this pr so i'll look into them when i get more time; working on this has cooked my brain so i need a break! |
e663838
to
3ba9e29
Compare
3ba9e29
to
b31a746
Compare
The rector failures are caused by the action installing https://github.com/rectorphp/rector-src/blob/main/composer.json#L26 is it worth me handling this by adding a |
*/ | ||
function testNestedClosures(Closure $closure, string $str, int $int): void | ||
{ | ||
assertType('Closure<TRetFirst of mixed>(TRetFirst): (Closure<TRetSecond of mixed>(TRetSecond $valtwo): (TRetFirst|TRetSecond))', $closure); |
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.
the return type Closure
does not have the parameter names removed as the removal is done in describe()
not toPhpDocNode()
, and describe is only called for the top-level. not sure if this is intended or not so thought i'd mention it
i can investigate adding these in a follow up pr if you'd like, though i'm a bit overwhelmed by the list 😅 |
This pull request has been marked as ready for review. |
About #2938 (review) - yeah, sure, this can be addressed in multiple small follow-up PRs :) I'd really appreciate if you continued working on this :) (I have even more similar stuff for you if you want :)) |
Thank you! |
Trying to follow up the
phpdoc-parser
pr with implementing generic callables and closures. About as far as i can get tonight so putting this up to get some feedback; not sure if this is the right direction at all, just trying to apply what i learned from the other prs.Right now the test is outputting:
which seems somewhat familiar to the
$nameScope
thing you mentioned in the generic@method
pr, but i am calling->withTemplateTypeMap()
like you suggested over there.One major thing that i can't seem to find is where the template types are replaced with the actual real types; as i feel like with the addition of
$resolvedTemplateTypeMap
onCallableType
i'm going to have to do that somewhere to then pass in the resolved type map, but where i should do that escapes me.