Merge pull request #7842 from ethereum/fixPushWithArg

Fix Yul IR push with and without argument

libsolidity/codegen/YulUtilFunctions.cpp

@@ -633,11 +633,10 @@ string YulUtilFunctions::storageArrayPushFunction(ArrayType const& _type)
 	string functionName = "array_push_" + _type.identifier();
 	return m_functionCollector->createFunction(functionName, [&]() {
 		return Whiskers(R"(
-			function <functionName>(array, value) -> newLen {
+			function <functionName>(array, value) {
 				let oldLen := <fetchLength>(array)
 				if iszero(lt(oldLen, <maxArrayLength>)) { invalid() }
-				newLen := add(oldLen, 1)
-				sstore(array, newLen)
+				sstore(array, add(oldLen, 1))
 
 				let slot, offset := <indexAccess>(array, oldLen)
 				<storeValue>(slot, offset, value)
@@ -651,6 +650,33 @@ string YulUtilFunctions::storageArrayPushFunction(ArrayType const& _type)
 	});
 }
 
+string YulUtilFunctions::storageArrayPushZeroFunction(ArrayType const& _type)
+{
+	solAssert(_type.location() == DataLocation::Storage, "");
+	solAssert(_type.isDynamicallySized(), "");
+	solUnimplementedAssert(!_type.isByteArray(), "Byte Arrays not yet implemented!");
+	solUnimplementedAssert(_type.baseType()->storageBytes() <= 32, "Base type is not yet implemented.");
+
+	string functionName = "array_push_zero_" + _type.identifier();
+	return m_functionCollector->createFunction(functionName, [&]() {
+		return Whiskers(R"(
+			function <functionName>(array) -> slot, offset {
+				let oldLen := <fetchLength>(array)
+				if iszero(lt(oldLen, <maxArrayLength>)) { invalid() }
+				sstore(array, add(oldLen, 1))
+				slot, offset := <indexAccess>(array, oldLen)
+				<storeValue>(slot, offset, <zeroValueFunction>())
+			})")
+			("functionName", functionName)
+			("fetchLength", arrayLengthFunction(_type))
+			("indexAccess", storageArrayIndexAccessFunction(_type))
+			("storeValue", updateStorageValueFunction(*_type.baseType()))
+			("maxArrayLength", (u256(1) << 64).str())
+			("zeroValueFunction", zeroValueFunction(*_type.baseType()))
+			.render();
+	});
+}
+
 string YulUtilFunctions::clearStorageRangeFunction(Type const& _type)
 {
 	string functionName = "clear_storage_range_" + _type.identifier();

libsolidity/codegen/YulUtilFunctions.h

@@ -126,9 +126,13 @@ public:
 	std::string storageArrayPopFunction(ArrayType const& _type);
 
 	/// @returns the name of a function that pushes an element to a storage array
-	/// signature: (array, value) -> newlength
+	/// signature: (array, value)
 	std::string storageArrayPushFunction(ArrayType const& _type);
 
+	/// @returns the name of a function that pushes the base type's zero element to a storage array and returns storage slot and offset of the added element.
+	/// signature: (array) -> slot, offset
+	std::string storageArrayPushZeroFunction(ArrayType const& _type);
+
 	/// @returns the name of a function that will clear the storage area given
 	/// by the start and end (exclusive) parameters (slots).
 	/// signature: (start, end)

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

@@ -659,15 +659,28 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall)
 		ArrayType const& arrayType = dynamic_cast<ArrayType const&>(
 			*dynamic_cast<MemberAccess const&>(_functionCall.expression()).expression().annotation().type
 		);
-		defineExpression(_functionCall) <<
-			m_utils.storageArrayPushFunction(arrayType) <<
-			"(" <<
-			m_context.variable(_functionCall.expression()) <<
-			", " << (
-				arguments.empty() ?
-				m_utils.zeroValueFunction(*arrayType.baseType()) + "()" :
-				expressionAsType(*arguments.front(), *arrayType.baseType())
-			) << ")\n";
+		if (arguments.empty())
+		{
+			auto slotName = m_context.newYulVariable();
+			auto offsetName = m_context.newYulVariable();
+			m_code << "let " << slotName << ", " << offsetName << " := " <<
+				m_utils.storageArrayPushZeroFunction(arrayType) <<
+				"(" << m_context.variable(_functionCall.expression()) << ")\n";
+			setLValue(_functionCall, make_unique<IRStorageItem>(
+				m_context.utils(),
+				slotName,
+				offsetName,
+				*arrayType.baseType()
+			));
+		}
+		else
+			m_code <<
+				m_utils.storageArrayPushFunction(arrayType) <<
+				"(" <<
+				m_context.variable(_functionCall.expression()) <<
+				", " <<
+				expressionAsType(*arguments.front(), *arrayType.baseType()) <<
+				")\n";
 		break;
 	}
 	default:

test/libsolidity/semanticTests/viaYul/array_push_return_reference.sol

@@ -0,0 +1,25 @@
+contract C {
+    uint[] storageArray;
+    function test(uint256 v) public {
+        storageArray.push() = v;
+    }
+    function getLength() public view returns (uint256) {
+        return storageArray.length;
+    }
+    function fetch(uint256 a) public view returns (uint256) {
+        return storageArray[a];
+    }
+}
+// ====
+// compileViaYul: also
+// ----
+// getLength() -> 0
+// test(uint256): 42 ->
+// getLength() -> 1
+// fetch(uint256): 0 -> 42
+// fetch(uint256): 1 -> FAILURE
+// test(uint256): 23 ->
+// getLength() -> 2
+// fetch(uint256): 0 -> 42
+// fetch(uint256): 1 -> 23
+// fetch(uint256): 2 -> FAILURE

test/libsolidity/semanticTests/viaYul/array_push_with_arg.sol

@@ -0,0 +1,25 @@
+contract C {
+    uint[] storageArray;
+    function test(uint256 v) public {
+        storageArray.push(v);
+    }
+    function getLength() public view returns (uint256) {
+        return storageArray.length;
+    }
+    function fetch(uint256 a) public view returns (uint256) {
+        return storageArray[a];
+    }
+}
+// ====
+// compileViaYul: also
+// ----
+// getLength() -> 0
+// test(uint256): 42 ->
+// getLength() -> 1
+// fetch(uint256): 0 -> 42
+// fetch(uint256): 1 -> FAILURE
+// test(uint256): 23 ->
+// getLength() -> 2
+// fetch(uint256): 0 -> 42
+// fetch(uint256): 1 -> 23
+// fetch(uint256): 2 -> FAILURE

test/libyul/yulOptimizerTests/fullSuite/clear_after_if_continue.yul

@@ -12,7 +12,7 @@
 // {
 //     {
 //         let y := mload(0x20)
-//         for { } iszero(iszero(and(y, 8))) { if y { revert(0, 0) } }
+//         for { } and(y, 8) { if y { revert(0, 0) } }
 //         {
 //             if y { continue }
 //             sstore(1, 0)