Improve loose comparison on IntegerRange containing zero #3764
Conversation
|
//cc @VincentLanglet fyi, as you implemented loose comparison on int ranges |
There was a problem hiding this comment.
At first sight, it seems ok.
The recent discovery I made about loose comparison was the fact that <= and >= behavior changed with the PHP Version. But I don't remember exactly the cases where the behavior changed base on PHP version and if it impacts your current PR.
I tried to implements it with #3484 / #3485 but didn't got a good enough solution for ondrej to merge it. I'm not against your opinion on it @staabm. Since you implemented compareConstantScalars you might have advice about how to rework this method.
|
This pull request has been marked as ready for review. |
| @@ -315,6 +315,16 @@ public function isSmallerThan(Type $otherType, PhpVersion $phpVersion): TrinaryL | |||
| $maxIsSmaller = (new ConstantIntegerType($this->max))->isSmallerThan($otherType, $phpVersion); | |||
| } | |||
|
|
|||
| // 0 can have different results in contrast to the interval edges, see https://3v4l.org/iGoti | |||
There was a problem hiding this comment.
This is expected, at least for me, better to document the -1 > null case.
I belive -1 > null is somethings that can be even fixed in php-src, as I would expect null to be compared as 0.
There was a problem hiding this comment.
feel free to open a php-src bug report. I guess they won't fix it because of BC concerns.
as it stands right now in PHPStan we should reflect php-src behaviour and don't invent our own rules for edge cases
There was a problem hiding this comment.
That might be a good reason to report >, >=, <, <= operator with nullable integer in phpstan-strict-rule no ?
https://phpstan.org/r/ed5e293c-721b-4b14-b9ea-aa42876f8a2c
thanks for the review
had a quick look and it looks like this is a PHP 7.x only edge case. low prio edge cases are pretty frustrating - especially when a lot of changes are needed - as we don't have enough resources to get them merged. I think your time is better spent somewhere else. thats just my opinion though |
|
conflicts resolved |
|
Thank you. |
relevant 3v4l.org links, as I think its pretty weird