class_exists(Interface::class) is always false #2838
Conversation
|
I tried similar things in #2277 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. |
| $argType = $scope->getType($node->getArgs()[0]->value); | ||
| if (count($argType->getConstantStrings()) !== 1) { | ||
| return null; | ||
| } | ||
| $argConstantString = $argType->getConstantStrings()[0]; | ||
| if (!$this->reflectionProvider->hasClass($argConstantString->getValue())) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
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.
Yeah, I dont want to cover all edgecases..
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| if ($functionName === 'enum_exists' && ($reflection->isClass() || $reflection->isInterface() || $reflection->isTrait())) { | ||
| return false; |
There was a problem hiding this comment.
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.
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 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 :) |
Ok, then we can probably close this PR. |
Recently found bug caused by this in phpstan-doctrine.