-
-
Notifications
You must be signed in to change notification settings - Fork 788
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[codegen]: bounds check for signed index accesses #3817
fix[codegen]: bounds check for signed index accesses #3817
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.
Would it make more sense to prevent indexing with signed types entirely or is it something that should be possible?
i think people want to use signed integers without conversion, a common usage is like for ix: int256 in ...:
ret += array[ix] and it would be annoying to have to write it with a for ix: int256 in ...:
ret += array[convert(ix, uint256)] |
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 order of the args of clamp2
seems incorrect and should probably be instead 0, ix, bound, ix.typ.is_signed
Additionally, even with the other order, the check is incorrect for signed integer types when the array's size is greater than or equal to 2**255
. [sle, arg, hi]
performs a signed comparison but hi
, the length of the array is an unsigned integer interpreted as a signed integer.
For example, compiling:
arr: public(uint256[max_value(uint256)-1])
@external
def foo():
self.arr[1] = 3
raises
vyper.exceptions.StaticAssertionException: assertion found to fail at compile time. (hint: did you mean `raise`?) [assert,
[and,
[sge, 1 <1>, 0],
[sle,
1 <1>,
115792089237316195423570985008687907853269984665640564039457584007913129639934]]]
ah sorry, i should have marked as draft until i have tests passing
hmm, so i guess it must be |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3817 +/- ##
==========================================
+ Coverage 85.15% 85.21% +0.05%
==========================================
Files 92 92
Lines 13907 13912 +5
Branches 3116 3117 +1
==========================================
+ Hits 11843 11855 +12
+ Misses 1571 1564 -7
Partials 493 493 ☔ View full report in Codecov by Sentry. |
chore: add array indexing tests
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.
LGTM :)
What I did
fix #3762
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture