Merge pull request #1729 from ethereum/constantvariables

Only allow pure expressions for constant state variables.

8 files changed, 237 insertions, 31 deletions

diff --git a/Changelog.md b/Changelog.md
index 68f0037b..69c75615 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -20,6 +20,7 @@ Bugfixes:
  * Type system: Detect cyclic dependencies between constants.
  * Type system: Disallow arrays with negative length.
  * Type system: Fix a crash related to invalid binary operators.
+ * Type system: Warn if constant state variables are not compile-time constants.
  * Type system: Disallow ``var`` declaration with empty tuple type.
  * Type system: Correctly convert function argument types to pointers for member functions.
  * Type system: Move privateness of constructor into AST itself.
diff --git a/docs/contracts.rst b/docs/contracts.rst
index 1516ba0b..9145f016 100644
--- a/docs/contracts.rst
+++ b/docs/contracts.rst
@@ -428,8 +428,25 @@ change by overriding).
 Constant State Variables
 ************************
 
-State variables can be declared as constant (this is not yet implemented
-for array and struct types and not possible for mapping types).
+State variables can be declared as ``constant``. In this case, they have to be
+assigned from an expression which is a constant at compile time. Any expression
+that accesses storage, blockchain data (e.g. ``now``, ``this.balance`` or
+``block.number``) or
+execution data (``msg.gas``) or make calls to external contracts are disallowed. Expressions
+that might have a side-effect on memory allocation are allowed, but those that
+might have a side-effect on other memory objects are not. The built-in functions
+``keccak256``, ``sha256``, ``ripemd160``, ``ecrecover``, ``addmod`` and ``mulmod``
+are allowed (ever though they do call external contracts).
+
+The reason behind allowing side-effects on the memory allocator is that it
+should be possible to construct complex objects like e.g. lookup-tables.
+This feature is not yet fully usable.
+
+The compiler does not reserve a storage slot for these variables and every occurrence is
+replaced by the respective constant expression (which might be computed to a single value by the optimizer).
+
+Not all types for constants are implemented at this time. The only supported types are
+value types and strings.
 
 ::
 
@@ -438,12 +455,9 @@ for array and struct types and not possible for mapping types).
     contract C {
         uint constant x = 32**22 + 8;
         string constant text = "abc";
+        bytes32 constant myHash = keccak256("abc");
     }
 
-This has the effect that the compiler does not reserve a storage slot
-for these variables and every occurrence is replaced by their constant value.
-
-The value expression can only contain integer arithmetics.
 
 ******************
 Constant Functions
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index fbff6865..8e7ec29b 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/libsolidity/analysis/TypeChecker.cpp
@@ -467,27 +467,29 @@ bool TypeChecker::visit(VariableDeclaration const& _variable)
 	// TypeChecker at the VariableDeclarationStatement level.
 	TypePointer varType = _variable.annotation().type;
 	solAssert(!!varType, "Failed to infer variable type.");
+	if (_variable.value())
+		expectType(*_variable.value(), *varType);
 	if (_variable.isConstant())
 	{
-		if (!dynamic_cast<ContractDefinition const*>(_variable.scope()))
+		if (!_variable.isStateVariable())
 			typeError(_variable.location(), "Illegal use of \"constant\" specifier.");
-		if (!_variable.value())
-			typeError(_variable.location(), "Uninitialized \"constant\" variable.");
-		if (!varType->isValueType())
+		if (!_variable.type()->isValueType())
 		{
-			bool constImplemented = false;
-			if (auto arrayType = dynamic_cast<ArrayType const*>(varType.get()))
-				constImplemented = arrayType->isByteArray();
-			if (!constImplemented)
-				typeError(
-					_variable.location(),
-					"Illegal use of \"constant\" specifier. \"constant\" "
-					"is not yet implemented for this type."
-				);
+			bool allowed = false;
+			if (auto arrayType = dynamic_cast<ArrayType const*>(_variable.type().get()))
+				allowed = arrayType->isString();
+			if (!allowed)
+				typeError(_variable.location(), "Constants of non-value type not yet implemented.");
 		}
+		if (!_variable.value())
+			typeError(_variable.location(), "Uninitialized \"constant\" variable.");
+		else if (!_variable.value()->annotation().isPure)
+			warning(
+				_variable.value()->location(),
+				"Initial value for constant variable has to be compile-time constant. "
+				"This will fail to compile with the next breaking version change."
+			);
 	}
