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
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. |
419daca to
3747223
Compare
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
|
|
||
| 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
| { | ||
| 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 ;)
5ac59c6 to
11e4f76
Compare
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.