-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Test coverage for constants and literal expressions with type smaller than uint256
#16456
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| uint8 constant base = 255; | ||
| contract C layout at base + 257 { | ||
| uint public x = 7; | ||
| function test() public returns (uint r) { | ||
| assembly { | ||
| r := sload(512) | ||
| sstore(512, add(r, 1)) | ||
| } | ||
| } | ||
| } | ||
| // ---- | ||
| // test() -> 7 | ||
| // x() -> 8 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| uint16 constant base = 511; | ||
| uint8 constant offset = 1; | ||
| contract C layout at base + offset { | ||
| uint public x = 7; | ||
| function test() public returns (uint r) { | ||
| assembly { | ||
| r := sload(512) | ||
| sstore(512, add(r, 1)) | ||
| } | ||
| } | ||
| } | ||
| // ---- | ||
| // test() -> 7 | ||
| // x() -> 8 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| contract C { | ||
| uint8 constant LEN = 254; | ||
| uint[LEN + 1] array1; | ||
| uint[LEN + 256] array2; | ||
| uint[LEN * 2] array3; // Error, outside of constant and literal range | ||
|
Comment on lines
+4
to
+5
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I missing something or should both of these overflow? In either case the result is outside of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the literal gets converted to the smallest integer type that can hold it rather than the type of the limited-precision operand? I did not expect that. Looks like we do have that (somewhat) documented under Rational and Integer Literals. I think we should at some point extend these docs to have more than just a single note for such conversions and explain the rules properly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that's the case. I found it a bit weird at the time, but then remembered reading that in the docs. |
||
| } | ||
| // ---- | ||
| // TypeError 2643: (106-113): Arithmetic error when computing constant value. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| contract C { | ||
| uint8 constant LEN = 255; | ||
| uint16 constant MORE = 32767; | ||
| uint[LEN] array1; | ||
| uint[LEN + MORE] array2; | ||
| uint[LEN * MORE] array3; // Error, outside uint16 range | ||
| } | ||
| // ---- | ||
| // TypeError 2643: (137-147): Arithmetic error when computing constant value. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| uint constant LEN = 2**255; | ||
| uint constant MUL = 2; | ||
| contract C { | ||
| uint[LEN * MUL] array; | ||
| } | ||
| // ---- | ||
| // TypeError 2643: (73-82): Arithmetic error when computing constant value. |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need coverage also for underflow in multiplication. Consider this: uint8 constant UNSIGNED2 = 2;
int8 constant SIGNED2 = 2;
contract S layout at SIGNED2 * -1 * -1 { // Error: Base slot expression of type 'int8' is not convertible to uint256.
uint[SIGNED2 * -1 * -1] a; // OK
uint[2 * -1 * -1] b; // OK
}
contract U layout at UNSIGNED2 * -1 * -1 { // Built-in binary operator * cannot be applied to types uint8 and int_const -1.
uint[UNSIGNED2 * -1 * -1] a; // Error: Operator * not compatible with types uint8 and int_const -1
uint[2 * -1 * -1] b; // OK
}And this shows that we were indeed missing some coverage. We have a discrepancy between layouts and arrays here. Fortunately this is not a bug in how underflow is handled but rather the fact that we require the layout expression to be unsigned, even if the value is signed. I think the two should work the same way. IMO it would be better for both to only accept unsigned, but changing that for arrays would be breaking. We have to change it the other way then - allow signed types in the specifier. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| uint constant START = 10; | ||
| uint constant END = 2; | ||
| contract C { | ||
| uint[END - START] array; | ||
| } | ||
| // ---- | ||
| // TypeError 2643: (71-82): Arithmetic error when computing constant value. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| uint8 constant base = 2; | ||
| contract A layout at base + 256 {} | ||
| contract B layout at base + 255 {} // Error, outside of range of constant and literal | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is bizarre. Also the fact that you cannot fix it with an explicit type conversion, but you can use a unintuitive workaround like We really need #16420. |
||
| // ---- | ||
| // TypeError 2643: (81-91): Arithmetic error when computing constant value. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| uint8 constant base = 255; | ||
| uint16 constant offset = 32767; | ||
| contract A layout at base {} // Ok | ||
| contract B layout at base + offset {} // Ok, inside uint16 range | ||
| contract D layout at base * offset {} // Error, outside uint16 range | ||
| // ---- | ||
| // TypeError 2643: (193-206): Arithmetic error when computing constant value. |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should cover shifts as well.
where None of these should overflow, neither in limited nor unlimited precision. I checked this and the second one actually seems to be buggy, because it produces an error:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was looking into this and found the issue. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| uint constant base = 2**256 - 1; | ||
| contract C layout at base + 1 {} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we also need coverage for intermediate overflows and underflows:
where In unlimited precision all of these expressions would be valid. Would not hurt to have these intermediate overflows covered for
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also have a test verifying that division on constants is performed in limited precision and therefore truncates:
This would be an error in unlimited precision. |
||
| // ---- | ||
| // TypeError 2643: (54-62): Arithmetic error when computing constant value. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| uint constant base = 2**256 - 1; | ||
| uint constant offset = 512; | ||
| contract C layout at base + offset {} | ||
| // ---- | ||
| // TypeError 2643: (82-95): Arithmetic error when computing constant value. |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it is redundant. I think I meant to write |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| uint constant base = 512; | ||
| contract C layout at base - 1024 {} | ||
| // ---- | ||
| // TypeError 2643: (47-58): Arithmetic error when computing constant value. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| uint constant base = 256; | ||
| uint constant offset = 512; | ||
| contract C layout at base - offset {} | ||
| // ---- | ||
| // TypeError 2643: (75-88): Arithmetic error when computing constant value. |
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.
Please use capitals for constant names (e.g.
BASE). This makes tests easier to read because you see at a glance what is what.