-
Notifications
You must be signed in to change notification settings - Fork 545
Add support for the callable-string type #296
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 the callable-string type #296
Conversation
Hi, I don't understand this:
looks like there is no validation at all of the value itself
What's the point then? I'd definitely want the validation.
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'll fix the build of dev-master, you can then rebase.
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.
You can also add some testcases to TypeCombinatorTest::dataIntersect. It should work like this:
- Intersection of string and callable-string -> callable-string.
- Intersection of ConstantStringType and CallableStringType - depends on the value, can be normalized to ConstantStringType, it might not. Please test both cases.
Actually, I just checked what is_callable() does and it looks like it creates an intersection of CallableType and StringType: https://phpstan.org/r/4321ed59-36f6-4ee3-aad9-30da2a08c367
So we can scratch the new whole CallableStringType class and just work with the intersection...
So we can scratch the new whole CallableStringType class
I did some testing (I'm not practical enough of how PHPStan internals works, so I'm having an hard time contributing), but changing the TypeNodeResolver to return UnionType([new StringType(), new CallableType()]) instead of a whole new type produces issues elsewhere, for example PHPDoc tag @param for parameter $str with type (callable)|string is not subtype of native type string. I'm sure it's possible to handle this and that I'm doing something wrong, but wouldn't be more simple to handle the callable-string with its own type like the class-string implementation?
You need to make callable-string basically an alias of callable&string, not callable|string. Which means using IntersectionType, not UnionType.
Thank you.
Uuum, thank you for merging the PR but I still had to push the commit with the additional tests of the TypeCombinatorTest class... I will open a new PR to add them
PR is a PR, you're always risking I'm gonna merge it unless it's a draft :)
When you're at it, a new NodeScopeResolver::testFileAsserts data provider with callable-string in PHPDoc and assertType('callable&string', $param) would also be nice.
you're always risking I'm gonna merge it unless it's a draft
Lesson learned 😄
When you're at it, a new NodeScopeResolver::testFileAsserts data provider with
callable-stringin PHPDoc andassertType('callable&string', $param)would also be nice.
Sure, will do 👍
With this PR I would like to add support for the
callable-stringtype as it works in Psalm. I have deliberately decided not to check through the broker if the function exists as I think that it may be an issue in some cases due to false positives, but let me know if we want to go stricter. I did some tests with Psalm and it looks like there is no validation at all of the value itself. This fixes the specific case reported in phpstan/phpstan#3723, although the original problem with the missing fallback of PHPStan to the plain PHPDoc annotation remains