Make ambigous override checker generic.

libsolidity/analysis/ContractLevelChecker.cpp

@@ -744,13 +744,22 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 	// Walk through the set of functions signature by signature.
 	for (auto it = nonOverriddenFunctions.cbegin(); it != nonOverriddenFunctions.cend();)
 	{
-		static constexpr auto compareById = [](auto const* a, auto const* b) { return a->id() < b->id(); };
+		std::function<bool(CallableDeclaration const*, CallableDeclaration const*)> compareById = [](auto const* a, auto const* b) { return a->id() < b->id(); };
-		std::set<FunctionDefinition const*, decltype(compareById)> baseFunctions(compareById);
+		std::set<CallableDeclaration const*, std::function<bool(CallableDeclaration const*, CallableDeclaration const*)>> baseFunctions(compareById);
 		for (auto nextSignature = nonOverriddenFunctions.upper_bound(*it); it != nextSignature; ++it)
 			baseFunctions.insert(*it);
 
-		if (baseFunctions.size() <= 1)
+		checkAmbiguousOverridesInternal(std::move(baseFunctions), _contract.location());
-			continue;
+	}
+}
+
+void ContractLevelChecker::checkAmbiguousOverridesInternal(set<
+	CallableDeclaration const*,
+	std::function<bool(CallableDeclaration const*, CallableDeclaration const*)>
+> _baseCallables, SourceLocation const& _location) const
+{
+	if (_baseCallables.size() <= 1)
+		return;
 
 	// Construct the override graph for this signature.
 	// Reserve node 0 for the current contract and node
@@ -758,13 +767,13 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 	// connect at the end.
 	struct OverrideGraph
 	{
-			OverrideGraph(decltype(baseFunctions) const& _baseFunctions)
+		OverrideGraph(decltype(_baseCallables) const& __baseCallables)
 		{
-				for (auto const* baseFunction: _baseFunctions)
+			for (auto const* baseFunction: __baseCallables)
 				addEdge(0, visit(baseFunction));
 		}
-			std::map<FunctionDefinition const*, int> nodes;
+		std::map<CallableDeclaration const*, int> nodes;
-			std::map<int, FunctionDefinition const*> nodeInv;
+		std::map<int, CallableDeclaration const*> nodeInv;
 		std::map<int, std::set<int>> edges;
 		int numNodes = 2;
 		void addEdge(int _a, int _b)
@@ -775,7 +784,7 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 	private:
 		/// Completes the graph starting from @a _function and
 		/// @returns the node ID.
-			int visit(FunctionDefinition const* _function)
+		int visit(CallableDeclaration const* _function)
 		{
 			auto it = nodes.find(_function);
 			if (it != nodes.end())
@@ -791,7 +800,7 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 
 			return currentNode;
 		}
-		} overrideGraph(baseFunctions);
+	} overrideGraph(_baseCallables);
 
 	// Detect cut vertices following https://en.wikipedia.org/wiki/Biconnected_component#Pseudocode
 	// Can ignore the root node, since it is never a cut vertex in our case.
@@ -801,7 +810,7 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 		{
 			run();
 		}
-			std::set<FunctionDefinition const*> const& cutVertices() const { return m_cutVertices; }
+		std::set<CallableDeclaration const*> const& cutVertices() const { return m_cutVertices; }
 
 	private:
 		OverrideGraph const& m_graph;
@@ -810,7 +819,7 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 		std::vector<int> m_depths = std::vector<int>(m_graph.numNodes, -1);
 		std::vector<int> m_low = std::vector<int>(m_graph.numNodes, -1);
 		std::vector<int> m_parent = std::vector<int>(m_graph.numNodes, -1);
