Merge pull request #4056 from ethereum/safetupleassignment

Deprecate wildcard assignments.

14 files changed, 170 insertions, 89 deletions

diff --git a/Changelog.md b/Changelog.md
index 7c4ac925..37e0b0a1 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -6,6 +6,7 @@ Features:
  * Optimizer: Remove unnecessary masking of the result of known short instructions (``ADDRESS``, ``CALLER``, ``ORIGIN`` and ``COINBASE``).
  * Type Checker: Deprecate the ``years`` unit denomination and raise a warning for it (or an error as experimental 0.5.0 feature).
  * Type Checker: Make literals (without explicit type casting) an error for tight packing as experimental 0.5.0 feature.
+ * Type Checker: Warn about wildcard tuple assignments (this will turn into an error with version 0.5.0).
 
 Bugfixes:
  * Type Checker: Show proper error when trying to ``emit`` a non-event.
diff --git a/docs/control-structures.rst b/docs/control-structures.rst
index f3c351dd..f18e1e10 100644
--- a/docs/control-structures.rst
+++ b/docs/control-structures.rst
@@ -293,12 +293,7 @@ Solidity internally allows tuple types, i.e. a list of objects of potentially di
             // Common trick to swap values -- does not work for non-value storage types.
             (x, y) = (y, x);
             // Components can be left out (also for variable declarations).
-            // If the tuple ends in an empty component,
-            // the rest of the values are discarded.
-            (data.length,) = f(); // Sets the length to 7
-            // The same can be done on the left side.
-            // If the tuple begins in an empty component, the beginning values are discarded.
-            (,data[3]) = f(); // Sets data[3] to 2
+            (data.length,,) = f(); // Sets the length to 7
             // Components can only be left out at the left-hand-side of assignments, with
             // one exception:
             (x,) = (1,);
@@ -307,6 +302,11 @@ Solidity internally allows tuple types, i.e. a list of objects of potentially di
         }
     }
 
+.. note::
+    Prior to version 0.4.24 it was possible to assign to tuples of smaller size, either
+    filling up on the left or on the right side (which ever was empty). This is
+    now deprecated, both sides have to have the same number of components.
+
 Complications for Arrays and Structs
 ------------------------------------
 
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index 32cf1b18..a222bdf0 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/libsolidity/analysis/TypeChecker.cpp
@@ -1076,6 +1076,7 @@ void TypeChecker::endVisit(EmitStatement const& _emit)
 
 bool TypeChecker::visit(VariableDeclarationStatement const& _statement)
 {
+	bool const v050 = m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);
 	if (!_statement.initialValue())
 	{
 		// No initial value is only permitted for single variables with specified type.
@@ -1092,7 +1093,7 @@ bool TypeChecker::visit(VariableDeclarationStatement const& _statement)
 				if (varDecl.referenceLocation() == VariableDeclaration::Location::Default)
 					errorText += " Did you mean '<type> memory " + varDecl.name() + "'?";
 				solAssert(m_scope, "");
-				if (m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050))
+				if (v050)
 					m_errorReporter.declarationError(varDecl.location(), errorText);
 				else
 					m_errorReporter.warning(varDecl.location(), errorText);
@@ -1132,12 +1133,33 @@ bool TypeChecker::visit(VariableDeclarationStatement const& _statement)
 				") in value for variable assignment (0) needed"
 			);
 	}
-	else if (valueTypes.size() != variables.size() && !variables.front() && !variables.back())
-		m_errorReporter.fatalTypeError(
-			_statement.location(),
-			"Wildcard both at beginning and end of variable declaration list is only allowed "
-			"if the number of components is equal."
-		);
+	else if (valueTypes.size() != variables.size())
+	{
+		if (v050)
+			m_errorReporter.fatalTypeError(
+				_statement.location(),
+				"Different number of components on the left hand side (" +
+				toString(variables.size()) +
+				") than on the right hand side (" +
+				toString(valueTypes.size()) +
+				")."
+			);
+		else if (!variables.front() && !variables.back())
+			m_errorReporter.fatalTypeError(
+				_statement.location(),
+				"Wildcard both at beginning and end of variable declaration list is only allowed "
+				"if the number of components is equal."
+			);
+		else
+			m_errorReporter.warning(
+				_statement.location(),
+				"Different number of components on the left hand side (" +
+				toString(variables.size()) +
+				") than on the right hand side (" +
+				toString(valueTypes.size()) +
+				")."
+			);
+	}
 	size_t minNumValues = variables.size();
 	if (!variables.empty() && (!variables.back() || !variables.front()))
 		--minNumValues;
