-
Notifications
You must be signed in to change notification settings - Fork 436
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
Keep numeric-strings in str_repeat() #2914
Conversation
This pull request has been marked as ready for review. |
ef7d199
to
f4b72a7
Compare
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.
Please put AccessoryNumericStringType in ConstantStringType::generalize()
too.
@@ -74,6 +75,10 @@ public function getTypeFromFunctionCall( | |||
$accessoryTypes[] = new AccessoryLiteralStringType(); | |||
} | |||
|
|||
if ($inputType->isNumericString()->yes()) { | |||
$accessoryTypes[] = new AccessoryNumericStringType(); |
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'm afraid this can be a bit tricky. Repeating a numeric type multiple times can lead to a non-numeric type: https://3v4l.org/aRBdZ
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.
So we should probably do it only with a constant (literal) types with no decimal points.
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.
good catch. fixed, thanks.
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 it should only be done for strings that only contain digits. Because of these numeric strings: https://3v4l.org/U4bM2
It should also be ensured that the multiplier is not 0.
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 my, its even different across php versions ;)
6275102
to
480b972
Compare
3851f2f
to
0099b70
Compare
I had a look into blackfire profiles and did not see any bottlenecks/resource hogs |
c94ad6c
to
1e874c5
Compare
This pull request has been marked as ready for review. |
5449067
to
b579b25
Compare
b579b25
to
5b4a4a2
Compare
Thank you. |
closes phpstan/phpstan#10572