implement isClassStringType() on Type #1970
Conversation
|
in 11ca353 I added a test from phpstan/phpstan#1267 (comment) which was not covered by #1969 |
phpstan-baseline.neon
Outdated
| @@ -392,6 +392,14 @@ parameters: | |||
| count: 1 | |||
| path: src/Testing/PHPStanTestCase.php | |||
|
|
|||
| - | |||
There was a problem hiding this comment.
I was able to drop all uses of isClassString and instanceof ClassStringType with 48f8635 but these 2 occurences. These seemed harder to solve, therefore I put them into the baseline.
There was a problem hiding this comment.
I might be missing something, but it looks like the internals of isClassString() should be moved into isClassStringType() and wrapped in a trinary. At least you don't have to use the deprecated method in the new one then. You could even turn it around and let the deprecated one use the new one internally?
Not sure what the problem with the other usage was 🤔
There was a problem hiding this comment.
after a couple of hours sleep I was able to resolve the remining stuff :)
|
This pull request has been marked as ready for review. |
staabm
force-pushed
the
type-class-string
branch
3 times, most recently
from
419daca
to
3747223
Compare
staabm
force-pushed
the
type-class-string
branch
from
3747223
to
770354c
Compare
src/Type/MixedType.php
Outdated
| { | ||
| if ($this->subtractedType !== null) { | ||
| $stringType = new StringType(); | ||
| if ($this->subtractedType->isSuperTypeOf($stringType)->yes()) { |
There was a problem hiding this comment.
Alright, so mixed~string is definitely no. mixed~class-string should also be no.
There was a problem hiding this comment.
fixed, and tests added.
src/Type/ClassStringType.php
Outdated
| @@ -27,7 +27,7 @@ public function accepts(Type $type, bool $strictTypes): TrinaryLogic | |||
| } | |||
|
|
|||
| if ($type instanceof ConstantStringType) { | |||
| return TrinaryLogic::createFromBoolean($type->isClassString()); | |||
| return TrinaryLogic::createFromBoolean($type->isClassStringType()->yes()); | |||
There was a problem hiding this comment.
I don't get it, why degrade maybe to no?
You can do just return $type->isClassStringType(); and it's gonna be even better.
There was a problem hiding this comment.
I made a 1:1 translation of pre-existing logic. didn't realize that returning the trinary from isClassStringType would be even more precise.
fixed
src/Type/ClassStringType.php
Outdated
| @@ -44,7 +44,7 @@ public function accepts(Type $type, bool $strictTypes): TrinaryLogic | |||
| public function isSuperTypeOf(Type $type): TrinaryLogic | |||
| { | |||
| if ($type instanceof ConstantStringType) { | |||
| return TrinaryLogic::createFromBoolean($type->isClassString()); | |||
| return TrinaryLogic::createFromBoolean($type->isClassStringType()->yes()); | |||
There was a problem hiding this comment.
You can simplify this and return the original trinary here and above maybe?
There was a problem hiding this comment.
I thought so too, but I think the alternative would be $type->isClassStringType()->and(Trinary::createYes()) which is not that readable either. just returning doesn't work, because of Maybe.
There was a problem hiding this comment.
I was wrong, see https://github.com/phpstan/phpstan-src/pull/1970/files#r1014798873 ;)
staabm
force-pushed
the
type-class-string
branch
from
5ac59c6
to
11e4f76
Compare
staabm
force-pushed
the
type-class-string
branch
from
11e4f76
to
e3ddd35
Compare
|
BTW :) b78ff97 Similarly it should be possible to simplify complicated GenericClassStringType::accepts() and isSuperTypeOf(), but that's gonna need more methods on Type. |
|
Thank you. |
No description provided.