-
Couldn't load subscription status.
- Fork 544
class_exists(Interface::class) is always false #2838
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
I tried similar things in #2277
I think you can get inspiration about a few more cases which need to be handled here in the linked PR.
beware: ondrej might have different opinions.. feedback on the linked PR was not discussed yet
and we should have an eye on issuebot, as IIRC there should be more open issues regarding this topic.
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 are missing things like
$c = _Enum::class;
if (rand(0,1)) {
$c = _Class::class;
}
class_exists($c);
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 dont want to cover all edgecases..
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'd rather be if this was somehow done in ClassExistsFunctionTypeSpecifyingExtension instead of ImpossibleCheckTypeHelper.
ClassExistsFunctionTypeSpecifyingExtension is "closer to the core" and can influence the correctness in more places, for example class_exists($foo) for $foo being class-string<SomeInterface> should lead to NeverType for $foo.
ImpossibleCheckTypeHelper is only informing the rules right at the end of the analysis.
I understand this can lead to some more work because telling that Scalar\String_ should be NeverType can break assumptions in some places.
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 these conditions could be improved. Name what it cannot be instead of what it can be, for example:
if ($functionName === 'interface_exists' && !$reflection->isInterface()) {
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.
But that can break if some future PHP version onboards new struct type. But that would probably lead to many changes everywhere, so why not...
I'd rather be if this was somehow done in ClassExistsFunctionTypeSpecifyingExtension
I agree, but cannot we do it as next step and merge this?
Note: I dont feel implementing that in near future.
We're in no rush here and doing this correctly can always teach us something new :)
We're in no rush here and doing this correctly can always teach us something new :)
Ok, then we can probably close this PR.
Recently found bug caused by this in phpstan-doctrine.