-			std::set<FunctionDefinition const*> m_cutVertices{};
+		std::set<CallableDeclaration const*> m_cutVertices{};
 
 		void run(int _u = 0, int _depth = 0)
 		{
@@ -833,36 +842,47 @@ void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _co
 	// Remove all base functions overridden by cut vertices (they don't need to be overridden).
 	for (auto const* function: cutVertexFinder.cutVertices())
 	{
-			std::set<FunctionDefinition const*> toTraverse = function->annotation().baseFunctions;
+		std::set<CallableDeclaration const*> toTraverse = function->annotation().baseFunctions;
 		while (!toTraverse.empty())
 		{
-				auto const* base = *toTraverse.begin();
+			auto const *base = *toTraverse.begin();
 			toTraverse.erase(toTraverse.begin());
-				baseFunctions.erase(base);
+			_baseCallables.erase(base);
-				for (auto const* f: base->annotation().baseFunctions)
+			for (CallableDeclaration const* f: base->annotation().baseFunctions)
 				toTraverse.insert(f);
 		}
-			// Remove unimplemented base functions at the cut vertices themselves as well.
+		// Remove unimplemented base functions at the cut vertices itself as well.
-			if (!function->isImplemented())
+		if (auto opt = dynamic_cast<ImplementationOptional const*>(function))
-				baseFunctions.erase(function);
+			if (!opt->isImplemented())
+				_baseCallables.erase(function);
 	}
 
 	// If more than one function is left, they have to be overridden.
-		if (baseFunctions.size() <= 1)
+	if (_baseCallables.size() <= 1)
-			continue;
+		return;
 
 	SecondarySourceLocation ssl;
-		for (auto const* baseFunction: baseFunctions)
+	for (auto const* baseFunction: _baseCallables)
-			ssl.append("Definition here: ", baseFunction->location());
+	{
+		string contractName = dynamic_cast<ContractDefinition const&>(*baseFunction->scope()).name();
+		ssl.append("Definition in \"" + contractName + "\": ", baseFunction->location());
+	}
+
+	string callableName;
+	if (dynamic_cast<FunctionDefinition const*>(*_baseCallables.begin()))
+		callableName = "function";
+	else if (dynamic_cast<ModifierDefinition const*>(*_baseCallables.begin()))
+		callableName = "modifier";
+	else
+		solAssert(false, "Invalid type for ambiguous override.");
 
 	m_errorReporter.typeError(
-			_contract.location(),
+		_location,
 		ssl,
-			"Derived contract must override function \"" +
+		"Derived contract must override " + callableName + " \"" +
-			(*baseFunctions.begin())->name() +
+		(*_baseCallables.begin())->name() +
-			"\". Function with the same name and parameter types defined in two or more base classes."
+		"\". Two or more base classes define " + callableName + " with same name and parameter types."
 	);
-	}
 }
 
 set<ContractDefinition const*, ContractLevelChecker::LessFunction> ContractLevelChecker::resolveOverrideList(OverrideSpecifier const& _overrides) const

libsolidity/analysis/ContractLevelChecker.h

@@ -22,7 +22,9 @@
 #pragma once
 
 #include <libsolidity/ast/ASTForward.h>
+#include <liblangutil/SourceLocation.h>
 #include <map>
+#include <functional>
 #include <set>
 
 namespace langutil
@@ -115,6 +117,10 @@ private:
 	/// Checks for functions in different base contracts which conflict with each
 	/// other and thus need to be overridden explicitly.
 	void checkAmbiguousOverrides(ContractDefinition const& _contract) const;
+	void checkAmbiguousOverridesInternal(std::set<
+		CallableDeclaration const*,
+		std::function<bool(CallableDeclaration const*, CallableDeclaration const*)>
+	> _baseCallables, langutil::SourceLocation const& _location) const;
 	/// Resolves an override list of UserDefinedTypeNames to a list of contracts.
 	std::set<ContractDefinition const*, LessFunction> resolveOverrideList(OverrideSpecifier const& _overrides) const;
 

libsolidity/ast/AST.cpp

@@ -312,6 +312,16 @@ ContractDefinition::ContractKind FunctionDefinition::inContractKind() const
 	return contractDef->contractKind();
 }
 
+CallableDeclarationAnnotation& CallableDeclaration::annotation() const
+{
+	solAssert(
+		m_annotation,
+		"CallableDeclarationAnnotation is an abstract base, need to call annotation on the concrete class first."
+	);
+	return dynamic_cast<CallableDeclarationAnnotation&>(*m_annotation);
+}
+
+
 FunctionTypePointer FunctionDefinition::functionType(bool _internal) const
 {
 	if (_internal)

libsolidity/ast/AST.h

@@ -624,6 +624,8 @@ public:
 	bool markedVirtual() const { return m_isVirtual; }
 	virtual bool virtualSemantics() const { return markedVirtual(); }
 
+	CallableDeclarationAnnotation& annotation() const override;
+
 protected:
 	ASTPointer<ParameterList> m_parameters;
 	ASTPointer<OverrideSpecifier> m_overrides;

libsolidity/ast/ASTAnnotations.h

@@ -104,19 +104,23 @@ struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, DocumentedAnnota
 	std::map<FunctionDefinition const*, ASTNode const*> baseConstructorArguments;
 };
 
-struct FunctionDefinitionAnnotation: ASTAnnotation, DocumentedAnnotation
+struct CallableDeclarationAnnotation: ASTAnnotation
+{
+	/// The set of functions/modifiers/events this callable overrides.
+	std::set<CallableDeclaration const*> baseFunctions;
+};
+
+struct FunctionDefinitionAnnotation: CallableDeclarationAnnotation, DocumentedAnnotation
 {
-	/// The set of functions this function overrides.
-	std::set<FunctionDefinition const*> baseFunctions;
 	/// Pointer to the contract this function is defined in
 	ContractDefinition const* contract = nullptr;
 };
 
-struct EventDefinitionAnnotation: ASTAnnotation, DocumentedAnnotation
+struct EventDefinitionAnnotation: CallableDeclarationAnnotation, DocumentedAnnotation
 {
 };
 
-struct ModifierDefinitionAnnotation: ASTAnnotation, DocumentedAnnotation
+struct ModifierDefinitionAnnotation: CallableDeclarationAnnotation, DocumentedAnnotation
 {
 };
 

