Warn about multiple assignments to storage byte pushes and fix warnings about multiple storage to storage copies.

Co-authored-by: Kamil Śliwak <kamil.sliwak@codepoets.it>

Changelog.md

@@ -11,6 +11,7 @@ Language Features:
 
 Compiler Features:
  * LSP: Add rudimentary support for semantic highlighting.
+ * Type Checker: Warn about assignments involving multiple pushes to storage ``bytes`` that may invalidate references.
  * Yul Optimizer: Improve inlining heuristics for via IR code generation and pure Yul compilation.
 
 

libsolidity/analysis/ControlFlowBuilder.cpp

@@ -618,7 +618,7 @@ bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDecl
 						solAssert(tupleExpression->components().size() > i, "");
 						expression = tupleExpression->components()[i].get();
 					}
-				expression = resolveUnaryTuples(expression);
+				expression = resolveOuterUnaryTuples(expression);
 				m_currentNode->variableOccurrences.emplace_back(
 					*var,
 					VariableOccurrence::Kind::Assignment,

libsolidity/analysis/TypeChecker.cpp

@@ -40,9 +40,10 @@
 #include <boost/algorithm/string/join.hpp>
 #include <boost/algorithm/string/predicate.hpp>
 
-#include <range/v3/view/zip.hpp>
-#include <range/v3/view/drop_exactly.hpp>
 #include <range/v3/algorithm/count_if.hpp>
+#include <range/v3/view/drop_exactly.hpp>
+#include <range/v3/view/enumerate.hpp>
+#include <range/v3/view/zip.hpp>
 
 #include <memory>
 #include <vector>
@@ -105,26 +106,71 @@ bool TypeChecker::visit(ContractDefinition const& _contract)
 
 void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment)
 {
-	TupleType const& lhs = dynamic_cast<TupleType const&>(*type(_assignment.leftHandSide()));
-	TupleType const& rhs = dynamic_cast<TupleType const&>(*type(_assignment.rightHandSide()));
-
-	if (lhs.components().size() != rhs.components().size())
-	{
-		solAssert(m_errorReporter.hasErrors(), "");
-		return;
-	}
-
 	size_t storageToStorageCopies = 0;
 	size_t toStorageCopies = 0;
-	for (size_t i = 0; i < lhs.components().size(); ++i)
-	{
-		ReferenceType const* ref = dynamic_cast<ReferenceType const*>(lhs.components()[i]);
-		if (!ref || !ref->dataStoredIn(DataLocation::Storage) || ref->isPointer())
-			continue;
-		toStorageCopies++;
-		if (rhs.components()[i]->dataStoredIn(DataLocation::Storage))
-			storageToStorageCopies++;
-	}
+	size_t storageByteArrayPushes = 0;
+	size_t storageByteAccesses = 0;
+	auto count = [&](TupleExpression const& _lhs, TupleType const& _rhs, auto _recurse) -> void {
+		TupleType const& lhsType = dynamic_cast<TupleType const&>(*type(_lhs));
+
+		if (lhsType.components().size() != _rhs.components().size())
+		{
+			solAssert(m_errorReporter.hasErrors(), "");
+			return;
+		}
+
+		for (auto&& [index, componentType]: lhsType.components() | ranges::views::enumerate)
+		{
+			if (ReferenceType const* ref = dynamic_cast<ReferenceType const*>(componentType))
+			{
+				if (ref && ref->dataStoredIn(DataLocation::Storage) && !ref->isPointer())
+				{
+					toStorageCopies++;
+					if (_rhs.components()[index]->dataStoredIn(DataLocation::Storage))
+						storageToStorageCopies++;
+				}
+			}
+			else if (FixedBytesType const* bytesType = dynamic_cast<FixedBytesType const*>(componentType))
+			{
+				if (bytesType && bytesType->numBytes() == 1)
+				{
+					if (FunctionCall const* lhsCall = dynamic_cast<FunctionCall const*>(resolveOuterUnaryTuples(_lhs.components().at(index).get())))
+					{
+						FunctionType const& callType = dynamic_cast<FunctionType const&>(*type(lhsCall->expression()));
+						if (callType.kind() == FunctionType::Kind::ArrayPush)
+						{
+							ArrayType const& arrayType = dynamic_cast<ArrayType const&>(*callType.selfType());
+							if (arrayType.isByteArray() && arrayType.dataStoredIn(DataLocation::Storage))
+							{
+								++storageByteAccesses;
+								++storageByteArrayPushes;
+							}
+						}
+					}
+					else if (IndexAccess const* indexAccess = dynamic_cast<IndexAccess const*>(resolveOuterUnaryTuples(_lhs.components().at(index).get())))
+					{
+						if (ArrayType const* arrayType = dynamic_cast<ArrayType const*>(type(indexAccess->baseExpression())))
+							if (arrayType->isByteArray() && arrayType->dataStoredIn(DataLocation::Storage))
+								++storageByteAccesses;
+					}
+				}
+			}
+			else if (TupleType const* tupleType = dynamic_cast<TupleType const*>(componentType))
+				if (auto const* lhsNested = dynamic_cast<TupleExpression const*>(_lhs.components().at(index).get()))
+					if (auto const* rhsNestedType = dynamic_cast<TupleType const*>(_rhs.components().at(index)))
+						_recurse(
+							*lhsNested,
+							*rhsNestedType,
+							_recurse
+						);
+		}
+	};
+	count(
+		dynamic_cast<TupleExpression const&>(_assignment.leftHandSide()),
+		dynamic_cast<TupleType const&>(*type(_assignment.rightHandSide())),
+		count
+	);
+
 	if (storageToStorageCopies >= 1 && toStorageCopies >= 2)
 		m_errorReporter.warning(
 			7238_error,
@@ -134,6 +180,16 @@ void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment)
 			"is executed and thus may have unexpected effects. It is safer to perform the copies "
 			"separately or assign to storage pointers first."
 		);
