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

@@ -430,7 +430,8 @@ bool TypeChecker::visit(FunctionDefinition const& _function)
 	for (ASTPointer<ModifierInvocation> const& modifier: _function.modifiers())
 	{
 		vector<ContractDefinition const*> baseContracts;
-		if (auto contract = dynamic_cast<ContractDefinition const*>(_function.scope()))
+		auto contract = dynamic_cast<ContractDefinition const*>(_function.scope());
+		if (contract)
 		{
 			baseContracts = contract->annotation().linearizedBaseContracts;
 			// Delete first base which is just the main contract itself
@@ -439,6 +440,7 @@ bool TypeChecker::visit(FunctionDefinition const& _function)
 
 		visitManually(
 			*modifier,
+			contract,
 			_function.isConstructor() ? baseContracts : vector<ContractDefinition const*>()
 		);
 		Declaration const* decl = &dereference(modifier->name());
@@ -613,6 +615,7 @@ bool TypeChecker::visit(VariableDeclaration const& _variable)
 
 void TypeChecker::visitManually(
 	ModifierInvocation const& _modifier,
+	ContractDefinition const* _contract,
 	vector<ContractDefinition const*> const& _bases
 )
 {
@@ -630,6 +633,7 @@ void TypeChecker::visitManually(
 	{
 		parameters = &modifierDecl->parameters();
 		if (auto const* modifierContract = dynamic_cast<ContractDefinition const*>(modifierDecl->scope()))
+		{
 			if (m_currentContract)
 			{
 				if (!contains(m_currentContract->annotation().linearizedBaseContracts, modifierContract))
@@ -639,6 +643,17 @@ void TypeChecker::visitManually(
 						"Can only use modifiers defined in the current contract or in base contracts."
 					);
 			}
+
+			if (!modifierDecl->isImplemented() && !_contract->abstract())
+			{
+				m_errorReporter.typeError(
+					9113_error,
+					_modifier.location(),
+					"Cannot call unimplemented modifier."
+				);
+			}
+		}
+
 	}
 	else
 		// check parameters for Base constructors

libsolidity/analysis/TypeChecker.h

@@ -122,7 +122,7 @@ class TypeChecker: private ASTConstVisitor
 	bool visit(VariableDeclaration const& _variable) override;
 	/// We need to do this manually because we want to pass the bases of the current contract in
 	/// case this is a base constructor call.
-	void visitManually(ModifierInvocation const& _modifier, std::vector<ContractDefinition const*> const& _bases);
+	void visitManually(ModifierInvocation const& _modifier, ContractDefinition const* _contract, std::vector<ContractDefinition const*> const& _bases);
 	bool visit(EventDefinition const& _eventDef) override;
 	bool visit(ErrorDefinition const& _errorDef) override;
 	void endVisit(FunctionTypeName const& _funType) override;

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.