-
Couldn't load subscription status.
- Fork 544
feat: add dynamic parameter type extensions #4290
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
Conversation
49c7da8 to
58e51fb
Compare
58e51fb to
79de0a6
Compare
This pull request has been marked as ready for review.
I tested it by porting my previous implementation for Larastan to this one, and looks like it works fine! Will try to implement more use cases and see if I can find any bugs. Also will try to test it on real projects.But so far so good. Thanks for this!
I'll also try to review this PR (though Ondrej would do a better job 😄 )
79de0a6 to
7c605c1
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 looks solid, just a few questions as a start, will do deep review later 👍
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'm not sure what the e2e/parameter-type-extension directory is for. These directories are supposed to execute something in .github/workflows/e2e-tests.yml but this one does not. Either move this under e2e/bug-12585/ if it's used there, or delete it.
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.
Does not have to be optional, this is not under BC promise.
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.
Why this does have to be here actually at all? Why hasn't been the passedToType parameter kept? Why is this named "overriden type"? What is it overriding?
@calebdw @ondrejmirtes Sorry to bother you, but I wanted to ask how we can move this forward? I'm really interested in this feature.
No worries, just been busy with personal life, I'll try to get to the comments soon
Closes phpstan/phpstan#11707, closes phpstan/phpstan#12585
Supersedes #3828, supersedes #3131, supersedes #3823
Hello!
This adds generalized dynamic parameter type extensions and deprecates the parameter closure type extensions per phpstan/phpstan#11707 (comment).
This also fixes the
return.typeand theargument.typeerrors described in phpstan/phpstan#12585 when the parameter type is overridden via the extension.Note
The original type from the closure type extensions was being passed around as
$passedToTypeto the methods that needed it. However, with the new extensions the type must be passed around more, instead of adding parameter types to all the methods, I opted to add it to the ExpressionContext object that was already being passed around (given that this is context and whatnot) asoverriddenTypesince I wasn't quite sure what$passedToTypereally meant. Let me know if there's something you'd rather do differently.CC: @canvural, @Neol3108
Thanks!