Merge pull request #1479 from ethereum/function_variable_mixin

Disallow mixin of functions and attributes under the same name

9 files changed, 190 insertions, 67 deletions

diff --git a/Changelog.md b/Changelog.md
index 3e999d13..bef98f15 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -8,6 +8,7 @@ Bugfixes:
  * Remappings: Prefer longer context over longer prefix.
  * Type checker, code generator: enable access to events of base contracts' names.
  * Imports: ``import ".dir/a"`` is not a relative path.  Relative paths begin with directory ``.`` or ``..``.
+ * Type checker, disallow inheritances of different kinds (e.g. a function and a modifier) of members of the same name
 
 ### 0.4.7 (2016-12-15)
 
diff --git a/docs/contracts.rst b/docs/contracts.rst
index e82b7495..edc42c3d 100644
--- a/docs/contracts.rst
+++ b/docs/contracts.rst
@@ -877,6 +877,13 @@ cannot be resolved.
 A simple rule to remember is to specify the base classes in
 the order from "most base-like" to "most derived".
 
+Inheriting Different Kinds of Members of the Same Name
+======================================================
+
+When the inheritance results in a contract with a function and a modifier of the same name, it is considered as an error.
+This error is produced also by an event and a modifier of the same name, and a function and an event of the same name.
+As an exception, a state variable accessor can override a public function.
+
 .. index:: ! contract;abstract, ! abstract contract
 
 ******************
