Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Bugfixes:
* Standalone Assembly: Do not ignore input after closing brace of top level block.
* Standard JSON: Catch errors properly when invalid "sources" are passed.
* Standard JSON: Ensure that library addresses supplied are of correct length and hex prefixed.
* Type Checker: Properly detect which array and struct types are unsupported by the old ABI encoder.
* Type Checker: Properly warn when using ``_offset`` and ``_slot`` for constants in inline assembly.
* Commandline interface: throw error if option is unknown

Expand Down
30 changes: 26 additions & 4 deletions libsolidity/analysis/TypeChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,29 @@ using namespace std;
using namespace dev;
using namespace dev::solidity;

namespace
{

bool typeSupportedByOldABIEncoder(Type const& _type)
{
if (_type.dataStoredIn(DataLocation::Storage))
return true;
else if (_type.category() == Type::Category::Struct)
return false;
else if (_type.category() == Type::Category::Array)
{
auto const& arrayType = dynamic_cast<ArrayType const&>(_type);
auto base = arrayType.baseType();
if (!typeSupportedByOldABIEncoder(*base))
return false;
else if (base->category() == Type::Category::Array && base->isDynamicallySized())
return false;
}
return true;
}

}


bool TypeChecker::checkTypeRequirements(ASTNode const& _contract)
{
Expand Down Expand Up @@ -561,13 +584,12 @@ bool TypeChecker::visit(FunctionDefinition const& _function)
m_errorReporter.fatalTypeError(var->location(), "Internal or recursive type is not allowed for public or external functions.");
if (
_function.visibility() > FunctionDefinition::Visibility::Internal &&
type(*var)->category() == Type::Category::Struct &&
!type(*var)->dataStoredIn(DataLocation::Storage) &&
!_function.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::ABIEncoderV2)
!_function.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::ABIEncoderV2) &&
!typeSupportedByOldABIEncoder(*type(*var))
)
m_errorReporter.typeError(
var->location(),
"Structs are only supported in the new experimental ABI encoder. "
"This type is only supported in the new experimental ABI encoder. "
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

"Use \"pragma experimental ABIEncoderV2;\" to enable the feature."
);

Expand Down
2 changes: 0 additions & 2 deletions libsolidity/ast/Types.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still disabled by the type checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function typeSupportedByOldABIEncoder is much stricter than what was there before.

else
return true;
}
Expand Down
56 changes: 56 additions & 0 deletions test/libsolidity/SolidityNameAndTypeResolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"(
Expand Down