-
Notifications
You must be signed in to change notification settings - Fork 478
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
random_int() return type and parameters rule #99
Conversation
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.
Thank you!
19, | ||
], | ||
[ | ||
'Cannot call random_int() with intersecting $min (int<-5, 0>) and $max (int<-1, 5>) parameters.', |
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.
This situation might be fine - the code might be correct. This is typically only reported on level 7 thanks to reportMaybes
config option.
Feel free to also add this situation to acceptTypes.php
in tests/PHPStan/Levels. If you add an offending code and re-run the tests, the asserting JSON files will be updated automatically. Most of the violations should be reported on level 6, but this one on level 7.
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.
This is the only thing I have not address yet.
$minType = $scope->getType($node->args[0]->value)->toInteger(); | ||
$maxType = $scope->getType($node->args[1]->value)->toInteger(); | ||
|
||
if ($minType instanceof ConstantIntegerType |
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.
We might be able to get rid of all of these ifs with Type::isSuperTypeOf()
method. It's really powerful :) If you imagine types as sets then this abstraction holds really well :)
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.
Yeah, the eureka moment was realising all I needed to do was create a type representing the allowable values for the $max
parameter.
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.
Also, you should look into the level 7 stuff with reportMaybe
I suggested in the first review. Otherwise it's mergeable! 👍
Yeah not entirely sure what should be considered a "maybe", if the max argument could be less than the min argument should that be a maybe? e.g. |
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.
Last change and we're good to go!
Thank you, I love it! |
This PR adds a return type extension which teaches PHPStan that
random_int()
will return an integer between the min/max parameters. The code I've written to get the lower/upper bounds could probably be extracted to be reused as it should handle crazy types likeint<0,10>|20|25|int<30,100>
which I don't think PHPStan will even create currently.Secondly there is a rule which performs validation of the input parameters to ensure
$min <= $max
always holds true. Not sure on what level this should be configured or if it belongs in the strict rules repo.Unfortunately it's not possible to validate
random_int(PHP_INT_MAX, PHP_INT_MIN)
as the min/max constants are just converted to a plain integer type, perhaps there is scope for{Min,Max}IntegerType
classes.