-
Notifications
You must be signed in to change notification settings - Fork 551
NodeScopeResolver: Prevent repetitive union of static types #4843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.