NodeScopeResolver: Prevent repetitive union of static types#4843
NodeScopeResolver: Prevent repetitive union of static types#4843ondrejmirtes merged 5 commits intophpstan:2.1.xfrom
Conversation
|
This pull request has been marked as ready for review. |
src/Type/StaticTypeFactory.php
Outdated
| return $truthy; | ||
| } | ||
|
|
||
| public static function getNonIntArrayKeyValueType(): Type |
There was a problem hiding this comment.
I think name could be challenged for 3 reasons:
-
Existing methods are called
falsey(andtruthy) and notgetFalseyType.
Should it be something likenonIntArrayKeyValue? -
I had trouble ton understand what was
IntArrayKeyValueTypeandnonIntArrayKeyValueType. Should another name be use ? With offset maybe ? -
nonIntis not really valid since we was using$offsetType->isInteger()->yes()forgetIntArrayKeyValueTypeandgetNonIntArrayKeyValueTypein the other case, including the MAYBE case. So it's more ageneralcase.
Maybe something like
generalOffsetAccessible()
intOffsetAccessible()
There was a problem hiding this comment.
great suggestions. I was not happy with the names either.
I now have a mix of your suggestions and my own idea.
wdyt?
There was a problem hiding this comment.
You'll have to rename the variable inside the method too.
It's just unclear why you're calling this OffsetValueType.
If I have $foo[$bar] = $baz,
- bar is the key/index/offset/dim type
- baz is the value type
You seems to call $foo the offsetValue. Is it something often used in PHPStan codebase ?
I assume it's related to HasOffsetValueType but here we are doing nothing with the Value, so it's more HasOffsetType no ? Like hasIntOffset.
There was a problem hiding this comment.
the method is returning the Type for $baz (speaking from your example) - thats why I call it OffsetValueType
There was a problem hiding this comment.
Oh, I thought it was returning the type for $foo.
With the idea that $string[$int] is allowed but not $string[$string].
I'm surprise by the fact it returns $baz cause then, it shouldn't forbid scalar... (?)
There was a problem hiding this comment.
ohh you are right. its $foo not $baz and thats the wrong term as you already pointed out
|
Nice, thank you! |
re-use types introduced in #4841
removes a few
TypeCombinator::unioncallssince
TypeCombinator::unionis the slowest path we have right now, this is a great thing to have.