-
Notifications
You must be signed in to change notification settings - Fork 482
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
Fix ConstantArrayType::isCallable #3717
base: 2.1.x
Are you sure you want to change the base?
Conversation
return TrinaryLogic::createYes()->and(...$results); | ||
$isCallable = TrinaryLogic::createYes()->and(...$results); | ||
if ($isCallable->yes()) { | ||
$callableArray = $this->getClassOrObjectAndMethods(); |
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 ineffective. This method was already called once inside findTypeAndMethodNames
above. It's a bit of a mess right now.
I just made sure this kind of mistake isn't possible: 30b9cd8
So the fix should probably be done inside findTypeAndMethodNames
but I'm not sure.
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.
So the fix should probably be done inside findTypeAndMethodNames but I'm not sure.
Not sure too. Depends of the purpose of the method.
Currently the findTypeAndMethodNames
returns all the possible callable with at least a Maybe certainty. Cf #3637 (comment)
Currently it's not possible to create
ConstantArrayTypeAndMethod::createConcrete($type, $method->getValue(), TrinaryLogic::createNo());
So if you have a string 'foo'|'bar' and only 'foo' is a callable name, if it will returns 'foo'.
But it doesn't mean that 'foo'|'bar' is always a callable.
That's why I added the logic in isCallable to check the count of possible callable didn't drop (which mean some value has No as certainty).
Reopened from #3637 since I messed up with my branches (and the previous changes was on 1.12.x branch...)
Closes phpstan/phpstan#12063