add Type::isError method #4530
add Type::isError method #4530
Conversation
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
ApiInstanceofTypeRule should be made aware of this 😊
canvural
force-pushed
the
is-error
branch
2 times, most recently
from
b940dc5 to
e8922d3
Compare
ondrejmirtes
force-pushed
the
is-error
branch
from
e8922d3 to
2cd5ddf
Compare
|
Do you also feel like doing |
Didn't ever need that I guess, but sure I can. In a separate PR? |
|
The mutations look valid, but I don't know how I can change them in bulk. Edit: I guess |
|
I'm not sure how to cover the mutations with tests here. i'm more concerned about the things the issue bot found https://github.com/phpstan/phpstan-src/actions/runs/19215786400, I'm not sure why that's happening. |
| if ($this->default !== null) { | ||
| $recursionGuard = RecursionGuard::runOnObjectIdentity($this->default, fn () => $this->default->describe($level)); | ||
| if (!$recursionGuard instanceof ErrorType) { | ||
| if (is_string($recursionGuard)) { |
There was a problem hiding this comment.
Why? It can only be string or ErrorType here. So I thought is_string is ok instead of instanceof
|
|
||
| $classHasProperty = RecursionGuard::run($this, static fn (): bool => $classReflection->hasProperty($propertyName)); | ||
| if ($classHasProperty === true || $classHasProperty instanceof ErrorType) { | ||
| if ($classHasProperty !== false) { |
There was a problem hiding this comment.
are we sure this does not change existing logic?
There was a problem hiding this comment.
Yeah, doesn't change it. It can only be bool or ErrorType.
|
just a gut feeling in case the 2 above comments regarding behaviour changes are not valid: another source of problems could be
because |
| $containsUnresolvable = false; | ||
| TypeTraverser::map($type, static function (Type $type, callable $traverse) use (&$containsUnresolvable): Type { | ||
| if ($type instanceof ErrorType) { | ||
| if ($type->isError()->yes()) { |
There was a problem hiding this comment.
I debugged a bit and I think the issue bot errors might be valid. It's always an issue with ValueOfType and it comes down to this line. When we do $type->isError() here ValueOfType resolves the underlying value and it resolves to ErrorType. We do not get the issue with instanceof check because that's always ValueOfType vs ErrorType
Does it make sense?
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
In #4547
$expressionType instanceof NeverType
by
!$expressionType->isNever()->no()
But here you're replacing
$type instanceof ErrorType
by
$type->isError()->yes()
How did you choose ? I feel like we should have consistency...
| return TrinaryLogic::createMaybe(); | ||
| } | ||
|
|
||
| public function isError(): TrinaryLogic |
There was a problem hiding this comment.
Isn't it just simpler to always return no ?
Cause currently MixedType instanceof ErrorType is just false.
And anyway, I don't think there is a isError()->maybe() used somewhere...
ErrorType extends MixedType for simplicity, but a "real" MixedType is never an error.
And Mixed~Error is not a Type I often saw...
|
|
||
| $classHasProperty = RecursionGuard::run($this, static fn (): bool => $classReflection->hasProperty($propertyName)); | ||
| if ($classHasProperty === true || $classHasProperty instanceof ErrorType) { | ||
| if ($classHasProperty !== false) { |
There was a problem hiding this comment.
Might be easier to understand
if ($classHasProperty === true || $classHasProperty->isError()->yes())
| $containsUnresolvable = false; | ||
| TypeTraverser::map($type, static function (Type $type, callable $traverse) use (&$containsUnresolvable): Type { | ||
| if ($type instanceof ErrorType) { | ||
| if ($type->isError()->yes()) { |
4 participants
This one does not add too much value, but I just didn't want to do
instanceof ErrorTypein my extensions 😄