From 4c7f9f9751e569e5e0a32e5e53677811d98f28d7 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Tue, 3 Dec 2019 13:58:30 +0100 Subject: [PATCH 1/3] Do not require overriding for functions in common base with unique implementation. --- libdevcore/CommonData.h | 20 +- libsolidity/analysis/ContractLevelChecker.cpp | 176 +++++++++++++----- libsolidity/analysis/ContractLevelChecker.h | 4 +- ...biguous_base_and_unique_implementation.sol | 20 ++ .../ambiguous_base_and_unique_mention.sol | 17 ++ ...ctions_overridden_in_intermediate_base.sol | 12 ++ .../common_base_and_unique_implementation.sol | 17 ++ .../common_base_and_unique_mention.sol | 12 ++ ..._common_base_and_unique_implementation.sol | 13 ++ ..._common_base_and_unique_implementation.sol | 19 ++ .../inheritance/override/triangle_impl.sol | 6 + .../inheritance/override/triangle_no_impl.sol | 5 + 12 files changed, 264 insertions(+), 57 deletions(-) create mode 100644 test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_implementation.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_mention.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_functions_overridden_in_intermediate_base.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_implementation.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_mention.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/no_common_base_and_unique_implementation.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/triangle_impl.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/triangle_no_impl.sol diff --git a/libdevcore/CommonData.h b/libdevcore/CommonData.h index c34472ed0..6128e1dd1 100644 --- a/libdevcore/CommonData.h +++ b/libdevcore/CommonData.h @@ -112,24 +112,32 @@ inline std::set operator+(std::set&& _a, U&& _b) return ret; } -/// Remove one set from another one. -template -inline std::set& operator-=(std::set& _a, std::set const& _b) +/// Remove the elements of a container from a set. +template +inline std::set& operator-=(std::set& _a, C const& _b) { for (auto const& x: _b) _a.erase(x); return _a; } -template -inline std::set operator-(std::set const& _a, std::set const& _b) +template +inline std::set operator-(std::set const& _a, C const& _b) { auto result = _a; result -= _b; - return result; } +/// Remove the elements of a container from a multiset. +template +inline std::multiset& operator-=(std::multiset& _a, C const& _b) +{ + for (auto const& x: _b) + _a.erase(x); + return _a; +} + namespace dev { diff --git a/libsolidity/analysis/ContractLevelChecker.cpp b/libsolidity/analysis/ContractLevelChecker.cpp index 875fa532f..f581f9976 100644 --- a/libsolidity/analysis/ContractLevelChecker.cpp +++ b/libsolidity/analysis/ContractLevelChecker.cpp @@ -238,8 +238,8 @@ void ContractLevelChecker::findDuplicateDefinitions(map> 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 contractFuncs = _contract.definedFunctions(); + // 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(); - 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)) + // 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 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; - } - - // Function has been explicitly overridden - if (contains_if( - contractFuncs, - [&] (FunctionDefinition const* _f) { - return hasEqualNameAndParameters(*_f, **it); + OverrideGraph(decltype(baseFunctions) const& _baseFunctions) + { + for (auto const* baseFunction: _baseFunctions) + addEdge(0, visit(baseFunction)); } - )) - continue; + std::map nodes; + std::map nodeInv; + std::map> 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); - set ambiguousFunctions; - SecondarySourceLocation ssl; + return currentNode; + } + } overrideGraph(baseFunctions); - for (;begin != end; begin++) + // 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 { - ambiguousFunctions.insert(*begin); - ssl.append("Definition here: ", (*begin)->location()); + CutVertexFinder(OverrideGraph const& _graph): m_graph(_graph) + { + run(); + } + std::set const& cutVertices() const { return m_cutVertices; } + + private: + OverrideGraph const& m_graph; + + std::vector m_visited = std::vector(m_graph.numNodes, false); + std::vector m_depths = std::vector(m_graph.numNodes, -1); + std::vector m_low = std::vector(m_graph.numNodes, -1); + std::vector m_parent = std::vector(m_graph.numNodes, -1); + std::set m_cutVertices{}; + + 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()) + { + std::set 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 tmpSet = - convertContainer(base->definedFunctions()); + std::set 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 tmpSet = convertContainer(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) diff --git a/libsolidity/analysis/ContractLevelChecker.h b/libsolidity/analysis/ContractLevelChecker.h index e1e421e05..bcc54a643 100644 --- a/libsolidity/analysis/ContractLevelChecker.h +++ b/libsolidity/analysis/ContractLevelChecker.h @@ -106,8 +106,8 @@ private: /// 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); diff --git a/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_implementation.sol b/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_implementation.sol new file mode 100644 index 000000000..35488814c --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_mention.sol b/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_and_unique_mention.sol new file mode 100644 index 000000000..103cb235c --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_functions_overridden_in_intermediate_base.sol b/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_functions_overridden_in_intermediate_base.sol new file mode 100644 index 000000000..cd358c420 --- /dev/null +++ b/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 { +} +// ---- diff --git a/test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_implementation.sol b/test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_implementation.sol new file mode 100644 index 000000000..00029c3a8 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_implementation.sol @@ -0,0 +1,17 @@ +interface I { + function f() external; + function g() external; +} +abstract contract A is I { + function f() external override {} + function g() external override virtual; +} +abstract contract B is I { + function g() external override {} + function f() external override virtual; +} +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 "g". Function with the same name and parameter types defined in two or more base classes. diff --git a/test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_mention.sol b/test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_mention.sol new file mode 100644 index 000000000..0cedc3da4 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/common_base_and_unique_mention.sol @@ -0,0 +1,12 @@ +interface I { + function f() external; + function g() external; +} +abstract contract A is I { + function f() external override {} +} +abstract contract B is I { + function g() external override {} +} +contract C is A, B { +} \ No newline at end of file diff --git a/test/libsolidity/syntaxTests/inheritance/override/no_common_base_and_unique_implementation.sol b/test/libsolidity/syntaxTests/inheritance/override/no_common_base_and_unique_implementation.sol new file mode 100644 index 000000000..c4b33f13d --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/no_common_base_and_unique_implementation.sol @@ -0,0 +1,13 @@ +abstract contract A { + function f() external {} + function g() external virtual; +} +abstract contract B { + function g() external {} + function f() external virtual; +} +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 "g". Function with the same name and parameter types defined in two or more base classes. diff --git a/test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation.sol b/test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation.sol new file mode 100644 index 000000000..3aab863c3 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation.sol @@ -0,0 +1,19 @@ +interface I { + function f() external; + function g() external; +} +interface J { + function f() external; +} +abstract contract IJ is I, J { + function f() external virtual override (I, J); +} +abstract contract A is IJ +{ + function f() external override {} +} +abstract contract B is IJ +{ + function g() external override {} +} +contract C is A, B {} diff --git a/test/libsolidity/syntaxTests/inheritance/override/triangle_impl.sol b/test/libsolidity/syntaxTests/inheritance/override/triangle_impl.sol new file mode 100644 index 000000000..35ab2f1c7 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/triangle_impl.sol @@ -0,0 +1,6 @@ +contract A { function f() public pure virtual {} } +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. diff --git a/test/libsolidity/syntaxTests/inheritance/override/triangle_no_impl.sol b/test/libsolidity/syntaxTests/inheritance/override/triangle_no_impl.sol new file mode 100644 index 000000000..786a78df2 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/triangle_no_impl.sol @@ -0,0 +1,5 @@ +abstract contract A { function f() public pure virtual; } +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) {} } +// ---- From 6d2f1f3068dd8c2fbb6c860d96f604bd46e036e3 Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 6 Dec 2019 09:19:51 +0100 Subject: [PATCH 2/3] Documentation about unique base functions. --- docs/contracts/inheritance.rst | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/contracts/inheritance.rst b/docs/contracts/inheritance.rst index ddaaafd1f..8f2cb1144 100644 --- a/docs/contracts/inheritance.rst +++ b/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``. From 06e8e216b341ea4ecf20980d821ed3b4cea77d93 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 9 Dec 2019 13:05:07 +0100 Subject: [PATCH 3/3] Some more tests. --- ...den_in_intermediate_base_unimplemented.sol | 15 +++++++++++++++ ...nd_unique_implementation_unimplemented.sol | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_functions_overridden_in_intermediate_base_unimplemented.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation_unimplemented.sol diff --git a/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_functions_overridden_in_intermediate_base_unimplemented.sol b/test/libsolidity/syntaxTests/inheritance/override/ambiguous_base_functions_overridden_in_intermediate_base_unimplemented.sol new file mode 100644 index 000000000..b30bc85b0 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation_unimplemented.sol b/test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation_unimplemented.sol new file mode 100644 index 000000000..6b6e7091e --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/nonintermediate_common_base_and_unique_implementation_unimplemented.sol @@ -0,0 +1,19 @@ +interface I { + function f() external; + function g() external; +} +interface J { + function f() external; +} +abstract contract IJ is I, J { + function f() external virtual override (I, J); +} +abstract contract A is IJ +{ + function f() external virtual override; +} +abstract contract B is IJ +{ + function g() external override {} +} +abstract contract C is A, B {}