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
use IntegerRangeType
in modulo-operator
#614
Conversation
Please squash and rebase your commits so there aren't any merge commits with master, but a clean linear history. Thanks. |
c96bb9c
to
f5fd4e2
Compare
f5fd4e2
to
cb50241
Compare
@ondrejmirtes squashed and rebased |
$rangeMax = null; | ||
if ($rightType instanceof IntegerRangeType) { | ||
$rangeMax = $rightType->getMax() !== null ? $rightType->getMax() - 1 : null; | ||
} elseif ($rightType 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.
I think this will lead to creating int<X, max>
when you have 1|2|3
which is wrong I guess.
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.
another great catch. added a test and a fix
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.
That's because instanceof *Type
is almost always wrong and you can find edge-cases like that.. https://phpstan.org/developing-extensions/type-system#querying-a-specific-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.
Right now it's still wrong for unions of disjoint ranges like int<min, 3>|int<4, max>
.
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.
got this case and a few more also covered. I guess you won't like that, I had to add a few more instanceof *Type
calls.
I somehow get the feeling you let me built this thing intentionally and we will now refactor the logic regarding merging IntegerRangeType
and ConstantIntegerType
types (or unions of it) into either TypeUtils
or IntegerRange
class, right? ;)
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 guess it's fine like that. A correct approach is to add use-case based methods on PHPStan\Type\Type
that would tell us exactly what we need here. Each Type
implementation would return what it knows.
I'm preparing myself mentally for this refactoring for a long time, in the end PHPStan\Type\Type
might have hundreds of methods, and it might be the only valid situation considered quality code in the universe 😂
IntegerRangeType
in modulo-operator
Thank you. |
closes phpstan/phpstan#5442