1.x: Fix negation types for Inspector #861
Conversation
tests/src/Type/data/inspector.php
Outdated
| // Inspector::assertAll() | ||
| $callable = fn (string $value): bool => $value === 'foo'; | ||
| $input = mixed_function(); | ||
| assert(Inspector::assertAll($callable, $input)); | ||
| assertType("iterable", $input); | ||
| if (Inspector::assertAll($callable, $input)) { | ||
| assertType("iterable", $input); | ||
| } | ||
| else { | ||
| assertType('mixed~iterable', $input); | ||
| } | ||
|
|
||
| $input = mixed_function(); | ||
| $callable = is_string(...); | ||
| assert(Inspector::assertAll($callable, $input)); | ||
| assertType('iterable', $input); | ||
| if (Inspector::assertAll($callable, $input)) { | ||
| assertType('iterable', $input); | ||
| } | ||
| else { | ||
| assertType('mixed~iterable', $input); | ||
| } |
There was a problem hiding this comment.
@mglaman it's kind of works, but I still don't like it.
$input = mixed_function();
$callable = is_string(...);
if (Inspector::assertAll($callable, $input)) {
assertType('iterable', $input);
} else {
assertType('mixed~iterable', $input);
}Actually, it doesn't mean that $input is not iterable after that ::assertAll(). It just means it does not contain strings, but it can still be iterable... I think in else, it should be mixed, as we can't guarantee it is not iterable. We can't narrow it down to mixed~iterable<string>, so we better fallback to mixed. But if it passes that condition, it is clearly iterable, so we need this method support, but it should be improved.
There was a problem hiding this comment.
I see what you mean, but it is never intended to be used in conditionals like this – it's meant to be used in asserts which fail if false. So I think it's OK that the condition fails. Because none of the type specifying assert extensions would work
EDIT: Unless there are examples in PHPStan for is_string that make it feasible. I only read for assert
| // In a negation context, we cannot precisely narrow types because we do | ||
| // not know the exact logic of the callable function. This means we | ||
| // cannot safely return 'mixed~iterable' since the value might still be | ||
| // a valid iterable. | ||
| // | ||
| // For example, a negation context with an 'is_string(...)' callable | ||
| // does not necessarily mean that the value cannot be an | ||
| // 'iterable<int>'. In such cases, it is safer to skip type narrowing | ||
| // altogether to prevent introducing new bugs into the code. | ||
| if ($context->false()) { | ||
| return new SpecifiedTypes(); | ||
| } | ||
|
|
||
| return $this->typeSpecifier->create( | ||
| $node->getArgs()[1]->value, | ||
| new IterableType(new MixedType(), new MixedType()), | ||
| TypeSpecifierContext::createTruthy(), | ||
| $context, | ||
| false, | ||
| $scope, | ||
| ); |
There was a problem hiding this comment.
@mglaman now it works much safer:
// Inspector::assertAll()
$callable = fn (string $value): bool => $value === 'foo';
$input = mixed_function();
if (Inspector::assertAll($callable, $input)) {
assertType("iterable", $input);
}
else {
assertType('mixed', $input);
}We can only confirm that it is iterable if the result is TRUE, but when it's FALSE, we cannot be sure what the type is, so we should keep its original type, which is mixed.
It might be possible to narrow it based on the callable, but I think for now it is too complex and not worth the effort. We can always improve it later.
There was a problem hiding this comment.
This works for me, and good call on the comment
Port of #860