-	if (_variable.value())
-		expectType(*_variable.value(), *varType);
 	if (!_variable.isStateVariable())
 	{
 		if (varType->dataStoredIn(DataLocation::Memory) || varType->dataStoredIn(DataLocation::CallData))
@@ -928,6 +930,10 @@ bool TypeChecker::visit(Conditional const& _conditional)
 	}
 
 	_conditional.annotation().type = commonType;
+	_conditional.annotation().isPure =
+		_conditional.condition().annotation().isPure &&
+		_conditional.trueExpression().annotation().isPure &&
+		_conditional.falseExpression().annotation().isPure;
 
 	if (_conditional.annotation().lValueRequested)
 		typeError(
@@ -1009,6 +1015,7 @@ bool TypeChecker::visit(TupleExpression const& _tuple)
 	}
 	else
 	{
+		bool isPure = true;
 		TypePointer inlineArrayType;
 		for (size_t i = 0; i < components.size(); ++i)
 		{
@@ -1031,10 +1038,13 @@ bool TypeChecker::visit(TupleExpression const& _tuple)
 					else if (inlineArrayType)
 						inlineArrayType = Type::commonType(inlineArrayType, types[i]);
 				}
+				if (!components[i]->annotation().isPure)
+					isPure = false;
 			}
 			else
 				types.push_back(TypePointer());
 		}
+		_tuple.annotation().isPure = isPure;
 		if (_tuple.isInlineArray())
 		{
 			if (!inlineArrayType) 
@@ -1061,7 +1071,8 @@ bool TypeChecker::visit(UnaryOperation const& _operation)
 {
 	// Inc, Dec, Add, Sub, Not, BitNot, Delete
 	Token::Value op = _operation.getOperator();
-	if (op == Token::Value::Inc || op == Token::Value::Dec || op == Token::Value::Delete)
+	bool const modifying = (op == Token::Value::Inc || op == Token::Value::Dec || op == Token::Value::Delete);
+	if (modifying)
 		requireLValue(_operation.subExpression());
 	else
 		_operation.subExpression().accept(*this);
@@ -1079,6 +1090,7 @@ bool TypeChecker::visit(UnaryOperation const& _operation)
 		t = subExprType;
 	}
 	_operation.annotation().type = t;
+	_operation.annotation().isPure = !modifying && _operation.subExpression().annotation().isPure;
 	return false;
 }
 
@@ -1105,6 +1117,10 @@ void TypeChecker::endVisit(BinaryOperation const& _operation)
 		Token::isCompareOp(_operation.getOperator()) ?
 		make_shared<BoolType>() :
 		commonType;
+	_operation.annotation().isPure =
+		_operation.leftExpression().annotation().isPure &&
+		_operation.rightExpression().annotation().isPure;
+
 	if (_operation.getOperator() == Token::Exp)
 	{
 		if (
@@ -1133,6 +1149,8 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
 	vector<ASTPointer<Expression const>> arguments = _functionCall.arguments();
 	vector<ASTPointer<ASTString>> const& argumentNames = _functionCall.names();
 
+	bool isPure = true;
+
 	// We need to check arguments' type first as they will be needed for overload resolution.
 	shared_ptr<TypePointers> argumentTypes;
 	if (isPositionalCall)
@@ -1140,6 +1158,8 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
 	for (ASTPointer<Expression const> const& argument: arguments)
 	{
 		argument->accept(*this);
+		if (!argument->annotation().isPure)
+			isPure = false;
 		// only store them for positional calls
 		if (isPositionalCall)
 			argumentTypes->push_back(type(*argument));
@@ -1177,6 +1197,7 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
 				typeError(_functionCall.location(), "Explicit type conversion not allowed.");
 		}
 		_functionCall.annotation().type = resultType;
+		_functionCall.annotation().isPure = isPure;
 
 		return false;
 	}
@@ -1193,9 +1214,16 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
 		auto const& structType = dynamic_cast<StructType const&>(*t.actualType());
 		functionType = structType.constructorType();
 		membersRemovedForStructConstructor = structType.membersMissingInMemory();
+		_functionCall.annotation().isPure = isPure;
 	}
 	else