+
+	if (storageByteArrayPushes >= 1 && storageByteAccesses >= 2)
+		m_errorReporter.warning(
+			7239_error,
+			_assignment.location(),
+			"This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. "
+			"When a bytes array is enlarged, it may transition from short storage layout to long storage layout, "
+			"which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single "
+			"operation, one element at a time."
+		);
 }
 
 TypePointers TypeChecker::typeCheckABIDecodeAndRetrieveReturnType(FunctionCall const& _functionCall, bool _abiEncoderV2)

libsolidity/ast/ASTUtils.cpp

@@ -85,7 +85,7 @@ VariableDeclaration const* rootConstVariableDeclaration(VariableDeclaration cons
 	return rootDecl;
 }
 
-Expression const* resolveUnaryTuples(Expression const* _expr)
+Expression const* resolveOuterUnaryTuples(Expression const* _expr)
 {
 	while (auto const* tupleExpression = dynamic_cast<TupleExpression const*>(_expr))
 		if (tupleExpression->components().size() == 1)

libsolidity/ast/ASTUtils.h

@@ -38,8 +38,8 @@ bool isConstantVariableRecursive(VariableDeclaration const& _varDecl);
 /// Returns the innermost AST node that covers the given location or nullptr if not found.
 ASTNode const* locateInnermostASTNode(int _offsetInFile, SourceUnit const& _sourceUnit);
 
-/// @returns @a _expr itself, in case it is not an unary tuple expression. Otherwise it descends recursively
+/// @returns @a _expr itself, in case it is not a unary tuple expression. Otherwise it descends recursively
 /// into unary tuples and returns the contained expression.
-Expression const* resolveUnaryTuples(Expression const* _expr);
+Expression const* resolveOuterUnaryTuples(Expression const* _expr);
 
 }

test/libsolidity/syntaxTests/array/bytes1_array_push_assign_multi.sol

@@ -0,0 +1,14 @@
+contract C {
+    bytes1[] x;
+    bytes1[] z;
+    function f() public {
+        (x.push(), x.push()) = (0, 0);
+        (((x.push())), (x.push())) = (0, 0);
+        ((x.push(), x.push()), x.push()) = ((0, 0), 0);
+        (x.push(), x[0]) = (0, 0);
+        bytes1[] storage y = x;
+        (x.push(), y.push()) = (0, 0);
+        (x.push(), z.push()) = (0, 0);
+    }
+}
+// ----

test/libsolidity/syntaxTests/array/bytes_assign_multi_fine.sol

@@ -0,0 +1,8 @@
+contract C {
+	bytes x;
+	function f() public {
+		(x[0], x[1]) = (0, 0);
+		(x[0], x[1]) = (x[1], x[0]);
+	}
+}
+// ----

test/libsolidity/syntaxTests/array/bytes_push_assign_multi.sol

@@ -0,0 +1,21 @@
+contract C {
+    bytes x;
+    bytes z;
+    function f() public {
+        (x.push(), x.push()) = (0, 0);
+        (((x.push())), (x.push())) = (0, 0);
+        ((x.push(), x.push()), x.push()) = ((0, 0), 0);
+        (x.push(), x[0]) = (0, 0);
+        bytes storage y = x;
+        (x.push(), y.push()) = (0, 0);
+        // The following is a false positive.
+        (x.push(), z.push()) = (0, 0);
+    }
+}
+// ----
+// Warning 7239: (73-102): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time.
+// Warning 7239: (112-147): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time.
+// Warning 7239: (157-203): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time.
+// Warning 7239: (213-238): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time.
+// Warning 7239: (277-306): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time.
+// Warning 7239: (362-391): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time.

test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol

@@ -4,6 +4,11 @@ contract C {
 	function f() public {
 		(x, y) = (y, x);
 	}
+	function g() public {
+		uint z;
+		((x, y), z) = ((y, x), 0);
+	}
 }
 // ----
 // Warning 7238: (79-94): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first.
+// Warning 7238: (134-159): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first.