diff --git a/libsolidity/analysis/DeclarationContainer.cpp b/libsolidity/analysis/DeclarationContainer.cpp
index 1599b83a..f8c12c5b 100644
--- a/libsolidity/analysis/DeclarationContainer.cpp
+++ b/libsolidity/analysis/DeclarationContainer.cpp
@@ -44,10 +44,19 @@ Declaration const* DeclarationContainer::conflictingDeclaration(
 
 	if (dynamic_cast<FunctionDefinition const*>(&_declaration))
 	{
-		// check that all other declarations with the same name are functions
+		// check that all other declarations with the same name are functions or a public state variable
 		for (Declaration const* declaration: declarations)
-			if (!dynamic_cast<FunctionDefinition const*>(declaration))
+		{
+			if (dynamic_cast<FunctionDefinition const*>(declaration))
+				continue;
+			if (auto variableDeclaration = dynamic_cast<VariableDeclaration const*>(declaration))
+			{
+				if (variableDeclaration->isStateVariable() && !variableDeclaration->isConstant() && variableDeclaration->isPublic())
+					continue;
 				return declaration;
+			}
+			return declaration;
+		}
 	}
 	else if (declarations.size() == 1 && declarations.front() == &_declaration)
 		return nullptr;
diff --git a/libsolidity/analysis/NameAndTypeResolver.cpp b/libsolidity/analysis/NameAndTypeResolver.cpp
index 2a33a501..08323243 100644
--- a/libsolidity/analysis/NameAndTypeResolver.cpp
+++ b/libsolidity/analysis/NameAndTypeResolver.cpp
@@ -260,10 +260,16 @@ vector<Declaration const*> NameAndTypeResolver::cleanedDeclarations(
 	for (auto it = _declarations.begin(); it != _declarations.end(); ++it)
 	{
 		solAssert(*it, "");
-		// the declaration is functionDefinition while declarations > 1
-		FunctionDefinition const& functionDefinition = dynamic_cast<FunctionDefinition const&>(**it);
-		FunctionType functionType(functionDefinition);
-		for (auto parameter: functionType.parameterTypes() + functionType.returnParameterTypes())
+		// the declaration is functionDefinition or a VariableDeclaration while declarations > 1
+		solAssert(dynamic_cast<FunctionDefinition const*>(*it) || dynamic_cast<VariableDeclaration const*>(*it),
+			"Found overloading involving something not a function or a variable");
+
+		shared_ptr<FunctionType const> functionType { (*it)->functionType(false) };
+		if (!functionType)
+			functionType = (*it)->functionType(true);
+		solAssert(functionType, "failed to determine the function type of the overloaded");
+
+		for (auto parameter: functionType->parameterTypes() + functionType->returnParameterTypes())
 			if (!parameter)
 				reportFatalDeclarationError(_identifier.location(), "Function type can not be used in this context");
 
@@ -272,8 +278,10 @@ vector<Declaration const*> NameAndTypeResolver::cleanedDeclarations(
 			uniqueFunctions.end(),
 			[&](Declaration const* d)
 			{
-				FunctionType newFunctionType(dynamic_cast<FunctionDefinition const&>(*d));
-				return functionType.hasEqualArgumentTypes(newFunctionType);
+				shared_ptr<FunctionType const> newFunctionType { d->functionType(false) };
+				if (!newFunctionType)
+					newFunctionType = d->functionType(true);
+				return newFunctionType && functionType->hasEqualArgumentTypes(*newFunctionType);
 			}
 		))
 			uniqueFunctions.push_back(*it);
@@ -289,7 +297,39 @@ void NameAndTypeResolver::importInheritedScope(ContractDefinition const& _base)
 		for (auto const& declaration: nameAndDeclaration.second)
 			// Import if it was declared in the base, is not the constructor and is visible in derived classes
 			if (declaration->scope() == &_base && declaration->isVisibleInDerivedContracts())
-				m_currentScope->registerDeclaration(*declaration);
+				if (!m_currentScope->registerDeclaration(*declaration))
+				{
+					SourceLocation firstDeclarationLocation;
+					SourceLocation secondDeclarationLocation;
+					Declaration const* conflictingDeclaration = m_currentScope->conflictingDeclaration(*declaration);
+					solAssert(conflictingDeclaration, "");
+
+					// Usual shadowing is not an error
+					if (dynamic_cast<VariableDeclaration const*>(declaration) && dynamic_cast<VariableDeclaration const*>(conflictingDeclaration))
+						continue;
+
+					// Usual shadowing is not an error
+					if (dynamic_cast<ModifierDefinition const*>(declaration) && dynamic_cast<ModifierDefinition const*>(conflictingDeclaration))
+						continue;
+
+					if (declaration->location().start < conflictingDeclaration->location().start)
+					{
+						firstDeclarationLocation = declaration->location();
+						secondDeclarationLocation = conflictingDeclaration->location();
+					}
+					else
+					{
+						firstDeclarationLocation = conflictingDeclaration->location();
+						secondDeclarationLocation = declaration->location();
+					}
+
+					reportDeclarationError(
+						secondDeclarationLocation,
+						"Identifier already declared.",
+						firstDeclarationLocation,
+						"The previous declaration is here:"
+					);
+				}
 }
 
 void NameAndTypeResolver::linearizeBaseContracts(ContractDefinition& _contract)
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index e414e27c..67c8ac17 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/libsolidity/analysis/TypeChecker.cpp
@@ -1500,8 +1500,23 @@ bool TypeChecker::visit(Identifier const& _identifier)
 	if (!annotation.referencedDeclaration)
 	{
 		if (!annotation.argumentTypes)
-			fatalTypeError(_identifier.location(), "Unable to determine overloaded type.");
-		if (annotation.overloadedDeclarations.empty())
+		{
+			// The identifier should be a public state variable shadowing other functions
+			vector<Declaration const*> candidates;
+
+			for (Declaration const* declaration: annotation.overloadedDeclarations)
+			{
+				if (VariableDeclaration const* variableDeclaration = dynamic_cast<decltype(variableDeclaration)>(declaration))
+					candidates.push_back(declaration);
+			}
+			if (candidates.empty())
+				fatalTypeError(_identifier.location(), "No matching declaration found after variable lookup.");
+			else if (candidates.size() == 1)
+				annotation.referencedDeclaration = candidates.front();
+			else
+				fatalTypeError(_identifier.location(), "No unique declaration found after variable lookup.");
+		}
+		else if (annotation.overloadedDeclarations.empty())
 			fatalTypeError(_identifier.location(), "No candidates for overload resolution found.");
 		else if (annotation.overloadedDeclarations.size() == 1)
 			annotation.referencedDeclaration = *annotation.overloadedDeclarations.begin();
diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp
index 78d8949c..6f7a64dc 100644
--- a/libsolidity/ast/AST.cpp
+++ b/libsolidity/ast/AST.cpp
@@ -274,6 +274,45 @@ TypeDeclarationAnnotation& EnumDefinition::annotation() const
 	return static_cast<TypeDeclarationAnnotation&>(*m_annotation);
 }
 
+shared_ptr<FunctionType> FunctionDefinition::functionType(bool _internal) const
+{
+	if (_internal)
+	{
+		switch (visibility())
+		{
+		case Declaration::Visibility::Default:
+			solAssert(false, "visibility() should not return Default");
+		case Declaration::Visibility::Private:
+		case Declaration::Visibility::Internal:
+		case Declaration::Visibility::Public:
+			return make_shared<FunctionType>(*this, _internal);
+		case Declaration::Visibility::External:
+			return {};
+		default:
+			solAssert(false, "visibility() should not return a Visibility");
+		}
+	}
+	else
+	{
+		switch (visibility())
+		{
+		case Declaration::Visibility::Default:
+			solAssert(false, "visibility() should not return Default");
+		case Declaration::Visibility::Private:
+		case Declaration::Visibility::Internal:
+			return {};
+		case Declaration::Visibility::Public:
+		case Declaration::Visibility::External:
+			return make_shared<FunctionType>(*this, _internal);
+		default:
+			solAssert(false, "visibility() should not return a Visibility");
+		}
+	}
+
+	// To make the compiler happy
+	return {};
+}
+
 TypePointer FunctionDefinition::type() const
 {
 	return make_shared<FunctionType>(*this);
@@ -308,6 +347,14 @@ TypePointer EventDefinition::type() const
 	return make_shared<FunctionType>(*this);
 }
 
+std::shared_ptr<FunctionType> EventDefinition::functionType(bool _internal) const
+{
+	if (_internal)
+		return make_shared<FunctionType>(*this);
+	else
+		return {};
+}
+
 EventDefinitionAnnotation& EventDefinition::annotation() const
 {
 	if (!m_annotation)
@@ -365,6 +412,28 @@ TypePointer VariableDeclaration::type() const
 	return annotation().type;
 }
 
+shared_ptr<FunctionType> VariableDeclaration::functionType(bool _internal) const
+{
+	if (_internal)
+		return {};
+	switch (visibility())
+	{
+	case Declaration::Visibility::Default:
+		solAssert(false, "visibility() should not return Default");
+	case Declaration::Visibility::Private:
+	case Declaration::Visibility::Internal:
+		return {};
+	case Declaration::Visibility::Public:
+	case Declaration::Visibility::External:
+		return make_shared<FunctionType>(*this);
+	default:
+		solAssert(false, "visibility() should not return a Visibility");
+	}
+
+	// To make the compiler happy
+	return {};
+}
+
 VariableDeclarationAnnotation& VariableDeclaration::annotation() const
 {
 	if (!m_annotation)
diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h
index ab4be1ea..2d092408 100644
--- a/libsolidity/ast/AST.h
+++ b/libsolidity/ast/AST.h
@@ -171,6 +171,10 @@ public:
 	/// This can only be called once types of variable declarations have already been resolved.
 	virtual TypePointer type() const = 0;
 
+	/// @param _internal false indicates external interface is concerned, true indicates internal interface is concerned.
+	/// @returns null when it is not accessible as a function.
+	virtual std::shared_ptr<FunctionType> functionType(bool /*_internal*/) const { return {}; }
+
 protected:
 	virtual Visibility defaultVisibility() const { return Visibility::Public; }
 
@@ -581,6 +585,10 @@ public:
 
 	virtual TypePointer type() const override;
 
+	/// @param _internal false indicates external interface is concerned, true indicates internal interface is concerned.
+	/// @returns null when it is not accessible as a function.
+	virtual std::shared_ptr<FunctionType> functionType(bool /*_internal*/) const override;
+
 	virtual FunctionDefinitionAnnotation& annotation() const override;
 
 private:
@@ -643,6 +651,10 @@ public:
 
 	virtual TypePointer type() const override;
 
+	/// @param _internal false indicates external interface is concerned, true indicates internal interface is concerned.
+	/// @returns null when it is not accessible as a function.
+	virtual std::shared_ptr<FunctionType> functionType(bool /*_internal*/) const override;
+
 	virtual VariableDeclarationAnnotation& annotation() const override;
 
 protected:
@@ -740,6 +752,7 @@ public:
 	bool isAnonymous() const { return m_anonymous; }
 
 	virtual TypePointer type() const override;
+	virtual std::shared_ptr<FunctionType> functionType(bool /*_internal*/) const override;
 
 	virtual EventDefinitionAnnotation& annotation() const override;
 
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index d8539524..19161831 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -4869,60 +4869,6 @@ BOOST_AUTO_TEST_CASE(proper_order_of_overwriting_of_attributes)
 	BOOST_CHECK(callContractFunction("ok()") == encodeArgs(false));
 }
 
-BOOST_AUTO_TEST_CASE(proper_overwriting_accessor_by_function)
-{
-	// bug #1798
-	char const* sourceCode = R"(
-		contract attribute {
-			bool ok = false;
-		}
-		contract func {
-			function ok() returns (bool) { return true; }
-		}
-
-		contract attr_func is attribute, func {
-			function checkOk() returns (bool) { return ok(); }
-		}
-		contract func_attr is func, attribute {
-			function checkOk() returns (bool) { return ok; }
-		}
-	)";
-	compileAndRun(sourceCode, 0, "attr_func");
-	BOOST_CHECK(callContractFunction("ok()") == encodeArgs(true));
-	compileAndRun(sourceCode, 0, "func_attr");
-	BOOST_CHECK(callContractFunction("checkOk()") == encodeArgs(false));
-}
-
-
-BOOST_AUTO_TEST_CASE(overwriting_inheritance)
-{
-	// bug #1798
-	char const* sourceCode = R"(
-		contract A {
-			function ok() returns (uint) { return 1; }
-		}
-		contract B {
-			function ok() returns (uint) { return 2; }
-		}
-		contract C {
-			uint ok = 6;
-		}
-		contract AB is A, B {
-			function ok() returns (uint) { return 4; }
-		}
-		contract reversedE is C, AB {
-			function checkOk() returns (uint) { return ok(); }
-		}
-		contract E is AB, C {
-			function checkOk() returns (uint) { return ok; }
-		}
-	)";
-	compileAndRun(sourceCode, 0, "reversedE");
-	BOOST_CHECK(callContractFunction("checkOk()") == encodeArgs(4));
-	compileAndRun(sourceCode, 0, "E");
-	BOOST_CHECK(callContractFunction("checkOk()") == encodeArgs(6));
-}
-
 BOOST_AUTO_TEST_CASE(struct_assign_reference_to_struct)
 {
 	char const* sourceCode = R"(
diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp
index 576421fd..9f6ea2b3 100644
--- a/test/libsolidity/SolidityNameAndTypeResolution.cpp
+++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp
@@ -1056,7 +1056,9 @@ BOOST_AUTO_TEST_CASE(modifier_overrides_function)
 		contract A { modifier mod(uint a) { _; } }
 		contract B is A { function mod(uint a) { } }
 	)";