@@ -1335,6 +1357,7 @@ bool TypeChecker::visit(Conditional const& _conditional)
 
 bool TypeChecker::visit(Assignment const& _assignment)
 {
+	bool const v050 = m_scope->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);
 	requireLValue(_assignment.leftHandSide());
 	TypePointer t = type(_assignment.leftHandSide());
 	_assignment.annotation().type = t;
@@ -1347,11 +1370,29 @@ 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);
 
 		// expectType does not cause fatal errors, so we have to check again here.
-		if (dynamic_cast<TupleType const*>(type(_assignment.rightHandSide()).get()))
+		if (TupleType const* rhsType = dynamic_cast<TupleType const*>(type(_assignment.rightHandSide()).get()))
+		{
 			checkDoubleStorageAssignment(_assignment);
+			// @todo For 0.5.0, this code shoud move to TupleType::isImplicitlyConvertibleTo,
+			// but we cannot do it right now.
+			if (rhsType->components().size() != tupleType->components().size())
+			{
+				string message =
+					"Different number of components on the left hand side (" +
+					toString(tupleType->components().size()) +
+					") than on the right hand side (" +
+					toString(rhsType->components().size()) +
+					").";
+				if (v050)
+					m_errorReporter.typeError(_assignment.location(), message);
+				else
+					m_errorReporter.warning(_assignment.location(), message);
+			}
+		}
 	}
 	else if (t->category() == Type::Category::Mapping)
 	{
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index a2540302..91fd1fff 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -2894,7 +2894,7 @@ BOOST_AUTO_TEST_CASE(dynamic_return_types_not_possible)
 		contract C {
 			function f(uint) public returns (string);
 			function g() public {
-				var (x,) = this.f(2);
+				var x = this.f(2);
 				// we can assign to x but it is not usable.
 				bytes(x).length;
 			}
@@ -5833,80 +5833,6 @@ BOOST_AUTO_TEST_CASE(pure_statement_check_for_regular_for_loop)
 	CHECK_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() public {
-				(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() public {
-				(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() public {
-				(,x, y) = (1, 2, y, x);
-			}
-		}
-	)";
-	CHECK_WARNING(text, "This assignment performs two copies to storage.");
-}
-
-BOOST_AUTO_TEST_CASE(nowarn_swap_memory)
-{
-	char const* text = R"(
-		contract C {
-			struct S { uint a; uint b; }
-			function f() pure public {
-				S memory x;
-				S memory y;
-				(x, y) = (y, x);
-			}
-		}
-	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);
-}
-
-BOOST_AUTO_TEST_CASE(nowarn_swap_storage_pointers)
-{
-	char const* text = R"(
-		contract C {
-			struct S { uint a; uint b; }
-			S x; S y;
-			function f() public {
-				S storage x_local = x;
-				S storage y_local = y;
-				S storage z_local = x;
-				(x, y_local, x_local, z_local) = (y, x_local, y_local, y);
-			}
-		}
-	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);
-}
-
 BOOST_AUTO_TEST_CASE(warn_unused_local)
 {
 	char const* text = R"(
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol b/test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol
new file mode 100644
index 00000000..5b7f870b
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/error_fill.sol
@@ -0,0 +1,12 @@
+pragma experimental "v0.5.0";
+contract C {
+	function f() public pure returns (uint, uint, bytes32) {
+		uint a;
+		bytes32 b;
+		(a,) = f();
+		(,b) = f();
+	}
+}
+// ----
+// TypeError: (126-136): Different number of components on the left hand side (2) than on the right hand side (3).
+// TypeError: (140-150): Different number of components on the left hand side (2) than on the right hand side (3).
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol b/test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol
new file mode 100644
index 00000000..bbf21d7e
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/large_component_count.sol
@@ -0,0 +1,24 @@
+pragma experimental "v0.5.0";
+contract C {
+	function g() public pure returns (
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint,
+		uint
+	) { }
+	function f() public pure returns (uint, uint, bytes32) {
+		uint a;
+		uint b;
+		(,,,,a,,,,b,,,,) = g();
+	}
+}
+// ----
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol
new file mode 100644
index 00000000..3262781b
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_explicit_singleton_token_expression.sol
@@ -0,0 +1,8 @@
+contract C {
+	function f() public pure {
+		uint a;
+		(a,) = (uint(1),);
+	}
+}
+// ----
+// Warning: (53-70): Different number of components on the left hand side (2) than on the right hand side (1).
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol
new file mode 100644
index 00000000..b20bbf90
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_memory.sol
@@ -0,0 +1,8 @@
+contract C {
+	struct S { uint a; uint b; }
+	function f() pure public {
+		S memory x;
+		S memory y;
+		(x, y) = (y, x);
+	}
+}
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol
new file mode 100644
index 00000000..5f7a18f7
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/nowarn_swap_storage_pointers.sol
@@ -0,0 +1,10 @@
+		contract C {
+			struct S { uint a; uint b; }
+			S x; S y;
+			function f() public {
+				S storage x_local = x;
+				S storage y_local = y;
+				S storage z_local = x;
+				(x, y_local, x_local, z_local) = (y, x_local, y_local, y);
+			}
+		}
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol
new file mode 100644
index 00000000..a079a509
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_assignment.sol
@@ -0,0 +1,11 @@
+contract C {
+	function f() public pure returns (uint, uint, bytes32) {
+		uint a;
+		bytes32 b;
+		(a,) = f();
+		(,b) = f();
+	}
+}
+// ----
+// Warning: (96-106): Different number of components on the left hand side (2) than on the right hand side (3).
+// Warning: (110-120): Different number of components on the left hand side (2) than on the right hand side (3).
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol
new file mode 100644
index 00000000..1d243c7c
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_fill_vardecl.sol
@@ -0,0 +1,11 @@
+contract C {
+	function f() public pure returns (uint, uint, uint, uint) {
+		// Can later be replaced by (uint a, uint b,) = f();
+		var (a,b,) = f();
+		a; b;
+	}
+}
+// ----
+// Warning: (136-137): Use of the "var" keyword is deprecated.
+// Warning: (138-139): Use of the "var" keyword is deprecated.
+// Warning: (131-147): Different number of components on the left hand side (3) than on the right hand side (4).
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol
new file mode 100644
index 00000000..e4c3e694
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol
@@ -0,0 +1,9 @@
+contract C {
+	struct S { uint a; uint b; }
+	S x; S y;
+	function f() public {
+		(x, y) = (y, x);
+	}
+}
+// ----
+// Warning: (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.
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol
new file mode 100644
index 00000000..b2979804
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_left.sol
@@ -0,0 +1,10 @@
+contract C {
+	struct S { uint a; uint b; }
+	S x; S y;
+	function f() public {
+		(,x, y) = (1, 2, y, x);
+	}
+}
+// ----
+// Warning: (79-101): 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: (79-101): Different number of components on the left hand side (3) than on the right hand side (4).
diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol
new file mode 100644
index 00000000..aa35d7d4
--- /dev/null
+++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies_fill_right.sol
@@ -0,0 +1,10 @@
+contract C {
+	struct S { uint a; uint b; }
+	S x; S y;
+	function f() public {
+		(x, y, ) = (y, x, 1, 2);
+	}
+}
+// ----
+// Warning: (79-102): 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: (79-102): Different number of components on the left hand side (3) than on the right hand side (4).