Refactor function override check to remove duplicate logic

wenlinlee · Aug 11, 2017 · a694985 · a694985
1 parent f0dc572
commit a694985
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 47 deletions.
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
@@ -277,21 +277,10 @@ void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contr
 			string const& name = function->name();
 			if (modifiers.count(name))
 				m_errorReporter.typeError(modifiers[name]->location(), "Override changes function to modifier.");
-			FunctionType functionType(*function);
-			// function should not change the return type
+
 			for (FunctionDefinition const* overriding: functions[name])
-			{
-				FunctionType overridingType(*overriding);
-				if (!overridingType.hasEqualArgumentTypes(functionType))
-					continue;
-				if (
-					overriding->visibility() != function->visibility() ||
-					overriding->isDeclaredConst() != function->isDeclaredConst() ||
-					overriding->isPayable() != function->isPayable() ||
-					overridingType != functionType
-				)
-					overrideTypeError(*overriding, *function);
-			}
+				checkFunctionOverride(*overriding, *function);
+
 			functions[name].push_back(function);
 		}
 		for (ModifierDefinition const* modifier: contract->functionModifiers())
@@ -308,6 +297,51 @@ void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contr
 	}
 }
 
+void TypeChecker::checkFunctionOverride(FunctionDefinition const& function, FunctionDefinition const& super)
+{
+	FunctionType functionType(function);
+	FunctionType superType(super);
+
+	if (!functionType.hasEqualArgumentTypes(superType))
+		return;
+
+	if (function.visibility() != super.visibility())
+		m_errorReporter.typeError(
+			function.location(),
+			"Overriding function visibility differs from " + super.fullyQualifiedName() + "."
+		);
+
+	if (function.isDeclaredConst() && !super.isDeclaredConst())
+		m_errorReporter.typeError(
+			function.location(),
+			"Overriding function should not be declared constant."
+		);
+
+	if (!function.isDeclaredConst() && super.isDeclaredConst())
+		m_errorReporter.typeError(
+			function.location(),
+			"Overriding function should be declared constant."
+		);
+
+	if (function.isPayable() && !super.isPayable())
+		m_errorReporter.typeError(
+			function.location(),
+			"Overriding function should not be declared payable."
+		);
+
+	if (!function.isPayable() && super.isPayable())
+		m_errorReporter.typeError(
+			function.location(),
+			"Overriding function should be declared payable."
+		);
+
+	if (functionType != superType)
+		m_errorReporter.typeError(
+			function.location(),
+			"Overriding function return types differ from " + super.fullyQualifiedName() + "."
+		);
+}
+
 void TypeChecker::checkContractExternalTypeClashes(ContractDefinition const& _contract)
 {
 	map<string, vector<pair<Declaration const*, FunctionTypePointer>>> externalDeclarations;
@@ -1949,33 +1983,3 @@ void TypeChecker::requireLValue(Expression const& _expression)
 	else if (!_expression.annotation().isLValue)
 		m_errorReporter.typeError(_expression.location(), "Expression has to be an lvalue.");
 }
-
-void TypeChecker::overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super)
-{
-	string message;
-
-	if (function.visibility() != super.visibility())
-		message = "Overriding function visibility differs from " + super.fullyQualifiedName() + ".";
-	else if (function.isDeclaredConst() && !super.isDeclaredConst())
-		message = "Overriding function should not be declared constant.";
-	else if (!function.isDeclaredConst() && super.isDeclaredConst())
-		message = "Overriding function should be declared constant.";
-	else if (function.isPayable() && !super.isPayable())
-		message = "Overriding function should not be declared payable.";
-	else if (!function.isPayable() && super.isPayable())
-		message = "Overriding function should be declared payable.";
-
-	if (message.empty())
-	{
-		FunctionType functionType(function);
-		FunctionType superType(super);
-
-		if (functionType != superType)
-			message = "Overriding function return types differ from " + super.fullyQualifiedName() + ".";
-	}
-
-	if (message.empty())
-		message = "Overriding function signature differs from " + super.fullyQualifiedName() + ".";
-
-	m_errorReporter.typeError(function.location(), message);
-}
diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h
@@ -62,6 +62,8 @@ class TypeChecker: private ASTConstVisitor
 	/// arguments and that there is at most one constructor.
 	void checkContractDuplicateFunctions(ContractDefinition const& _contract);
 	void checkContractIllegalOverrides(ContractDefinition const& _contract);
+	/// Reports a type error with an appropiate message if overriden function signature differs.
+	void checkFunctionOverride(FunctionDefinition const& function, FunctionDefinition const& super);
 	void checkContractAbstractFunctions(ContractDefinition const& _contract);
 	void checkContractAbstractConstructors(ContractDefinition const& _contract);
 	/// Checks that different functions with external visibility end up having different
@@ -120,9 +122,6 @@ class TypeChecker: private ASTConstVisitor
 	/// Runs type checks on @a _expression to infer its type and then checks that it is an LValue.
 	void requireLValue(Expression const& _expression);
 
-	/// Reports a type error with an appropiate message when overriden function signature differs.
-	void overrideTypeError(FunctionDefinition const& function, FunctionDefinition const& super);
-
 	ContractDefinition const* m_scope = nullptr;
 
 	ErrorReporter& m_errorReporter;