Merge pull request #1334 from ethereum/enum_conversion

check enum value range during conversion

11 files changed, 92 insertions, 22 deletions

diff --git a/Changelog.md b/Changelog.md
index d76303ab..851f39a0 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -3,6 +3,12 @@
 Features:
  * Do-while loops: support for a C-style do{<block>}while(<expr>); control structure
  * Type checker: now more eagerly searches for a common type of an inline array with mixed types
+ * Code generator: generates a runtime error when an out-of-range value is converted into an enum type.
+
+Bugfixes:
+
+ * Parser: disallow empty enum definitions.
+ * Type checker: disallow conversion between different enum types.
 
 ### 0.4.4 (2016-10-31)
 
diff --git a/docs/control-structures.rst b/docs/control-structures.rst
index 51f43015..bbb90e6a 100644
--- a/docs/control-structures.rst
+++ b/docs/control-structures.rst
@@ -329,9 +329,10 @@ Currently, there are situations, where exceptions happen automatically in Solidi
 3. If you call a function via a message call but it does not finish properly (i.e. it runs out of gas, has no matching function, or throws an exception itself), except when a low level operation ``call``, ``send``, ``delegatecall`` or ``callcode`` is used.  The low level operations never throw exceptions but indicate failures by returning ``false``.
 4. If you create a contract using the ``new`` keyword but the contract creation does not finish properly (see above for the definition of "not finish properly").
 5. If you divide or modulo by zero (e.g. ``5 / 0`` or ``23 % 0``).
-6. If you perform an external function call targeting a contract that contains no code.
-7. If your contract receives Ether via a public function without ``payable`` modifier (including the constructor and the fallback function).
-8. If your contract receives Ether via a public accessor function.
+6. If you convert a value too big or negative into an enum type.
+7. If you perform an external function call targeting a contract that contains no code.
+8. If your contract receives Ether via a public function without ``payable`` modifier (including the constructor and the fallback function).
+9. If your contract receives Ether via a public accessor function.
 
 Internally, Solidity performs an "invalid jump" when an exception is thrown and thus causes the EVM to revert all changes made to the state. The reason for this is that there is no safe way to continue execution, because an expected effect did not occur. Because we want to retain the atomicity of transactions, the safest thing to do is to revert all changes and make the whole transaction (or at least call) without effect.
 
diff --git a/docs/types.rst b/docs/types.rst
index 9e7d9b4a..9ec9e526 100644
--- a/docs/types.rst
+++ b/docs/types.rst
@@ -237,7 +237,8 @@ Enums
 =====
 
 Enums are one way to create a user-defined type in Solidity. They are explicitly convertible
-to and from all integer types but implicit conversion is not allowed.
+to and from all integer types but implicit conversion is not allowed.  The explicit conversions
+check the value ranges at runtime and a failure causes an exception.  Enums needs at least one member.
 
 ::
 
diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp
index 0e077b32..f0995393 100644
--- a/libsolidity/ast/Types.cpp
+++ b/libsolidity/ast/Types.cpp
@@ -1561,7 +1561,7 @@ bool EnumType::operator==(Type const& _other) const
 
 unsigned EnumType::storageBytes() const
 {
-	size_t elements = m_enum.members().size();
+	size_t elements = numberOfMembers();
 	if (elements <= 1)
 		return 1;
 	else
@@ -1578,9 +1578,14 @@ string EnumType::canonicalName(bool) const
 	return m_enum.annotation().canonicalName;
 }
 
+size_t EnumType::numberOfMembers() const
+{
+	return m_enum.members().size();
+};
+
 bool EnumType::isExplicitlyConvertibleTo(Type const& _convertTo) const
 {
-	return _convertTo.category() == category() || _convertTo.category() == Category::Integer;
+	return _convertTo == *this || _convertTo.category() == Category::Integer;
 }
 
 unsigned EnumType::memberValue(ASTString const& _member) const
diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h
index 3f94d11a..082e16a6 100644
--- a/libsolidity/ast/Types.h
+++ b/libsolidity/ast/Types.h
@@ -738,6 +738,7 @@ public:
 	EnumDefinition const& enumDefinition() const { return m_enum; }
 	/// @returns the value that the string has in the Enum
 	unsigned int memberValue(ASTString const& _member) const;
+	size_t numberOfMembers() const;
 
 private:
 	EnumDefinition const& m_enum;
diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp
index e064c1a6..dd133aea 100644
--- a/libsolidity/codegen/CompilerUtils.cpp
+++ b/libsolidity/codegen/CompilerUtils.cpp
@@ -315,6 +315,8 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 	Type::Category stackTypeCategory = _typeOnStack.category();
 	Type::Category targetTypeCategory = _targetType.category();
 
+	bool enumOverflowCheckPending = (targetTypeCategory == Type::Category::Enum);
+
 	switch (stackTypeCategory)
 	{
 	case Type::Category::FixedBytes:
@@ -348,7 +350,15 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 	}
 		break;
 	case Type::Category::Enum:
-		solAssert(targetTypeCategory == Type::Category::Integer || targetTypeCategory == Type::Category::Enum, "");
+		solAssert(_targetType == _typeOnStack || targetTypeCategory == Type::Category::Integer, "");
+		if (enumOverflowCheckPending)
+		{
+			EnumType const& enumType = dynamic_cast<decltype(enumType)>(_targetType);
+			solAssert(enumType.numberOfMembers() > 0, "empty enum should have caused a parser error.");
+			m_context << u256(enumType.numberOfMembers() - 1) << Instruction::DUP2 << Instruction::GT;
+			m_context.appendConditionalJumpTo(m_context.errorTag());
+			enumOverflowCheckPending = false;
+		}
 		break;
 	case Type::Category::FixedPoint:
 		solAssert(false, "Not yet implemented - FixedPointType.");
@@ -372,6 +382,11 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 			solAssert(_typeOnStack.mobileType(), "");
 			// just clean
 			convertType(_typeOnStack, *_typeOnStack.mobileType(), true);
+			EnumType const& enumType = dynamic_cast<decltype(enumType)>(_targetType);
+			solAssert(enumType.numberOfMembers() > 0, "empty enum should have caused a parser error.");
+			m_context << u256(enumType.numberOfMembers() - 1) << Instruction::DUP2 << Instruction::GT;
+			m_context.appendConditionalJumpTo(m_context.errorTag());
+			enumOverflowCheckPending = false;
 		}
 		else if (targetTypeCategory == Type::Category::FixedPoint)
 		{
@@ -656,6 +671,8 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 		solAssert(_typeOnStack == _targetType, "Invalid type conversion requested.");
 		break;
 	}
+
+	solAssert(!enumOverflowCheckPending, "enum overflow checking missing.");
 }
 
 void CompilerUtils::pushZeroValue(Type const& _type)
diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp
index 52b53619..df3ed7b2 100644
--- a/libsolidity/parsing/Parser.cpp
+++ b/libsolidity/parsing/Parser.cpp
@@ -406,6 +406,8 @@ ASTPointer<EnumDefinition> Parser::parseEnumDefinition()
 		if (m_scanner->currentToken() != Token::Identifier)
 			fatalParserError(string("Expected Identifier after ','"));
 	}
+	if (members.size() == 0)
+		parserError({"enum with no members is not allowed."});
 
 	nodeFactory.markEndPosition();
 	expectToken(Token::RBrace);
diff --git a/test/libsolidity/ASTJSON.cpp b/test/libsolidity/ASTJSON.cpp
index a0fc5dd7..6c062ee8 100644
--- a/test/libsolidity/ASTJSON.cpp
+++ b/test/libsolidity/ASTJSON.cpp
@@ -94,20 +94,6 @@ BOOST_AUTO_TEST_CASE(using_for_directive)
 	BOOST_CHECK_EQUAL(usingFor["children"][1]["attributes"]["name"], "uint");    
 }
 
