From ae6996efc198e555c7c7060e0db5ea4c656b430b Mon Sep 17 00:00:00 2001 From: Alexander Arlt Date: Wed, 3 Feb 2021 15:48:45 -0500 Subject: [PATCH] Fix issue with pop on storage array. --- Changelog.md | 1 + libsolidity/analysis/TypeChecker.cpp | 5 +-- libsolidity/ast/ASTAnnotations.h | 1 + libsolidity/ast/Types.cpp | 30 +++++++------- libsolidity/ast/Types.h | 1 - libsolidity/codegen/ExpressionCompiler.cpp | 41 ++++++++++--------- .../codegen/ir/IRGeneratorForStatements.cpp | 33 +++++++++------ libsolidity/formal/SMTEncoder.cpp | 5 +-- .../semanticTests/array/pop/parenthesized.sol | 12 ++++++ 9 files changed, 73 insertions(+), 56 deletions(-) create mode 100644 test/libsolidity/semanticTests/array/pop/parenthesized.sol diff --git a/Changelog.md b/Changelog.md index 9cbf4bb59..bdf4987a9 100644 --- a/Changelog.md +++ b/Changelog.md @@ -18,6 +18,7 @@ Bugfixes: AST Changes: * Support field `documentation` to hold NatSpec comments above each statement. * Adds `nameLocation` to declarations to represent the exact location of the symbolic name. + * Removed the redundant function type "bytearraypush" - replaced by "arraypush". ### 0.8.1 (2021-01-27) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 830f3d458..1bcfdcdc8 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -2345,10 +2345,7 @@ bool TypeChecker::visit(FunctionCall const& _functionCall) *_functionCall.expression().annotation().isPure && functionType->isPure(); - if ( - functionType->kind() == FunctionType::Kind::ArrayPush || - functionType->kind() == FunctionType::Kind::ByteArrayPush - ) + if (functionType->kind() == FunctionType::Kind::ArrayPush) isLValue = functionType->parameterTypes().empty(); break; diff --git a/libsolidity/ast/ASTAnnotations.h b/libsolidity/ast/ASTAnnotations.h index f2c2d5812..dbf70d7f1 100644 --- a/libsolidity/ast/ASTAnnotations.h +++ b/libsolidity/ast/ASTAnnotations.h @@ -47,6 +47,7 @@ namespace solidity::frontend class Type; using TypePointer = Type const*; +class ArrayType; using namespace util; struct CallGraph; diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp index a16fadc0f..032ec0ad9 100644 --- a/libsolidity/ast/Types.cpp +++ b/libsolidity/ast/Types.cpp @@ -1806,27 +1806,28 @@ MemberList::MemberMap ArrayType::nativeMembers(ASTNode const*) const members.emplace_back("length", TypeProvider::uint256()); if (isDynamicallySized() && location() == DataLocation::Storage) { + Type const* thisAsPointer = TypeProvider::withLocation(this, location(), true); members.emplace_back("push", TypeProvider::function( - TypePointers{}, + TypePointers{thisAsPointer}, TypePointers{baseType()}, - strings{}, strings{string()}, - isByteArray() ? FunctionType::Kind::ByteArrayPush : FunctionType::Kind::ArrayPush - )); + strings{string()}, + FunctionType::Kind::ArrayPush + )->asBoundFunction()); members.emplace_back("push", TypeProvider::function( - TypePointers{baseType()}, + TypePointers{thisAsPointer, baseType()}, TypePointers{}, - strings{string()}, + strings{string(),string()}, strings{}, - isByteArray() ? FunctionType::Kind::ByteArrayPush : FunctionType::Kind::ArrayPush - )); + FunctionType::Kind::ArrayPush + )->asBoundFunction()); members.emplace_back("pop", TypeProvider::function( + TypePointers{thisAsPointer}, TypePointers{}, - TypePointers{}, - strings{}, + strings{string()}, strings{}, FunctionType::Kind::ArrayPop - )); + )->asBoundFunction()); } } return members; @@ -2876,7 +2877,6 @@ string FunctionType::richIdentifier() const case Kind::MulMod: id += "mulmod"; break; case Kind::ArrayPush: id += "arraypush"; break; case Kind::ArrayPop: id += "arraypop"; break; - case Kind::ByteArrayPush: id += "bytearraypush"; break; case Kind::ObjectCreation: id += "objectcreation"; break; case Kind::Assert: id += "assert"; break; case Kind::Require: id += "require"; break; @@ -3083,8 +3083,8 @@ vector> FunctionType::makeStackItems() const break; case Kind::ArrayPush: case Kind::ArrayPop: - case Kind::ByteArrayPush: - slots = {make_tuple("slot", TypeProvider::uint256())}; + solAssert(bound(), ""); + slots = {}; break; default: break; @@ -3483,8 +3483,6 @@ TypePointer FunctionType::copyAndSetCallOptions(bool _setGas, bool _setValue, bo FunctionTypePointer FunctionType::asBoundFunction() const { solAssert(!m_parameterTypes.empty(), ""); - FunctionDefinition const* fun = dynamic_cast(m_declaration); - solAssert(fun && fun->libraryFunction(), ""); solAssert(!m_gasSet, ""); solAssert(!m_valueSet, ""); solAssert(!m_saltSet, ""); diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h index 5c58ebf4c..2ee749f1a 100644 --- a/libsolidity/ast/Types.h +++ b/libsolidity/ast/Types.h @@ -1157,7 +1157,6 @@ public: MulMod, ///< MULMOD ArrayPush, ///< .push() to a dynamically sized array in storage ArrayPop, ///< .pop() from a dynamically sized array in storage - ByteArrayPush, ///< .push() to a dynamically sized byte array in storage ObjectCreation, ///< array creation using new Assert, ///< assert() Require, ///< require() diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 4a3b7c7b4..f0e934e21 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -584,8 +584,12 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) { FunctionType const& function = *functionType; if (function.bound()) - // Only delegatecall and internal functions can be bound, this might be lifted later. - solAssert(function.kind() == FunctionType::Kind::DelegateCall || function.kind() == FunctionType::Kind::Internal, ""); + solAssert( + function.kind() == FunctionType::Kind::DelegateCall || + function.kind() == FunctionType::Kind::Internal || + function.kind() == FunctionType::Kind::ArrayPush || + function.kind() == FunctionType::Kind::ArrayPop, + ""); switch (function.kind()) { case FunctionType::Kind::Declaration: @@ -945,9 +949,9 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) appendExternalFunctionCall(function, arguments, false); break; } - case FunctionType::Kind::ByteArrayPush: case FunctionType::Kind::ArrayPush: { + solAssert(function.bound(), ""); _functionCall.expression().accept(*this); if (function.parameterTypes().size() == 0) @@ -955,10 +959,8 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) auto paramType = function.returnParameterTypes().at(0); solAssert(paramType, ""); - ArrayType const* arrayType = - function.kind() == FunctionType::Kind::ArrayPush ? - TypeProvider::array(DataLocation::Storage, paramType) : - TypeProvider::bytesStorage(); + ArrayType const* arrayType = dynamic_cast(function.selfType()); + solAssert(arrayType, ""); // stack: ArrayReference m_context << u256(1) << Instruction::DUP2; @@ -978,10 +980,8 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) solAssert(function.parameterTypes().size() == 1, ""); solAssert(!!function.parameterTypes()[0], ""); TypePointer paramType = function.parameterTypes()[0]; - ArrayType const* arrayType = - function.kind() == FunctionType::Kind::ArrayPush ? - TypeProvider::array(DataLocation::Storage, paramType) : - TypeProvider::bytesStorage(); + ArrayType const* arrayType = dynamic_cast(function.selfType()); + solAssert(arrayType, ""); // stack: ArrayReference arguments[0]->accept(*this); @@ -1004,7 +1004,7 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) utils().moveToStackTop(1 + type->sizeOnStack()); utils().moveToStackTop(1 + type->sizeOnStack()); // stack: argValue storageSlot slotOffset - if (function.kind() == FunctionType::Kind::ArrayPush) + if (!arrayType->isByteArray()) StorageItem(m_context, *paramType).storeValue(*type, _functionCall.location(), true); else StorageByteArrayElement(m_context).storeValue(*type, _functionCall.location(), true); @@ -1014,14 +1014,11 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) case FunctionType::Kind::ArrayPop: { _functionCall.expression().accept(*this); + solAssert(function.bound(), ""); solAssert(function.parameterTypes().empty(), ""); - - ArrayType const& arrayType = dynamic_cast( - *dynamic_cast(_functionCall.expression()).expression().annotation().type - ); - solAssert(arrayType.dataStoredIn(DataLocation::Storage), ""); - - ArrayUtils(m_context).popStorageArrayElement(arrayType); + ArrayType const* arrayType = dynamic_cast(function.selfType()); + solAssert(arrayType && arrayType->dataStoredIn(DataLocation::Storage), ""); + ArrayUtils(m_context).popStorageArrayElement(*arrayType); break; } case FunctionType::Kind::ObjectCreation: @@ -1325,6 +1322,12 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess) utils().pushCombinedFunctionEntryLabel(funDef); utils().moveIntoStack(funType->selfType()->sizeOnStack(), 1); } + else if ( + funType->kind() == FunctionType::Kind::ArrayPop || + funType->kind() == FunctionType::Kind::ArrayPush + ) + { + } else { solAssert(funType->kind() == FunctionType::Kind::DelegateCall, ""); diff --git a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp index d22112934..691724e1b 100644 --- a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp +++ b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp @@ -1276,30 +1276,31 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall) } case FunctionType::Kind::ArrayPop: { - auto const& memberAccessExpression = dynamic_cast(_functionCall.expression()).expression(); - ArrayType const& arrayType = dynamic_cast(*memberAccessExpression.annotation().type); + solAssert(functionType->bound(), ""); + solAssert(functionType->parameterTypes().empty(), ""); + ArrayType const* arrayType = dynamic_cast(functionType->selfType()); + solAssert(arrayType, ""); define(_functionCall) << - m_utils.storageArrayPopFunction(arrayType) << + m_utils.storageArrayPopFunction(*arrayType) << "(" << IRVariable(_functionCall.expression()).commaSeparatedList() << ")\n"; break; } - case FunctionType::Kind::ByteArrayPush: case FunctionType::Kind::ArrayPush: { - auto const& memberAccessExpression = dynamic_cast(_functionCall.expression()).expression(); - ArrayType const& arrayType = dynamic_cast(*memberAccessExpression.annotation().type); + ArrayType const* arrayType = dynamic_cast(functionType->selfType()); + solAssert(arrayType, ""); if (arguments.empty()) { auto slotName = m_context.newYulVariable(); auto offsetName = m_context.newYulVariable(); m_code << "let " << slotName << ", " << offsetName << " := " << - m_utils.storageArrayPushZeroFunction(arrayType) << + m_utils.storageArrayPushZeroFunction(*arrayType) << "(" << IRVariable(_functionCall.expression()).commaSeparatedList() << ")\n"; setLValue(_functionCall, IRLValue{ - *arrayType.baseType(), + *arrayType->baseType(), IRLValue::Storage{ slotName, offsetName, @@ -1309,12 +1310,12 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall) else { IRVariable argument = - arrayType.baseType()->isValueType() ? - convert(*arguments.front(), *arrayType.baseType()) : + arrayType->baseType()->isValueType() ? + convert(*arguments.front(), *arrayType->baseType()) : *arguments.front(); m_code << - m_utils.storageArrayPushFunction(arrayType, &argument.type()) << + m_utils.storageArrayPushFunction(*arrayType, &argument.type()) << "(" << IRVariable(_functionCall.expression()).commaSeparatedList() << (argument.stackSlots().empty() ? "" : (", " + argument.commaSeparatedList())) << @@ -1590,16 +1591,24 @@ void IRGeneratorForStatements::endVisit(MemberAccess const& _memberAccess) }).count(objectCategory) > 0, ""); define(IRVariable(_memberAccess).part("self"), _memberAccess.expression()); - auto const& functionDefinition = dynamic_cast(memberFunctionType->declaration()); solAssert(*_memberAccess.annotation().requiredLookup == VirtualLookup::Static, ""); if (memberFunctionType->kind() == FunctionType::Kind::Internal) { + auto const& functionDefinition = dynamic_cast(memberFunctionType->declaration()); define(IRVariable(_memberAccess).part("functionIdentifier")) << to_string(functionDefinition.id()) << "\n"; if (!_memberAccess.annotation().calledDirectly) m_context.addToInternalDispatch(functionDefinition); } + else if ( + memberFunctionType->kind() == FunctionType::Kind::ArrayPush || + memberFunctionType->kind() == FunctionType::Kind::ArrayPop + ) + { + // Nothing to do. + } else { + auto const& functionDefinition = dynamic_cast(memberFunctionType->declaration()); solAssert(memberFunctionType->kind() == FunctionType::Kind::DelegateCall, ""); auto contract = dynamic_cast(functionDefinition.scope()); solAssert(contract && contract->isLibrary(), ""); diff --git a/libsolidity/formal/SMTEncoder.cpp b/libsolidity/formal/SMTEncoder.cpp index e0c4c2cc1..53936da8b 100644 --- a/libsolidity/formal/SMTEncoder.cpp +++ b/libsolidity/formal/SMTEncoder.cpp @@ -718,7 +718,6 @@ void SMTEncoder::endVisit(FunctionCall const& _funCall) break; } case FunctionType::Kind::ArrayPush: - case FunctionType::Kind::ByteArrayPush: arrayPush(_funCall); break; case FunctionType::Kind::ArrayPop: @@ -1656,8 +1655,6 @@ void SMTEncoder::arrayPushPopAssign(Expression const& _expr, smtutil::Expression else if (auto const* funCall = dynamic_cast(expr)) { FunctionType const& funType = dynamic_cast(*funCall->expression().annotation().type); - // Push cannot occur on an expression that is itself a ByteArrayPush, i.e., bytes.push().push() is not possible. - solAssert(funType.kind() != FunctionType::Kind::ByteArrayPush, ""); if (funType.kind() == FunctionType::Kind::ArrayPush) { auto memberAccess = dynamic_cast(&funCall->expression()); @@ -2649,7 +2646,7 @@ MemberAccess const* SMTEncoder::isEmptyPush(Expression const& _expr) const ) { auto const& funType = dynamic_cast(*funCall->expression().annotation().type); - if (funType.kind() == FunctionType::Kind::ArrayPush || funType.kind() == FunctionType::Kind::ByteArrayPush) + if (funType.kind() == FunctionType::Kind::ArrayPush) return &dynamic_cast(funCall->expression()); } return nullptr; diff --git a/test/libsolidity/semanticTests/array/pop/parenthesized.sol b/test/libsolidity/semanticTests/array/pop/parenthesized.sol new file mode 100644 index 000000000..04ac0c3e3 --- /dev/null +++ b/test/libsolidity/semanticTests/array/pop/parenthesized.sol @@ -0,0 +1,12 @@ +contract C { + int[] data; + function f() public returns (uint) { + data.push(1); + (data.pop)(); + return data.length; + } +} +// ==== +// compileViaYul: also +// ---- +// f() -> 0