Merge pull request #1479 from ethereum/function_variable_mixin

Disallow mixin of functions and attributes under the same name

5 files changed, 157 insertions, 11 deletions

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;