-BOOST_AUTO_TEST_CASE(enum_definition)
-{
-	CompilerStack c;
-	c.addSource("a", "contract C { enum E {} }");
-	c.parse();
-	map<string, unsigned> sourceIndices;
-	sourceIndices["a"] = 1;
-	Json::Value astJson = ASTJsonConverter(c.ast("a"), sourceIndices).json();
-	Json::Value enumDefinition = astJson["children"][0]["children"][0];
-	BOOST_CHECK_EQUAL(enumDefinition["name"], "EnumDefinition");
-	BOOST_CHECK_EQUAL(enumDefinition["attributes"]["name"], "E");
-	BOOST_CHECK_EQUAL(enumDefinition["src"], "13:9:1");
-}
-
 BOOST_AUTO_TEST_CASE(enum_value)
 {
 	CompilerStack c;
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index bab54fab..5582e4a1 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -3342,6 +3342,42 @@ BOOST_AUTO_TEST_CASE(using_enums)
 	BOOST_CHECK(callContractFunction("getChoice()") == encodeArgs(2));
 }
 
+BOOST_AUTO_TEST_CASE(enum_explicit_overflow)
+{
+	char const* sourceCode = R"(
+			contract test {
+				enum ActionChoices { GoLeft, GoRight, GoStraight }
+				function test()
+				{
+				}
+				function getChoiceExp(uint x) returns (uint d)
+				{
+					choice = ActionChoices(x);
+					d = uint256(choice);
+				}
+				function getChoiceFromSigned(int x) returns (uint d)
+				{
+					choice = ActionChoices(x);
+					d = uint256(choice);
+				}
+				function getChoiceFromNegativeLiteral() returns (uint d)
+				{
+					choice = ActionChoices(-1);
+					d = uint256(choice);
+				}
+				ActionChoices choice;
+			}
+	)";
+	compileAndRun(sourceCode);
+	// These should throw
+	BOOST_CHECK(callContractFunction("getChoiceExp(uint256)", 3) == encodeArgs());
+	BOOST_CHECK(callContractFunction("getChoiceFromSigned(int256)", -1) == encodeArgs());
+	BOOST_CHECK(callContractFunction("getChoiceFromNegativeLiteral()") == encodeArgs());
+	// These should work
+	BOOST_CHECK(callContractFunction("getChoiceExp(uint256)", 2) == encodeArgs(2));
+	BOOST_CHECK(callContractFunction("getChoiceExp(uint256)", 0) == encodeArgs(0));
+}
+
 BOOST_AUTO_TEST_CASE(using_contract_enums_with_explicit_contract_name)
 {
 	char const* sourceCode = R"(
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 099c3c00..f498a0b9 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -1535,6 +1535,21 @@ BOOST_AUTO_TEST_CASE(enum_implicit_conversion_is_not_okay)
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
 }
 
+BOOST_AUTO_TEST_CASE(enum_to_enum_conversion_is_not_okay)
+{
+	char const* text = R"(
+		contract test {
+			enum Paper { Up, Down, Left, Right }
+			enum Ground { North, South, West, East }
+			function test()
+			{
+				Ground(Paper.Up);
+			}
+		}
+	)";
+	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
+}
+
 BOOST_AUTO_TEST_CASE(enum_duplicate_values)
 {
 	char const* text = R"(
diff --git a/test/libsolidity/SolidityParser.cpp b/test/libsolidity/SolidityParser.cpp
index a81a9828..ec23d5fd 100644
--- a/test/libsolidity/SolidityParser.cpp
+++ b/test/libsolidity/SolidityParser.cpp
@@ -824,7 +824,7 @@ BOOST_AUTO_TEST_CASE(empty_enum_declaration)
 		contract c {
 			enum foo { }
 		})";
-	BOOST_CHECK(successParse(text));
+	BOOST_CHECK(!successParse(text));
 }
 
 BOOST_AUTO_TEST_CASE(malformed_enum_declaration)