+	{
 		functionType = dynamic_pointer_cast<FunctionType const>(expressionType);
+		_functionCall.annotation().isPure =
+			isPure &&
+			_functionCall.expression().annotation().isPure &&
+			functionType->isPure();
+	}
 
 	if (!functionType)
 	{
@@ -1360,6 +1388,7 @@ void TypeChecker::endVisit(NewExpression const& _newExpression)
 			strings(),
 			FunctionType::Location::ObjectCreation
 		);
+		_newExpression.annotation().isPure = true;
 	}
 	else
 		fatalTypeError(_newExpression.location(), "Contract or array type expected.");
@@ -1445,6 +1474,12 @@ bool TypeChecker::visit(MemberAccess const& _memberAccess)
 			annotation.isLValue = annotation.referencedDeclaration->isLValue();
 	}
 
+	// TODO some members might be pure, but for example `address(0x123).balance` is not pure
+	// although every subexpression is, so leaving this limited for now.
+	if (auto tt = dynamic_cast<TypeType const*>(exprType.get()))
+		if (tt->actualType()->category() == Type::Category::Enum)
+			annotation.isPure = true;
+
 	return false;
 }
 
@@ -1454,6 +1489,7 @@ bool TypeChecker::visit(IndexAccess const& _access)
 	TypePointer baseType = type(_access.baseExpression());
 	TypePointer resultType;
 	bool isLValue = false;
+	bool isPure = _access.baseExpression().annotation().isPure;
 	Expression const* index = _access.indexExpression();
 	switch (baseType->category())
 	{
@@ -1535,6 +1571,9 @@ bool TypeChecker::visit(IndexAccess const& _access)
 	}
 	_access.annotation().type = move(resultType);
 	_access.annotation().isLValue = isLValue;
+	if (index && !index->annotation().isPure)
+		isPure = false;
+	_access.annotation().isPure = isPure;
 
 	return false;
 }
@@ -1589,18 +1628,22 @@ bool TypeChecker::visit(Identifier const& _identifier)
 		!!annotation.referencedDeclaration,
 		"Referenced declaration is null after overload resolution."
 	);
-	auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration);
-	annotation.isConstant = variableDeclaration != nullptr && variableDeclaration->isConstant();
 	annotation.isLValue = annotation.referencedDeclaration->isLValue();
 	annotation.type = annotation.referencedDeclaration->type();
 	if (!annotation.type)
 		fatalTypeError(_identifier.location(), "Declaration referenced before type could be determined.");
+	if (auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(annotation.referencedDeclaration))
+		annotation.isPure = annotation.isConstant = variableDeclaration->isConstant();
+	else if (dynamic_cast<MagicVariableDeclaration const*>(annotation.referencedDeclaration))
+		if (auto functionType = dynamic_cast<FunctionType const*>(annotation.type.get()))
+			annotation.isPure = functionType->isPure();
 	return false;
 }
 
 void TypeChecker::endVisit(ElementaryTypeNameExpression const& _expr)
 {
 	_expr.annotation().type = make_shared<TypeType>(Type::fromElementaryTypeName(_expr.typeName()));
+	_expr.annotation().isPure = true;
 }
 
 void TypeChecker::endVisit(Literal const& _literal)
@@ -1620,6 +1663,7 @@ void TypeChecker::endVisit(Literal const& _literal)
 			);
 	}
 	_literal.annotation().type = Type::forLiteral(_literal);
+	_literal.annotation().isPure = true;
 	if (!_literal.annotation().type)
 		fatalTypeError(_literal.location(), "Invalid literal value.");
 }
