-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Properly detect which array and struct types are unsupported by the old ABI encoder. #3549
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
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 |
|---|---|---|
|
|
@@ -1589,8 +1589,6 @@ bool ArrayType::canBeUsedExternally(bool _inLibrary) const | |
| return true; | ||
| else if (!m_baseType->canBeUsedExternally(_inLibrary)) | ||
| return false; | ||
| else if (m_baseType->category() == Category::Array && m_baseType->isDynamicallySized()) | ||
| return false; | ||
|
Contributor
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. So basically we enable multi level arrays now for non-experimental mode. Will it be still disabled due to no support for returndatacopy? Do we have a test for this?
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. It is still disabled by the type checker.
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. The function |
||
| else | ||
| return true; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -979,6 +979,62 @@ BOOST_AUTO_TEST_CASE(functions_with_stucts_of_non_external_types_in_interface_ne | |
| CHECK_ERROR(text, TypeError, "Internal or recursive type is not allowed for public or external functions."); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(returning_multi_dimensional_arrays_new_abi) | ||
| { | ||
| char const* text = R"( | ||
| pragma experimental ABIEncoderV2; | ||
|
|
||
| contract C { | ||
| function f() public pure returns (string[][]) {} | ||
| } | ||
| )"; | ||
| CHECK_WARNING(text, "Experimental features"); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(returning_multi_dimensional_arrays) | ||
| { | ||
| char const* text = R"( | ||
| contract C { | ||
| function f() public pure returns (string[][]) {} | ||
| } | ||
| )"; | ||
| CHECK_ERROR(text, TypeError, "only supported in the new experimental ABI encoder"); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(returning_multi_dimensional_static_arrays) | ||
| { | ||
| char const* text = R"( | ||
| contract C { | ||
| function f() public pure returns (uint[][2]) {} | ||
| } | ||
| )"; | ||
| CHECK_ERROR(text, TypeError, "only supported in the new experimental ABI encoder"); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(returning_arrays_in_structs_new_abi) | ||
| { | ||
| char const* text = R"( | ||
| pragma experimental ABIEncoderV2; | ||
|
|
||
| contract C { | ||
| struct S { string[] s; } | ||
| function f() public pure returns (S) {} | ||
| } | ||
| )"; | ||
| CHECK_WARNING(text, "Experimental features"); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(returning_arrays_in_structs_arrays) | ||
| { | ||
| char const* text = R"( | ||
| contract C { | ||
| struct S { string[] s; } | ||
| function f() public pure returns (S x) {} | ||
|
Contributor
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. Hm, why does this one needs a named return parameter while the other one doesn't?
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. Probably a rebasing problem. |
||
| } | ||
| )"; | ||
| CHECK_ERROR(text, TypeError, "only supported in the new experimental ABI encoder"); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(function_external_call_allowed_conversion) | ||
| { | ||
| char const* text = R"( | ||
|
|
||
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.
Were the current (not the new ones) tests only checking for "only supported" or we didn't had tests to cover this?
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.
We only had tests for structs, not structs inside arrays or arrays containing arrays etc.
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.
Yeah but none of the test expectations had to change so either there was no text or it didn't expect the entire message.
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 seems we did not have tests for that.
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.
Is this a blocker for merging? Please make clear statements.
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.
Grepped the sources, there was no test for this before.
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.
No, there are tests now hopefully covering all of it.