Merge pull request #10896 from ethereum/issue-10870

Fix issue with pop on storage array.

Changelog.md

@@ -20,6 +20,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)
 

libsolidity/analysis/TypeChecker.cpp

@@ -2345,10 +2345,7 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
 			*_functionCall.expression().annotation().isPure &&
 			functionType->isPure();
 
-		if (
+		if (functionType->kind() == FunctionType::Kind::ArrayPush)
-			functionType->kind() == FunctionType::Kind::ArrayPush ||
-			functionType->kind() == FunctionType::Kind::ByteArrayPush
-		)
 			isLValue = functionType->parameterTypes().empty();
 
 		break;

libsolidity/ast/ASTAnnotations.h

@@ -47,6 +47,7 @@ namespace solidity::frontend
 
 class Type;
 using TypePointer = Type const*;
+class ArrayType;
 using namespace util;
 
 struct CallGraph;

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{string()},
-				strings{},
 				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<tuple<string, TypePointer>> FunctionType::makeStackItems() const
 		break;
 	case Kind::ArrayPush:
 	case Kind::ArrayPop:
-	case Kind::ByteArrayPush:
+		solAssert(bound(), "");
-		slots = {make_tuple("slot", TypeProvider::uint256())};
+		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<FunctionDefinition const*>(m_declaration);
-	solAssert(fun && fun->libraryFunction(), "");
 	solAssert(!m_gasSet, "");
 	solAssert(!m_valueSet, "");
 	solAssert(!m_saltSet, "");

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()

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(
-			solAssert(function.kind() == FunctionType::Kind::DelegateCall || function.kind() == FunctionType::Kind::Internal, "");
+				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 =
+				ArrayType const* arrayType = dynamic_cast<ArrayType const*>(function.selfType());
-					function.kind() == FunctionType::Kind::ArrayPush ?
+				solAssert(arrayType, "");
-					TypeProvider::array(DataLocation::Storage, paramType) :
-					TypeProvider::bytesStorage();
 
 				// 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 =
+				ArrayType const* arrayType = dynamic_cast<ArrayType const*>(function.selfType());
-					function.kind() == FunctionType::Kind::ArrayPush ?
+				solAssert(arrayType, "");
-					TypeProvider::array(DataLocation::Storage, paramType) :
-					TypeProvider::bytesStorage();
 
 				// 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<ArrayType const*>(function.selfType());
-			ArrayType const& arrayType = dynamic_cast<ArrayType const&>(
+			solAssert(arrayType && arrayType->dataStoredIn(DataLocation::Storage), "");
-				*dynamic_cast<MemberAccess const&>(_functionCall.expression()).expression().annotation().type
+			ArrayUtils(m_context).popStorageArrayElement(*arrayType);
-			);
-			solAssert(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, "");

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

@@ -1276,30 +1276,31 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall)
 	}
 	case FunctionType::Kind::ArrayPop:
 	{
-		auto const& memberAccessExpression = dynamic_cast<MemberAccess const&>(_functionCall.expression()).expression();
+		solAssert(functionType->bound(), "");
-		ArrayType const& arrayType = dynamic_cast<ArrayType const&>(*memberAccessExpression.annotation().type);
+		solAssert(functionType->parameterTypes().empty(), "");
+		ArrayType const* arrayType = dynamic_cast<ArrayType const*>(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<MemberAccess const&>(_functionCall.expression()).expression();
+		ArrayType const* arrayType = dynamic_cast<ArrayType const*>(functionType->selfType());
-		ArrayType const& arrayType = dynamic_cast<ArrayType const&>(*memberAccessExpression.annotation().type);
+		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() ?
+				arrayType->baseType()->isValueType() ?
-				convert(*arguments.front(), *arrayType.baseType()) :
+				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<FunctionDefinition const&>(memberFunctionType->declaration());
 		solAssert(*_memberAccess.annotation().requiredLookup == VirtualLookup::Static, "");
 		if (memberFunctionType->kind() == FunctionType::Kind::Internal)
 		{
+			auto const& functionDefinition = dynamic_cast<FunctionDefinition const&>(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<FunctionDefinition const&>(memberFunctionType->declaration());
 			solAssert(memberFunctionType->kind() == FunctionType::Kind::DelegateCall, "");
 			auto contract = dynamic_cast<ContractDefinition const*>(functionDefinition.scope());
 			solAssert(contract && contract->isLibrary(), "");

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<FunctionCall const*>(expr))
 	{
 		FunctionType const& funType = dynamic_cast<FunctionType const&>(*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<MemberAccess const*>(&funCall->expression());
@@ -2649,7 +2646,7 @@ MemberAccess const* SMTEncoder::isEmptyPush(Expression const& _expr) const
 	)
 	{
 		auto const& funType = dynamic_cast<FunctionType const&>(*funCall->expression().annotation().type);
-		if (funType.kind() == FunctionType::Kind::ArrayPush || funType.kind() == FunctionType::Kind::ByteArrayPush)
+		if (funType.kind() == FunctionType::Kind::ArrayPush)
 			return &dynamic_cast<MemberAccess const&>(funCall->expression());
 	}
 	return nullptr;

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