libsolidity/ast/ASTJsonConverter.cpp

@@ -392,7 +392,7 @@ bool ASTJsonConverter::visit(VariableDeclaration const& _node)
 
 bool ASTJsonConverter::visit(ModifierDefinition const& _node)
 {
-	setJsonNode(_node, "ModifierDefinition", {
+	std::vector<pair<string, Json::Value>> attributes = {
 		make_pair("name", _node.name()),
 		make_pair("documentation", _node.documentation() ? Json::Value(*_node.documentation()) : Json::nullValue),
 		make_pair("visibility", Declaration::visibilityToString(_node.visibility())),
@@ -400,7 +400,10 @@ bool ASTJsonConverter::visit(ModifierDefinition const& _node)
 		make_pair("virtual", _node.markedVirtual()),
 		make_pair("overrides", _node.overrides() ? toJson(*_node.overrides()) : Json::nullValue),
 		make_pair("body", toJson(_node.body()))
-	});
+	};
+	if (!_node.annotation().baseFunctions.empty())
+		attributes.emplace_back(make_pair("baseModifiers", getContainerIds(_node.annotation().baseFunctions, true)));
+	setJsonNode(_node, "ModifierDefinition", std::move(attributes));
 	return false;
 }
 

test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_implementation.sol

@@ -16,5 +16,5 @@ abstract contract B is I {
 contract C is A, B {
 }
 // ----
-// TypeError: (342-364): Derived contract must override function "f". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (342-364): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.
-// TypeError: (342-364): Derived contract must override function "g". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (342-364): Derived contract must override function "g". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_mention.sol

@@ -14,4 +14,4 @@ abstract contract B is I {
 contract C is A, B {
 }
 // ----
-// TypeError: (254-276): Derived contract must override function "f". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (254-276): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_implementation.sol

@@ -13,5 +13,5 @@ abstract contract B is I {
 contract C is A, B {
 }
 // ----
-// TypeError: (292-314): Derived contract must override function "f". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (292-314): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.
-// TypeError: (292-314): Derived contract must override function "g". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (292-314): Derived contract must override function "g". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/internal_external_inheritance.sol

@@ -6,4 +6,4 @@ contract B {
 }
 contract C is A, B {}
 // ----
-// TypeError: (126-147): Derived contract must override function "f". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (126-147): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/no_common_base_and_unique_implementation.sol

@@ -9,5 +9,5 @@ abstract contract B {
 contract C is A, B {
 }
 // ----
-// TypeError: (176-198): Derived contract must override function "f". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (176-198): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.
-// TypeError: (176-198): Derived contract must override function "g". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (176-198): Derived contract must override function "g". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation_modifier.sol

@@ -15,3 +15,6 @@ contract B is IJ
 {
 }
 contract C is A, B {}
+// ----
+// TypeError: (14-33): Trying to override non-virtual modifier. Did you forget to add "virtual"?
+// TypeError: (50-69): Trying to override non-virtual modifier. Did you forget to add "virtual"?

test/libsolidity/syntaxTests/inheritance/override/override_ambiguous.sol

@@ -9,4 +9,4 @@ abstract contract X is A, B {
 	function test() internal override returns (uint256) {}
 }
 // ----
-// TypeError: (205-292): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (205-292): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/override_multiple_no_virtual2.sol

@@ -10,4 +10,4 @@ contract C is A, B
 {
 }
 // ----
-// TypeError: (94-116): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (94-116): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/override_return_mismatch.sol

@@ -9,4 +9,4 @@ abstract contract X is A, B {
 	function test() internal override virtual returns (uint256);
 }
 // ----
-// TypeError: (203-296): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (203-296): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/public_vars_multiple.sol

@@ -8,4 +8,4 @@ contract X is A, B {
 	uint public override foo;
 }
 // ----
-// TypeError: (162-211): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (162-211): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/public_vars_multiple_diamond.sol

@@ -11,4 +11,4 @@ contract X is B, C {
 	uint public override foo;
 }
 // ----
-// TypeError: (271-320): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (271-320): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/public_vars_multiple_diamond1.sol

@@ -12,4 +12,4 @@ contract X is B, C {
 }
 // ----
 // DeclarationError: (245-269): Identifier already declared.
-// TypeError: (223-272): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (223-272): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/public_vars_multiple_explicit_override.sol

@@ -8,4 +8,4 @@ contract X is A, B {
 	uint public override(A, B) foo;
 }
 // ----
-// TypeError: (162-217): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (162-217): Derived contract must override function "foo". Two or more base classes define function with same name and parameter types.

test/libsolidity/syntaxTests/inheritance/override/triangle_impl.sol

@@ -3,4 +3,4 @@ contract B is A { function f() public pure virtual override {} }
 contract C is A, B { }
 contract D is A, B { function f() public pure override(A, B) {} }
 // ----
-// TypeError: (116-138): Derived contract must override function "f". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (116-138): Derived contract must override function "f". Two or more base classes define function with same name and parameter types.