-
-
Notifications
You must be signed in to change notification settings - Fork 840
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
fix: update upper bound for signed integers in min
and max
folding
#3288
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #3288 +/- ##
==========================================
+ Coverage 88.86% 88.92% +0.06%
==========================================
Files 84 84
Lines 10597 10605 +8
Branches 2213 2215 +2
==========================================
+ Hits 9417 9431 +14
+ Misses 770 768 -2
+ Partials 410 406 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
tests/parser/syntax/test_minmax.py
Outdated
""" | ||
@external | ||
def foo(): | ||
a: int256 = min(-1, max_value(int256) + 1) |
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 fails for a different reason, no?
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.
It fails on the same branch:
vyper/vyper/builtins/functions.py
Lines 2019 to 2020 in efb0032
if isinstance(left, int) and (min(left, right) < 0 and max(left, right) >= 2 ** 255): | |
raise TypeMismatch("Cannot perform action between dislike numeric types", node) |
Although max_value(int256) + 1
is out of bounds, it is currently not type-checked before folding so it is not flagged as an error. Therefore, this slightly modified example would also compile:
@external
def foo():
a: int248 = min(1, max_value(int248) + 1)
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.
the example is a bit confusing tbh because the operands are not really valid to begin with. and i'm realizing the fix itself does not block examples which are in the int255 range but do not typecheck, like:
min( min_value(int16)/*type: int16*/, max_value(int8)/*type: int8*/)
can we use get_common_types
here like other builtins which deal with numeric literals? i think that can fix the typechecking issue.
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, if/when we move constant folding after typechecking this will be moot and we can remove the checks inside of the evalute implementations
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.
the example is a bit confusing tbh because the operands are not really valid to begin with.
I have moved this test to #3201 instead. I think that PR is more relevant for addressing these cases.
What I did
Fix #3284.
How I did it
Update the bound to
int256
, which is>= 2 ** 255
.How to verify it
See tests.
Commit message
Description for the changelog
Update upper bound for signed integers in
min
andmax
foldingCute Animal Picture