diff --git a/libsolidity/ast/ASTAnnotations.h b/libsolidity/ast/ASTAnnotations.h
index 9c4c3ae8..bd297f9e 100644
--- a/libsolidity/ast/ASTAnnotations.h
+++ b/libsolidity/ast/ASTAnnotations.h
@@ -156,6 +156,8 @@ struct ExpressionAnnotation: ASTAnnotation
 	TypePointer type;
 	/// Whether the expression is a constant variable
 	bool isConstant = false;
+	/// Whether the expression is pure, i.e. compile-time constant.
+	bool isPure = false;
 	/// Whether it is an LValue (i.e. something that can be assigned to).
 	bool isLValue = false;
 	/// Whether the expression is used in a context where the LValue is actually required.
diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp
index d2793b6d..0e11c3ec 100644
--- a/libsolidity/ast/Types.cpp
+++ b/libsolidity/ast/Types.cpp
@@ -2456,6 +2456,18 @@ u256 FunctionType::externalIdentifier() const
 	return FixedHash<4>::Arith(FixedHash<4>(dev::keccak256(externalSignature())));
 }
 
+bool FunctionType::isPure() const
+{
+	return
+		m_location == Location::SHA3 ||
+		m_location == Location::ECRecover ||
+		m_location == Location::SHA256 ||
+		m_location == Location::RIPEMD160 ||
+		m_location == Location::AddMod ||
+		m_location == Location::MulMod ||
+		m_location == Location::ObjectCreation;
+}
+
 TypePointers FunctionType::parseElementaryTypeVector(strings const& _types)
 {
 	TypePointers pointers;
diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h
index 960b7e86..0a4878b8 100644
--- a/libsolidity/ast/Types.h
+++ b/libsolidity/ast/Types.h
@@ -973,6 +973,10 @@ public:
 	}
 	bool hasDeclaration() const { return !!m_declaration; }
 	bool isConstant() const { return m_isConstant; }
+	/// @returns true if the the result of this function only depends on its arguments
+	/// and it does not modify the state.
+	/// Currently, this will only return true for internal functions like keccak and ecrecover.
+	bool isPure() const;
 	bool isPayable() const { return m_isPayable; }
 	/// @return A shared pointer of an ASTString.
 	/// Can contain a nullptr in which case indicates absence of documentation
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 2a286cea..a4fab721 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -4542,7 +4542,6 @@ BOOST_AUTO_TEST_CASE(simple_constant_variables_test)
 
 BOOST_AUTO_TEST_CASE(constant_variables)
 {
-	//for now constant specifier is valid only for uint, bytesXX, string and enums
 	char const* sourceCode = R"(
 		contract Foo {
 			uint constant x = 56;
@@ -4553,6 +4552,58 @@ BOOST_AUTO_TEST_CASE(constant_variables)
 	compileAndRun(sourceCode);
 }
 
+BOOST_AUTO_TEST_CASE(assignment_to_const_var_involving_expression)
+{
+	char const* sourceCode = R"(
+		contract C {
+			uint constant x = 0x123 + 0x456;
+			function f() returns (uint) { return x + 1; }
+		}
+	)";
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(0x123 + 0x456 + 1));
+}
+
+BOOST_AUTO_TEST_CASE(assignment_to_const_var_involving_keccak)
+{
+	char const* sourceCode = R"(
+		contract C {
+			bytes32 constant x = keccak256("abc");
+			function f() returns (bytes32) { return x; }
+		}
+	)";
+	compileAndRun(sourceCode);
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(dev::keccak256("abc")));
+}
+
+// Disabled until https://github.com/ethereum/solidity/issues/715 is implemented
+//BOOST_AUTO_TEST_CASE(assignment_to_const_array_vars)
+//{
+//	char const* sourceCode = R"(
+//		contract C {
+//			uint[3] constant x = [uint(1), 2, 3];
+//			uint constant y = x[0] + x[1] + x[2];
+//			function f() returns (uint) { return y; }
+//		}
+//	)";
+//	compileAndRun(sourceCode);
+//	BOOST_CHECK(callContractFunction("f()") == encodeArgs(1 + 2 + 3));
+//}
+
+// Disabled until https://github.com/ethereum/solidity/issues/715 is implemented
+//BOOST_AUTO_TEST_CASE(constant_struct)
+//{
+//	char const* sourceCode = R"(
+//		contract C {
+//			struct S { uint x; uint[] y; }
+//			S constant x = S(5, new uint[](4));
+//			function f() returns (uint) { return x.x; }
+//		}
+//	)";
+//	compileAndRun(sourceCode);
+//	BOOST_CHECK(callContractFunction("f()") == encodeArgs(5));
+//}
+
 BOOST_AUTO_TEST_CASE(packed_storage_structs_uint)
 {
 	char const* sourceCode = R"(
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 865fd0c5..27791775 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -2173,16 +2173,94 @@ BOOST_AUTO_TEST_CASE(assigning_value_to_const_variable)
 	CHECK_ERROR(text, TypeError, "");
 }
 