-	CHECK_ERROR(text, TypeError, "");
+	// Error: Identifier already declared.
+	// Error: Override changes modifier to function.
+	CHECK_ERROR_ALLOW_MULTI(text, DeclarationError, "");
 }
 
 BOOST_AUTO_TEST_CASE(function_overrides_modifier)
@@ -1065,7 +1067,9 @@ BOOST_AUTO_TEST_CASE(function_overrides_modifier)
 		contract A { function mod(uint a) { } }
 		contract B is A { modifier mod(uint a) { _; } }
 	)";
-	CHECK_ERROR(text, TypeError, "");
+	// Error: Identifier already declared.
+	// Error: Override changes function to modifier.
+	CHECK_ERROR_ALLOW_MULTI(text, DeclarationError, "");
 }
 
 BOOST_AUTO_TEST_CASE(modifier_returns_value)
@@ -4304,6 +4308,25 @@ BOOST_AUTO_TEST_CASE(illegal_override_payable_nonpayable)
 	CHECK_ERROR(text, TypeError, "");
 }
 
+BOOST_AUTO_TEST_CASE(function_variable_mixin)
+{
+       // bug #1798 (cpp-ethereum), related to #1286 (solidity)
+       char const* text = R"(
+               contract attribute {
+                       bool ok = false;
+               }
+               contract func {
+                       function ok() returns (bool) { return true; }
+               }
+
+               contract attr_func is attribute, func {
+                       function checkOk() returns (bool) { return ok(); }
+               }
+       )";
+       CHECK_ERROR(text, DeclarationError, "");
+}
+
+
 BOOST_AUTO_TEST_CASE(payable_constant_conflict)
 {
 	char const* text = R"(