Merge pull request ethereum#7878 from ethereum/overrideUnimplementedW…

…ithBaseImpl

Do not require overriding for functions in common base with unique implementation.

docs/contracts/inheritance.rst

@@ -237,8 +237,10 @@ bases, it has to explicitly override it:
         function foo() public override(Base1, Base2) {}
     }
 
-A function defined in a common base contract does not have to be explicitly
-overridden when used with multiple inheritance:
+An explicit override specifier is not required if
+the function is defined in a common base contract
+or if there is a unique function in a common base contract
+that already overrides all other functions.
 
 ::
 
@@ -250,6 +252,19 @@ overridden when used with multiple inheritance:
     // No explicit override required
     contract D is B, C {}
 
+More formally, it is not required to override a function (directly or
+indirectly) inherited from multiple bases if there is a base contract
+that is part of all override paths for the signature, and (1) that
+base implements the function and no paths from the current contract
+to the base mentions a function with that signature or (2) that base
+does not implement the function and there is at most one mention of
+the function in all paths from the current contract to that base.
+
+In this sense, an override path for a signature is a path through
+the inheritance graph that starts at the contract under consideration
+and ends at a contract mentioning a function with that signature
+that does not override.
+
 .. note::
 
   Functions with the ``private`` visibility cannot be ``virtual``.

libdevcore/CommonData.h

@@ -112,24 +112,32 @@ inline std::set<T> operator+(std::set<T>&& _a, U&& _b)
 	return ret;
 }
 
-/// Remove one set from another one.
-template <class... T>
-inline std::set<T...>& operator-=(std::set<T...>& _a, std::set<T...> const& _b)
+/// Remove the elements of a container from a set.
+template <class C, class... T>
+inline std::set<T...>& operator-=(std::set<T...>& _a, C const& _b)
 {
 	for (auto const& x: _b)
 		_a.erase(x);
 	return _a;
 }
 
-template <class... T>
-inline std::set<T...> operator-(std::set<T...> const& _a, std::set<T...> const& _b)
+template <class C, class... T>
+inline std::set<T...> operator-(std::set<T...> const& _a, C const& _b)
 {
 	auto result = _a;
 	result -= _b;
-
 	return result;
 }
 