-BOOST_AUTO_TEST_CASE(complex_const_variable)
+BOOST_AUTO_TEST_CASE(assigning_state_to_const_variable)
 {
-	//for now constant specifier is valid only for uint bytesXX and enums
 	char const* text = R"(
-		contract Foo {
-			mapping(uint => bool) x;
-			mapping(uint => bool) constant mapVar = x;
+		contract C {
+			address constant x = msg.sender;
 		}
 	)";
-	CHECK_ERROR(text, TypeError, "");
+	// Change to TypeError for 0.5.0.
+	CHECK_WARNING(text, "Initial value for constant variable has to be compile-time constant.");
+}
+
+BOOST_AUTO_TEST_CASE(constant_string_literal_disallows_assignment)
+{
+	char const* text = R"(
+		contract Test {
+			string constant x = "abefghijklmnopqabcdefghijklmnopqabcdefghijklmnopqabca";
+			function f() {
+				x[0] = "f";
+			}
+		}
+	)";
+
+	// Even if this is made possible in the future, we should not allow assignment
+	// to elements of constant arrays.
+	CHECK_ERROR(text, TypeError, "Index access for string is not possible.");
+}
+
+BOOST_AUTO_TEST_CASE(assign_constant_function_value_to_constant)
+{
+	char const* text = R"(
+		contract C {
+			function () constant returns (uint) x;
+			uint constant y = x();
+		}
+	)";
+	// Change to TypeError for 0.5.0.
+	CHECK_WARNING(text, "Initial value for constant variable has to be compile-time constant.");
+}
+
+BOOST_AUTO_TEST_CASE(assignment_to_const_var_involving_conversion)
+{
+	char const* text = R"(
+		contract C {
+			C constant x = C(0x123);
+		}
+	)";
+	CHECK_SUCCESS(text);
+}
+
+BOOST_AUTO_TEST_CASE(assignment_to_const_var_involving_expression)
+{
+	char const* text = R"(
+		contract C {
+			uint constant x = 0x123 + 0x456;
+		}
+	)";
+	CHECK_SUCCESS(text);
+}
+
+BOOST_AUTO_TEST_CASE(assignment_to_const_var_involving_keccak)
+{
+	char const* text = R"(
+		contract C {
+			bytes32 constant x = keccak256("abc");
+		}
+	)";
+	CHECK_SUCCESS(text);
+}
+
+BOOST_AUTO_TEST_CASE(assignment_to_const_array_vars)
+{
+	char const* text = R"(
+		contract C {
+			uint[3] constant x = [uint(1), 2, 3];
+		}
+	)";
+	CHECK_ERROR(text, TypeError, "implemented");
+}
+
+BOOST_AUTO_TEST_CASE(constant_struct)
+{
+	char const* text = R"(
+		contract C {
+			struct S { uint x; uint[] y; }
+			S constant x = S(5, new uint[](4));
+		}
+	)";
+	CHECK_ERROR(text, TypeError, "implemented");
 }
 
 BOOST_AUTO_TEST_CASE(uninitialized_const_variable)