Warn about copies in storage that might overwrite unexpectedly.

Changelog.md

@@ -11,6 +11,7 @@ Features:
  * Inline Assembly: introduce ``keccak256`` as an opcode. ``sha3`` is still a valid alias.
  * Inline Assembly: ``for`` and ``switch`` statements.
  * Inline Assembly: function definitions and function calls.
+ * Type Checker: Warn about copies in storage that might overwrite unexpectedly.
  * Code Generator: Added the Whiskers template system.
  * Remove obsolete Why3 output.
 

libsolidity/analysis/TypeChecker.cpp

@@ -364,6 +364,35 @@ void TypeChecker::checkLibraryRequirements(ContractDefinition const& _contract)
 			m_errorReporter.typeError(var->location(), "Library cannot have non-constant state variables");
 }
 
+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()));
+
+	bool fillRight = !lhs.components().empty() && (!lhs.components().back() || lhs.components().front());
+	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].get());
+		if (!ref || !ref->dataStoredIn(DataLocation::Storage) || ref->isPointer())
+			continue;
+		size_t rhsPos = fillRight ? i : rhs.components().size() - (lhs.components().size() - i);
+		solAssert(rhsPos < rhs.components().size(), "");
+		toStorageCopies++;
+		if (rhs.components()[rhsPos]->dataStoredIn(DataLocation::Storage))
+			storageToStorageCopies++;
+	}
+	if (storageToStorageCopies >= 1 && toStorageCopies >= 2)
+		m_errorReporter.warning(
+			_assignment.location(),
+			"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."
+		);
+}
+
 void TypeChecker::endVisit(InheritanceSpecifier const& _inheritance)
 {
 	auto base = dynamic_cast<ContractDefinition const*>(&dereference(_inheritance.name()));
@@ -1047,6 +1076,8 @@ bool TypeChecker::visit(Assignment const& _assignment)
 		// Sequenced assignments of tuples is not valid, make the result a "void" type.
 		_assignment.annotation().type = make_shared<TupleType>();
 		expectType(_assignment.rightHandSide(), *tupleType);
+
+		checkDoubleStorageAssignment(_assignment);
 	}
 	else if (t->category() == Type::Category::Mapping)
 	{

libsolidity/analysis/TypeChecker.h

@@ -69,6 +69,9 @@ private:
 	void checkContractExternalTypeClashes(ContractDefinition const& _contract);
 	/// Checks that all requirements for a library are fulfilled if this is a library.
 	void checkLibraryRequirements(ContractDefinition const& _contract);
+	/// Checks (and warns) if a tuple assignment might cause unexpected overwrites in storage.
+	/// Should only be called if the left hand side is tuple-typed.
+	void checkDoubleStorageAssignment(Assignment const& _assignment);
 
 	virtual void endVisit(InheritanceSpecifier const& _inheritance) override;
 	virtual void endVisit(UsingForDirective const& _usingFor) override;

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -5771,6 +5771,48 @@ BOOST_AUTO_TEST_CASE(pure_statement_check_for_regular_for_loop)
 	success(text);
 }
 
+BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies)
+{
+	char const* text = R"(
+		contract C {
+			struct S { uint a; uint b; }
+			S x; S y;
+			function f() {
+				(x, y) = (y, x);
+			}
+		}
+	)";
+	CHECK_WARNING(text, "This assignment performs two copies to storage.");
+}
+
+BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_right)
+{
+	char const* text = R"(
+		contract C {
+			struct S { uint a; uint b; }
+			S x; S y;
+			function f() {
+				(x, y, ) = (y, x, 1, 2);
+			}
+		}
+	)";
+	CHECK_WARNING(text, "This assignment performs two copies to storage.");
+}
+
+BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_left)
+{
+	char const* text = R"(
+		contract C {
+			struct S { uint a; uint b; }
+			S x; S y;
+			function f() {
+				(,x, y) = (1, 2, y, x);
+			}
+		}
+	)";
+	CHECK_WARNING(text, "This assignment performs two copies to storage.");
+}
+
 BOOST_AUTO_TEST_CASE(warn_unused_local)
 {
 	char const* text = R"(