+/// Remove the elements of a container from a multiset.
+template <class C, class... T>
+inline std::multiset<T...>& operator-=(std::multiset<T...>& _a, C const& _b)
+{
+	for (auto const& x: _b)
+		_a.erase(x);
+	return _a;
+}
+
 namespace dev
 {
 

libsolidity/analysis/ContractLevelChecker.cpp

@@ -238,8 +238,8 @@ void ContractLevelChecker::findDuplicateDefinitions(map<string, vector<T>> const
 
 void ContractLevelChecker::checkIllegalOverrides(ContractDefinition const& _contract)
 {
-	FunctionMultiSet const& funcSet = inheritedFunctions(&_contract);
-	ModifierMultiSet const& modSet = inheritedModifiers(&_contract);
+	FunctionMultiSet const& funcSet = inheritedFunctions(_contract);
+	ModifierMultiSet const& modSet = inheritedModifiers(_contract);
 
 	checkModifierOverrides(funcSet, modSet, _contract.functionModifiers());
 
@@ -666,56 +666,132 @@ void ContractLevelChecker::checkBaseABICompatibility(ContractDefinition const& _
 
 void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _contract) const
 {
-	vector<FunctionDefinition const*> contractFuncs = _contract.definedFunctions();
-
-	auto const resolvedBases = resolveDirectBaseContracts(_contract);
-
-	FunctionMultiSet inheritedFuncs = inheritedFunctions(&_contract);;
-
-	// Check the sets of the most-inherited functions
-	for (auto it = inheritedFuncs.cbegin(); it != inheritedFuncs.cend(); it = inheritedFuncs.upper_bound(*it))
+	// Fetch inherited functions and sort them by signature.
+	// We get at least one function per signature and direct base contract, which is
+	// enough because we re-construct the inheritance graph later.
+	FunctionMultiSet nonOverriddenFunctions = inheritedFunctions(_contract);
+	// Remove all functions that match the signature of a function in the current contract.
+	nonOverriddenFunctions -= _contract.definedFunctions();
+
+	// Walk through the set of functions signature by signature.
+	for (auto it = nonOverriddenFunctions.cbegin(); it != nonOverriddenFunctions.cend();)
 	{
-		auto [begin,end] = inheritedFuncs.equal_range(*it);
+		static constexpr auto compareById = [](auto const* a, auto const* b) { return a->id() < b->id(); };
+		std::set<FunctionDefinition const*, decltype(compareById)> baseFunctions(compareById);
+		for (auto nextSignature = nonOverriddenFunctions.upper_bound(*it); it != nextSignature; ++it)
+			baseFunctions.insert(*it);
 
-		// Only one function
-		if (next(begin) == end)
+		if (baseFunctions.size() <= 1)
 			continue;
 
-		// Not an overridable function
-		if ((*it)->isConstructor())
+		// Construct the override graph for this signature.
+		// Reserve node 0 for the current contract and node
+		// 1 for an artificial top node to which all override paths
+		// connect at the end.
+		struct OverrideGraph
 		{
-			for (begin++; begin != end; begin++)
-				solAssert((*begin)->isConstructor(), "All functions in range expected to be constructors!");
-			continue;
-		}
+			OverrideGraph(decltype(baseFunctions) const& _baseFunctions)
+			{
+				for (auto const* baseFunction: _baseFunctions)
+					addEdge(0, visit(baseFunction));
+			}
+			std::map<FunctionDefinition const*, int> nodes;
+			std::map<int, FunctionDefinition const*> nodeInv;
+			std::map<int, std::set<int>> edges;
+			int numNodes = 2;
+			void addEdge(int _a, int _b)
+			{
+				edges[_a].insert(_b);
+				edges[_b].insert(_a);
+			}
+		private:
+			/// Completes the graph starting from @a _function and
+			/// @returns the node ID.
+			int visit(FunctionDefinition const* _function)
+			{
+				auto it = nodes.find(_function);
+				if (it != nodes.end())
+					return it->second;
+				int currentNode = numNodes++;
+				nodes[_function] = currentNode;
+				nodeInv[currentNode] = _function;
+				if (_function->overrides())
+					for (auto const* baseFunction: _function->annotation().baseFunctions)
+						addEdge(currentNode, visit(baseFunction));
+				else
+					addEdge(currentNode, 1);
+
+				return currentNode;
+			}
+		} overrideGraph(baseFunctions);
 
-		// Function has been explicitly overridden
-		if (contains_if(
-			contractFuncs,
-			[&] (FunctionDefinition const* _f) {
-				return hasEqualNameAndParameters(*_f, **it);
+		// 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.
+		struct CutVertexFinder
+		{
+			CutVertexFinder(OverrideGraph const& _graph): m_graph(_graph)
+			{
+				run();
 			}
-		))
-			continue;
+			std::set<FunctionDefinition const*> const& cutVertices() const { return m_cutVertices; }
 
-		set<FunctionDefinition const*> ambiguousFunctions;
-		SecondarySourceLocation ssl;
+		private:
+			OverrideGraph const& m_graph;
+
+			std::vector<bool> m_visited = std::vector<bool>(m_graph.numNodes, false);
+			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{};
 
-		for (;begin != end; begin++)
+			void run(int _u = 0, int _depth = 0)
+			{
+				m_visited.at(_u) = true;
+				m_depths.at(_u) = m_low.at(_u) = _depth;
+				for (int v: m_graph.edges.at(_u))
+					if (!m_visited.at(v))
+					{
+						m_parent[v] = _u;
+						run(v, _depth + 1);
+						if (m_low[v] >= m_depths[_u] && m_parent[_u] != -1)
+							m_cutVertices.insert(m_graph.nodeInv.at(_u));
+						m_low[_u] = min(m_low[_u], m_low[v]);
+					}
+					else if (v != m_parent[_u])
+						m_low[_u] = min(m_low[_u], m_depths[v]);
+			}
+		} cutVertexFinder{overrideGraph};
+
+		// Remove all base functions overridden by cut vertices (they don't need to be overridden).
+		for (auto const* function: cutVertexFinder.cutVertices())
 		{
-			ambiguousFunctions.insert(*begin);
-			ssl.append("Definition here: ", (*begin)->location());
+			std::set<FunctionDefinition const*> toTraverse = function->annotation().baseFunctions;
+			while (!toTraverse.empty())
+			{
+				auto const* base = *toTraverse.begin();
+				toTraverse.erase(toTraverse.begin());
+				baseFunctions.erase(base);
+				for (auto const* f: base->annotation().baseFunctions)
+					toTraverse.insert(f);
+			}
+			// Remove unimplemented base functions at the cut vertices themselves as well.
+			if (!function->isImplemented())
+				baseFunctions.erase(function);
 		}
 
-		// Make sure the functions are not from the same base contract
-		if (ambiguousFunctions.size() == 1)
+		// If more than one function is left, they have to be overridden.
+		if (baseFunctions.size() <= 1)
 			continue;
 
+		SecondarySourceLocation ssl;
+		for (auto const* baseFunction: baseFunctions)
+			ssl.append("Definition here: ", baseFunction->location());
+
 		m_errorReporter.typeError(
 			_contract.location(),
 			ssl,
 			"Derived contract must override function \"" +
-			(*it)->name() +
+			(*baseFunctions.begin())->name() +
 			"\". Function with the same name and parameter types defined in two or more base classes."
 		);
 	}
@@ -845,50 +921,52 @@ void ContractLevelChecker::checkOverrideList(FunctionMultiSet const& _funcSet, F
 		);
 }
 
-ContractLevelChecker::FunctionMultiSet const& ContractLevelChecker::inheritedFunctions(ContractDefinition const* _contract) const
+ContractLevelChecker::FunctionMultiSet const& ContractLevelChecker::inheritedFunctions(ContractDefinition const& _contract) const
 {
-	if (!m_inheritedFunctions.count(_contract))
+	if (!m_inheritedFunctions.count(&_contract))
 	{
 		FunctionMultiSet set;
 
-		for (auto const* base: resolveDirectBaseContracts(*_contract))
+		for (auto const* base: resolveDirectBaseContracts(_contract))
 		{
-			std::set<FunctionDefinition const*, LessFunction> tmpSet =
-				convertContainer<decltype(tmpSet)>(base->definedFunctions());
+			std::set<FunctionDefinition const*, LessFunction> functionsInBase;
+			for (FunctionDefinition const* fun: base->definedFunctions())
+				if (!fun->isConstructor())
+					functionsInBase.emplace(fun);
 
-			for (auto const& func: inheritedFunctions(base))
-				tmpSet.insert(func);
+			for (auto const& func: inheritedFunctions(*base))
+				functionsInBase.insert(func);
 
-			set += tmpSet;
+			set += functionsInBase;
 		}
 
-		m_inheritedFunctions[_contract] = set;
+		m_inheritedFunctions[&_contract] = set;
 	}
 
-	return m_inheritedFunctions[_contract];
+	return m_inheritedFunctions[&_contract];
 }
 
-ContractLevelChecker::ModifierMultiSet const& ContractLevelChecker::inheritedModifiers(ContractDefinition const* _contract) const
+ContractLevelChecker::ModifierMultiSet const& ContractLevelChecker::inheritedModifiers(ContractDefinition const& _contract) const
 {
-	auto const& result = m_contractBaseModifiers.find(_contract);
+	auto const& result = m_contractBaseModifiers.find(&_contract);
 
 	if (result != m_contractBaseModifiers.cend())
 		return result->second;
 
 	ModifierMultiSet set;
 
-	for (auto const* base: resolveDirectBaseContracts(*_contract))
+	for (auto const* base: resolveDirectBaseContracts(_contract))
 	{
 		std::set<ModifierDefinition const*, LessFunction> tmpSet =
 			convertContainer<decltype(tmpSet)>(base->functionModifiers());
 
-		for (auto const& mod: inheritedModifiers(base))
+		for (auto const& mod: inheritedModifiers(*base))
 			tmpSet.insert(mod);
 
 		set += tmpSet;
 	}
 
-	return m_contractBaseModifiers[_contract] = set;
+	return m_contractBaseModifiers[&_contract] = set;
 }
 
 void ContractLevelChecker::checkPayableFallbackWithoutReceive(ContractDefinition const& _contract)

libsolidity/analysis/ContractLevelChecker.h

@@ -106,8 +106,8 @@ class ContractLevelChecker
 
 	/// Returns all functions of bases that have not yet been overwritten.
 	/// May contain the same function multiple times when used with shared bases.
-	FunctionMultiSet const& inheritedFunctions(ContractDefinition const* _contract) const;
-	ModifierMultiSet const& inheritedModifiers(ContractDefinition const* _contract) const;
+	FunctionMultiSet const& inheritedFunctions(ContractDefinition const& _contract) const;
+	ModifierMultiSet const& inheritedModifiers(ContractDefinition const& _contract) const;
 
 	/// Warns if the contract has a payable fallback, but no receive ether function.
 	void checkPayableFallbackWithoutReceive(ContractDefinition const& _contract);

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

@@ -0,0 +1,20 @@
+interface I {
+    function f() external;
+    function g() external;
+}
+interface J {
+	function f() external;
+}
+abstract contract A is I, J {
+    function f() external override (I, J) {}
+    function g() external override virtual;
+}
+abstract contract B is I {
+    function f() external override virtual;
+    function g() external override {}
+}
+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 "g". Function with the same name and parameter types defined in two or more base classes.

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

@@ -0,0 +1,17 @@
+interface I {
+    function f() external;
+    function g() external;
+}
+interface J {
+	function f() external;
+}
+abstract contract A is I, J {
+    function f() external override (I, J) {}
+}
+abstract contract B is I {
+    function g() external override {}
+}
+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.

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

@@ -0,0 +1,12 @@
+contract A {
+	function f() external virtual {}
+}
+contract B {
+	function f() external virtual {}
+}
+contract C is A, B {
+	function f() external override (A, B) {}
+}
+contract X is C {
+}
+// ----

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

@@ -0,0 +1,15 @@
+contract A {
+	function f() external virtual {}
+}
+contract B {
+	function f() external virtual {}
+}
+contract C is A, B {
+	function f() external override (A, B);
+}
+contract X is C {
+}
+// ----
+// TypeError: (120-158): Overriding an implemented function with an unimplemented function is not allowed.
+// TypeError: (120-158): Overriding an implemented function with an unimplemented function is not allowed.
+// TypeError: (120-158): Functions without implementation must be marked virtual.