-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Remove the wrong codegen assumption that a conversion between non-byte calldata arrays can never happen #16415
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?
Conversation
|
There was an error when running Please check that your changes are working as intended. |
192825a to
1608553
Compare
1608553 to
424cd08
Compare
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 add tests for slices and static arrays as well. At least the latter also trigger the ICE.
contract C {
function f(uint[] calldata a) public {
(uint[] calldata b, ) = true ? (a[:], 0) : (a[0:1], 0);
}
}contract C {
function f(uint[3] calldata a) public {
(uint[3] calldata b, ) = true ? (a, 0) : (a, 0);
}
}Also a mix of arrays and slices ((a, 0) : (a[:], 0)).
| if (_to.dataStoredIn(DataLocation::CallData)) | ||
| solAssert( | ||
| _from.dataStoredIn(DataLocation::CallData) && _from.isByteArrayOrString() && _to.isByteArrayOrString(), | ||
| "" | ||
| ); | ||
| solAssert( | ||
| !_to.dataStoredIn(DataLocation::CallData) || |
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'd keep the if here. This is usually more readable than the negation you have to add to incorporate it into condition and our asserts are not debug anyway (we don't remove them from release builds).
| ss[0].ns = new N[](1); | ||
|
|
||
| ss[0].ns[0].addr = address(this); | ||
| ss[0].ns[0].data = "abdeff00"; | ||
|
|
||
| ss[0].data = "abdeff"; |
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.
| ss[0].ns = new N[](1); | |
| ss[0].ns[0].addr = address(this); | |
| ss[0].ns[0].data = "abdeff00"; | |
| ss[0].data = "abdeff"; | |
| ss[0].ns = new N[](1); | |
| ss[0].ns[0].addr = address(this); | |
| ss[0].ns[0].data = "abdeff00"; | |
| ss[0].data = "abdeff"; |
| return b; | ||
| } | ||
| } | ||
| // Two last results and input to last call must all be equal. |
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 would be better if the expectations were just formatted in a way that makes it more obvious at a glance that they're correct.
For example you could format strings as actual strings (soltest does accept those). I'd also use some regular-looking hard-coded addresses instead of things like tx.origin and msg.sender or address(this). Those are pretty noisy-looking, not easy to fish out visually. For fields with lots of zeros you can either use hex strings (those are not automatically padded to 32 bytes unlike integers) or left()/right() helpers to pad a shorter value to 32 bytes. I also usually use hex for offsets but decimals for sizes, which makes encoding a bit easier to decode visually.
Having to read it very carefully quickly gets tedious when reading a lot of tests. Overall we should try a bit harder optimize expectations for being able to quickly verifying them.
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.
Can you check #16066 (comment)? We should really check why something like true ? (a, true) : (a, true) does not trigger this bug. This may mean that either we're not doing an array conversion in some case were we need it or that we shouldn't be doing it in the true ? (a, 0) : (a, 0) case.
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.
TBH I'd have no idea what this test does just from the current name. What do you mean by "ordinary types"?
This should be way more specific:
conversion_of_ordinary_types_array.sol -> non_byte_calldata_array_conversion_in_ternary_with_tuple_operands.sol
| function g(uint[] calldata a) public returns(uint, uint, uint, uint) { | ||
| (uint[] calldata b, ) = true ? (a, 0) : (a, 0); | ||
| return (b.length, b[0], b[1], b[2]); | ||
| } |
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'd put this bit in a separate file. It's independent of the larger case, and when one of those fails it's usually nicer to have a smaller one that's easier to understand to debug.
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 PR is missing a changelog entry.
This PR allows generating a array conversion code for the same types array. It was mentioned already in #10893 (comment) that is should be allowed for equal types but it was not added eventually. Not sure why.
Fixes: #16066