Reports an error on unimplemented modifiers in a function definition.

Author: christianparpart

Changelog.md

@@ -30,6 +30,7 @@ Bugfixes:
  * Type Checker: Fix internal compiler error when attempting to use an invalid external function type on pre-byzantium EVMs.
  * Type Checker: Make errors about (nested) mapping type in event or error parameter into fatal type errors.
  * Type Checker: Fix internal compiler error when overriding receive ether function with one having different parameters during inheritance.
+ * Type Checker: Fix internal compiler error when overriding an implemented modifier with an unimplemented one.
 
 
 AST Changes:

libsolidity/analysis/OverrideChecker.cpp

@@ -573,6 +573,19 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
 			);
 	}
 
+	if (_overriding.unimplemented() && !_super.unimplemented())
+	{
+		solAssert(!_overriding.isVariable() || !_overriding.unimplemented(), "");
+		overrideError(
+			_overriding,
+			_super,
+			4593_error,
+			"Overriding an implemented " + _super.astNodeName() +
+			" with an unimplemented " + _overriding.astNodeName() +
+			" is not allowed."
+		);
+	}
+
 	if (_super.isFunction())
 	{
 		FunctionType const* functionType = _overriding.functionType();
@@ -613,14 +626,6 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr
 				stateMutabilityToString(_overriding.stateMutability()) +
 				"\"."
 			);
-
-		if (_overriding.unimplemented() && !_super.unimplemented())
-			overrideError(
-				_overriding,
-				_super,
-				4593_error,
-				"Overriding an implemented function with an unimplemented function is not allowed."
-			);
 	}
 }
 

libsolidity/analysis/TypeChecker.cpp

@@ -629,16 +629,30 @@ void TypeChecker::visitManually(
 	if (auto modifierDecl = dynamic_cast<ModifierDefinition const*>(declaration))
 	{
 		parameters = &modifierDecl->parameters();
-		if (auto const* modifierContract = dynamic_cast<ContractDefinition const*>(modifierDecl->scope()))
-			if (m_currentContract)
+		solAssert(modifierDecl->annotation().contract, "");
+		auto const* modifierContract = dynamic_cast<ContractDefinition const*>(modifierDecl->scope());
+		solAssert(modifierContract, "");
+		if (m_currentContract)
+		{
+			if (!contains(m_currentContract->annotation().linearizedBaseContracts, modifierContract))
+				m_errorReporter.typeError(
+				9428_error,
+					_modifier.location(),
+					"Can only use modifiers defined in the current contract or in base contracts."
+				);
+
+			if (m_currentContract->derivesFrom(*modifierDecl->annotation().contract) &&
+				!modifierDecl->isImplemented()
+			)
 			{
-				if (!contains(m_currentContract->annotation().linearizedBaseContracts, modifierContract))
-					m_errorReporter.typeError(
-						9428_error,
-						_modifier.location(),
-						"Can only use modifiers defined in the current contract or in base contracts."
-					);
+				m_errorReporter.typeError(
+					9113_error,
+					_modifier.location(),
+					"Cannot call unimplemented modifier."
+				);
 			}
+		}
+
 	}
 	else
 		// check parameters for Base constructors

test/libsolidity/syntaxTests/inheritance/virtual/modifier_virtual_err.sol

@@ -5,3 +5,4 @@ library test {
 }
 // ----
 // TypeError 3275: (19-38): Modifiers in a library cannot be virtual.
+// TypeError 9113: (56-57): Cannot call unimplemented modifier.

test/libsolidity/syntaxTests/modifiers/modifier_abstract_override.sol

@@ -0,0 +1,12 @@
+contract A {
+    modifier m() virtual { _; }
+}
+abstract contract B is A {
+    modifier m() virtual override;
+}
+contract C is B {
+    function f() m public {}
+}
+// ----
+// TypeError 4593: (78-108): Overriding an implemented modifier with an unimplemented modifier is not allowed.
+// TypeError 9113: (146-147): Cannot call unimplemented modifier.

test/libsolidity/syntaxTests/modifiers/modifier_call_unimplemented_1.sol

@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >= 0.5.0;
+
+contract A {
+    modifier m() virtual { _; }
+}
+abstract contract B {
+    modifier m() virtual;
+}
+contract C is A, B {
+    modifier m() override(A, B) { _; }
+    function f() B.m public {}
+}
+// ----
+// TypeError 9113: (240-243): Cannot call unimplemented modifier.

test/libsolidity/syntaxTests/modifiers/modifier_call_unimplemented_2.sol

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >= 0.5.0;
+
+abstract contract A {
+    modifier m() virtual;
+}
+contract B is A {
+    modifier m() virtual override { _; }
+}
+contract C is B {
+    function f() A.m public {}
+}
+// ----
+// TypeError 9113: (212-215): Cannot call unimplemented modifier.