Merge pull request #2581 from federicobond/improve-override-error

Improve override function error messages

Changelog.md

@@ -4,6 +4,7 @@ Features:
  * Parser: Display previous visibility specifier in error if multiple are found.
  * Syntax Checker: Support ``pragma experimental <feature>;`` to turn on experimental features.
  * Metadata: Store experimental flag in metadata CBOR.
+ * Type Checker: More detailed error message for invalid overrides.
 
 Bugfixes:
  * Parser: Enforce commas between array and tuple elements.

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])
-			{
+				checkFunctionOverride(*overriding, *function);
-				FunctionType overridingType(*overriding);
+
-				if (!overridingType.hasEqualArgumentTypes(functionType))
-					continue;
-				if (
-					overriding->visibility() != function->visibility() ||
-					overriding->isDeclaredConst() != function->isDeclaredConst() ||
-					overriding->isPayable() != function->isPayable() ||
-					overridingType != functionType
-				)
-					m_errorReporter.typeError(overriding->location(), "Override changes extended function signature.");
-			}
 			functions[name].push_back(function);
 		}
 		for (ModifierDefinition const* modifier: contract->functionModifiers())
@@ -308,6 +297,42 @@ 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())
+		overrideError(function, super, "Overriding function visibility differs.");
+
+	else if (function.isDeclaredConst() && !super.isDeclaredConst())
+		overrideError(function, super, "Overriding function should not be declared constant.");
+
+	else if (!function.isDeclaredConst() && super.isDeclaredConst())
+		overrideError(function, super, "Overriding function should be declared constant.");
+
+	else if (function.isPayable() && !super.isPayable())
+		overrideError(function, super, "Overriding function should not be declared payable.");
+
+	else if (!function.isPayable() && super.isPayable())
+		overrideError(function, super, "Overriding function should be declared payable.");
+
+	else if (functionType != superType)
+		overrideError(function, super, "Overriding function return types differ.");
+}
+
+void TypeChecker::overrideError(FunctionDefinition const& function, FunctionDefinition const& super, string message)
+{
+	m_errorReporter.typeError(
+		function.location(),
+		SecondarySourceLocation().append("Overriden function is here:", super.location()),
+		message
+	);
+}
+
 void TypeChecker::checkContractExternalTypeClashes(ContractDefinition const& _contract)
 {
 	map<string, vector<pair<Declaration const*, FunctionTypePointer>>> externalDeclarations;
@@ -1949,4 +1974,3 @@ void TypeChecker::requireLValue(Expression const& _expression)
 	else if (!_expression.annotation().isLValue)
 		m_errorReporter.typeError(_expression.location(), "Expression has to be an lvalue.");
 }
-

libsolidity/analysis/TypeChecker.h

@@ -62,6 +62,9 @@ private:
 	/// 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 overrideError(FunctionDefinition const& function, FunctionDefinition const& super, std::string message);
 	void checkContractAbstractFunctions(ContractDefinition const& _contract);
 	void checkContractAbstractConstructors(ContractDefinition const& _contract);
 	/// Checks that different functions with external visibility end up having different

libsolidity/ast/AST.cpp

@@ -371,6 +371,15 @@ string FunctionDefinition::externalSignature() const
 	return FunctionType(*this).externalSignature();
 }
 
+string FunctionDefinition::fullyQualifiedName() const
+{
+	auto const* contract = dynamic_cast<ContractDefinition const*>(scope());
+	solAssert(contract, "Enclosing scope of function definition was not set.");
+
+	auto fname = name().empty() ? "<fallback>" : name();
+	return sourceUnitName() + ":" + contract->name() + "." + fname;
+}
+
 FunctionDefinitionAnnotation& FunctionDefinition::annotation() const
 {
 	if (!m_annotation)

libsolidity/ast/AST.h

@@ -613,6 +613,7 @@ public:
 	std::vector<ASTPointer<ModifierInvocation>> const& modifiers() const { return m_functionModifiers; }
 	std::vector<ASTPointer<VariableDeclaration>> const& returnParameters() const { return m_returnParameters->parameters(); }
 	Block const& body() const { solAssert(m_body, ""); return *m_body; }
+	std::string fullyQualifiedName() const;
 	virtual bool isVisibleInContract() const override
 	{
 		return Declaration::isVisibleInContract() && !isConstructor() && !isFallback();

libsolidity/interface/ErrorReporter.h

@@ -75,8 +75,8 @@ public:
 
 	void typeError(
 		SourceLocation const& _location,
-		SecondarySourceLocation const& _secondaryLocation,
+		SecondarySourceLocation const& _secondaryLocation = SecondarySourceLocation(),
-		std::string const& _description
+		std::string const& _description = std::string()
 	);
 
 	void typeError(SourceLocation const& _location, std::string const& _description);

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -924,16 +924,25 @@ BOOST_AUTO_TEST_CASE(illegal_override_visibility)
 		contract B { function f() internal {} }
 		contract C is B { function f() public {} }
 	)";
-	CHECK_ERROR(text, TypeError, "Override changes extended function signature.");
+	CHECK_ERROR(text, TypeError, "Overriding function visibility differs");
 }
 
-BOOST_AUTO_TEST_CASE(illegal_override_constness)
+BOOST_AUTO_TEST_CASE(illegal_override_remove_constness)
 {
 	char const* text = R"(
 		contract B { function f() constant {} }
 		contract C is B { function f() {} }
 	)";
-	CHECK_ERROR(text, TypeError, "Override changes extended function signature.");
+	CHECK_ERROR(text, TypeError, "Overriding function should be declared constant.");
+}
+
+BOOST_AUTO_TEST_CASE(illegal_override_add_constness)
+{
+	char const* text = R"(
+		contract B { function f() {} }
+		contract C is B { function f() constant {} }
+	)";
+	CHECK_ERROR(text, TypeError, "Overriding function should not be declared constant.");
 }
 
 BOOST_AUTO_TEST_CASE(complex_inheritance)
@@ -2491,7 +2500,7 @@ BOOST_AUTO_TEST_CASE(override_changes_return_types)
 			function f(uint a) returns (uint8) { }
 		}
 	)";
-	CHECK_ERROR(sourceCode, TypeError, "Override changes extended function signature.");
+	CHECK_ERROR(sourceCode, TypeError, "Overriding function return types differ");
 }
 
 BOOST_AUTO_TEST_CASE(multiple_constructors)
@@ -4732,7 +4741,7 @@ BOOST_AUTO_TEST_CASE(illegal_override_payable)
 		contract B { function f() payable {} }
 		contract C is B { function f() {} }
 	)";
-	CHECK_ERROR(text, TypeError, "Override changes extended function signature.");
+	CHECK_ERROR(text, TypeError, "Overriding function should be declared payable.");
 }
 
 BOOST_AUTO_TEST_CASE(illegal_override_payable_nonpayable)
@@ -4741,7 +4750,7 @@ BOOST_AUTO_TEST_CASE(illegal_override_payable_nonpayable)
 		contract B { function f() {} }
 		contract C is B { function f() payable {} }
 	)";
-	CHECK_ERROR(text, TypeError, "Override changes extended function signature.");
+	CHECK_ERROR(text, TypeError, "Overriding function should not be declared payable.");
 }
 
 BOOST_AUTO_TEST_CASE(function_variable_mixin)