Skip to content

Make IntegerRangeType represent open intervals properly #409

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

Merged

Conversation

jlherren
Copy link
Contributor

This removes the usage of PHP_INT_MIN and PHP_INT_MAX to mark open-ended intervals and instead uses null for that purpose. Since using getMin() and getMax() now became slightly more complicated for callers, a lot of IntegerRangeType-related logic has been moved into the class itself.

I especially like that some type combinator logic is now inside the type class, but without too much pressure, since it is allowed to return null when it can't handle the situation. Perhaps this could be extended into the entire Type hierarchy.

@jlherren jlherren force-pushed the integer-range-type-improvements branch from 8ef3293 to 7a7ae06 Compare December 14, 2020 20:45
@ondrejmirtes
Copy link
Member

I really like this approach, moving stuff to Type usually tends to clean things up :) I'd definitely like to have the method on Type, but it should have a more specific name, like shiftInteger or something. It would allow also handling for UnionType of IntegerRangeTypes for example. And it should return ErrorType in cases where it doesn't make sense (shifting object by an integer amount for example).

I'll review and merge this PR as it is, we can move and rename the method in a late rPR.

@ondrejmirtes
Copy link
Member

Or shiftByInteger?

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like method names should be verbs (commands)...

This removes the usage of PHP_INT_MIN and PHP_INT_MAX to mark open-ended
intervals and instead uses null for that purpose.  Since using getMin() and
getMax() now became slightly more complicated for callers, a lot of
IntegerRangeType related logic has been moved into the class itself.
@jlherren jlherren force-pushed the integer-range-type-improvements branch from 7a7ae06 to 401815f Compare December 15, 2020 21:04
@jlherren
Copy link
Contributor Author

Is this okay now, or should I change anything?

@ondrejmirtes
Copy link
Member

Yes, thank you very much :) As a next step, we should move shift() to Type. I think it could be used for PostInc and similar operations not only on IntegerRangeType (and unions of them), but also on ConstantIntegerType in a meaningful way.

@ondrejmirtes ondrejmirtes merged commit 61d9de9 into phpstan:master Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants