-
Notifications
You must be signed in to change notification settings - Fork 545
Detect function variadic-ness within top-level function_exists() #3370
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
823cd05 to
cd11bb2
Compare
This pull request has been marked as ready for review.
cd11bb2 to
8836200
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.
I feel like this code is too complicated. We could simply traverse through everything similarly to how we do here:
phpstan-src/src/Type/FileTypeMapper.php
Lines 758 to 772 in ad32861
What's more important to also solve here is to answer the question: What if we find the same function multiple times? Is it enough if one of them is variadic, or all of them need to be variadic?
Also we might find multiple functions and some of them might be variadic natively (...$params) and some only through func_get_args(). The code needs to account for that.
And last but not least, do not forget to bump the version in the cache key here :)
8836200 to
1c6a8f4
Compare
I feel like this code is too complicated
from the pre-existing code I got the impression its on purpose filtering only the smallest possible AST, therefore I went with this ugly if-else analysis. adjusted it now to analyze the whole AST.
Also we might find multiple functions and some of them might be variadic natively (
...$params) and some only throughfunc_get_args(). The code needs to account for that.
I added tests for the cases, I think it works as expected now.
And last but not least, do not forget to bump the version in the cache key here :)
great catch
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 I'm missing looking at Param::$variadic here. Now it looks like it treats function foo($bar) { } and function foo(...$bar) { } the same and passes the function as variadic in either case. I think it should be more strict.
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.
native variadic is already checked in the outer scope.
for native variadic we don't call into callsFuncGetArgs at all
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 can have a situation when $isNativelyVariadic = $this->reflection->isVariadic(); returns false and still one of the function declarations in the file might have function foo(...$bar) .
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.
Please reproduce this situation and fix it, thanks.
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 was not able to reproduce it initially because of the caching involved.
After temporarily using different cache keys locally the case was showing up :-)
ccc362b to
06f044c
Compare
06f044c to
feb7615
Compare
Alright, this works for me :) Thank you!
Uh oh!
There was an error while loading. Please reload this page.
closes phpstan/phpstan#11559
closes phpstan/phpstan#4753