Ambigous overrides for modifiers.

libsolidity/analysis/ContractLevelChecker.cpp

@@ -730,26 +730,40 @@ void ContractLevelChecker::checkBaseABICompatibility(ContractDefinition const& _
 
 void ContractLevelChecker::checkAmbiguousOverrides(ContractDefinition const& _contract) const
 {
-	// TODO same for modifiers.
+	std::function<bool(CallableDeclaration const*, CallableDeclaration const*)> compareById =
+		[](auto const* _a, auto const* _b) { return _a->id() < _b->id(); };
 
-
-
-	// 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();)
 	{
-		std::function<bool(CallableDeclaration const*, CallableDeclaration const*)> compareById = [](auto const* a, auto const* b) { return a->id() < b->id(); };
+		// Fetch inherited functions and sort them by signature.
-		std::set<CallableDeclaration const*, std::function<bool(CallableDeclaration const*, CallableDeclaration const*)>> baseFunctions(compareById);
+		// We get at least one function per signature and direct base contract, which is
-		for (auto nextSignature = nonOverriddenFunctions.upper_bound(*it); it != nextSignature; ++it)
+		// enough because we re-construct the inheritance graph later.
-			baseFunctions.insert(*it);
+		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();)
+		{
+			std::set<CallableDeclaration const*, decltype(compareById)> baseFunctions(compareById);
+			for (auto nextSignature = nonOverriddenFunctions.upper_bound(*it); it != nextSignature; ++it)
+				baseFunctions.insert(*it);
+
+			checkAmbiguousOverridesInternal(std::move(baseFunctions), _contract.location());
+		}
+	}
+
+	{
+		ModifierMultiSet modifiers = inheritedModifiers(_contract);
+		modifiers -= _contract.functionModifiers();
+		for (auto it = modifiers.cbegin(); it != modifiers.cend();)
+		{
+			std::set<CallableDeclaration const*, decltype(compareById)> baseModifiers(compareById);
+			for (auto next = modifiers.upper_bound(*it); it != next; ++it)
+				baseModifiers.insert(*it);
+
+			checkAmbiguousOverridesInternal(std::move(baseModifiers), _contract.location());
+		}
 
-		checkAmbiguousOverridesInternal(std::move(baseFunctions), _contract.location());
 	}
 }
 
@@ -869,10 +883,17 @@ void ContractLevelChecker::checkAmbiguousOverridesInternal(set<
 	}
 
 	string callableName;
+	string distinguishigProperty;
 	if (dynamic_cast<FunctionDefinition const*>(*_baseCallables.begin()))
+	{
 		callableName = "function";
+		distinguishigProperty = "name and parameter types";
+	}
 	else if (dynamic_cast<ModifierDefinition const*>(*_baseCallables.begin()))
+	{
 		callableName = "modifier";
+		distinguishigProperty = "name";
+	}
 	else
 		solAssert(false, "Invalid type for ambiguous override.");
 
@@ -881,7 +902,7 @@ void ContractLevelChecker::checkAmbiguousOverridesInternal(set<
 		ssl,
 		"Derived contract must override " + callableName + " \"" +
 		(*_baseCallables.begin())->name() +
-		"\". Two or more base classes define " + callableName + " with same name and parameter types."
+		"\". Two or more base classes define " + callableName + " with same " + distinguishigProperty + "."
 	);
 }
 

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

@@ -7,4 +7,4 @@ contract B {
 contract C is A, B {
 }
 // ----
-// THIS NEEDS TO BE AN ERROR
+// TypeError: (94-116): Derived contract must override modifier "f". Two or more base classes define modifier with same name.

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

@@ -0,0 +1,10 @@
+contract A {
+    modifier f(uint a) virtual { _; }
+}
+contract B {
+    modifier f() virtual { _; }
+}
+contract C is A, B {
+}
+// ----
+// TypeError: (100-122): Derived contract must override modifier "f". Two or more base classes define modifier with same name.

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

@@ -0,0 +1,11 @@
+contract A {
+    modifier f(uint a) virtual { _; }
+}
+contract B {
+    modifier f() virtual { _; }
+}
+contract C is A, B {
+    modifier f() virtual override(A, B) { _; }
+}
+// ----
+// TypeError: (125-167): Override changes modifier signature.

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

@@ -1,8 +1,8 @@
 contract I {
-	modifier f() { _; }
+	modifier f() virtual { _; }
 }
 contract J {
-	modifier f() { _; }
+	modifier f() virtual { _; }
 }
 contract IJ is I, J {
 	modifier f() virtual override (I, J) { _; }
@@ -16,5 +16,4 @@ 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: (229-250): Derived contract must override modifier "f". Two or more base classes define modifier with same name.
-// TypeError: (50-69): Trying to override non-virtual modifier. Did you forget to add "virtual"?