Merge pull request #8659 from ethereum/empty-modifier

Allow modifier declaration without implementation

Changelog.md

@@ -2,6 +2,7 @@
 
 Language Features:
  * Add support for EIP 165 interface identifiers with `type(I).interfaceId`.
+ * Allow virtual modifiers inside abstract contracts to have empty body.
 
 
 Compiler Features:

docs/Solidity.g4

@@ -68,7 +68,7 @@ structDefinition
     '{' ( variableDeclaration ';' (variableDeclaration ';')* )? '}' ;
 
 modifierDefinition
-  : 'modifier' identifier parameterList? ( VirtualKeyword | overrideSpecifier )* block ;
+  : 'modifier' identifier parameterList? ( VirtualKeyword | overrideSpecifier )* ( ';' | block ) ;
 
 functionDefinition
   : functionDescriptor parameterList modifierList returnParameters? ( ';' | block ) ;

libsolidity/analysis/ContractLevelChecker.cpp

@@ -55,7 +55,7 @@ bool ContractLevelChecker::check(ContractDefinition const& _contract)
 	checkDuplicateEvents(_contract);
 	m_overrideChecker.check(_contract);
 	checkBaseConstructorArguments(_contract);
-	checkAbstractFunctions(_contract);
+	checkAbstractDefinitions(_contract);
 	checkExternalTypeClashes(_contract);
 	checkHashCollisions(_contract);
 	checkLibraryRequirements(_contract);
@@ -156,55 +156,43 @@ void ContractLevelChecker::findDuplicateDefinitions(map<string, vector<T>> const
 	}
 }
 
-void ContractLevelChecker::checkAbstractFunctions(ContractDefinition const& _contract)
+void ContractLevelChecker::checkAbstractDefinitions(ContractDefinition const& _contract)
 {
-	// Mapping from name to function definition (exactly one per argument type equality class) and
+	// Collects functions, static variable getters and modifiers. If they
-	// flag to indicate whether it is fully implemented.
+	// override (unimplemented) base class ones, they are replaced.
-	using FunTypeAndFlag = std::pair<FunctionTypePointer, bool>;
+	set<OverrideProxy, OverrideProxy::CompareBySignature> proxies;
-	map<string, vector<FunTypeAndFlag>> functions;
 
-	auto registerFunction = [&](Declaration const& _declaration, FunctionTypePointer const& _type, bool _implemented)
+	auto registerProxy = [&proxies](OverrideProxy const& _overrideProxy)
 	{
-		auto& overloads = functions[_declaration.name()];
+		// Overwrite an existing proxy, if it exists.
-		auto it = find_if(overloads.begin(), overloads.end(), [&](FunTypeAndFlag const& _funAndFlag)
+		if (!_overrideProxy.unimplemented())
-		{
+			proxies.erase(_overrideProxy);
-			return _type->hasEqualParameterTypes(*_funAndFlag.first);
+
-		});
+		proxies.insert(_overrideProxy);
-		if (it == overloads.end())
-			overloads.emplace_back(_type, _implemented);
-		else if (_implemented)
-			it->second = true;
 	};
 
-	// Search from base to derived, collect all functions and update
+	// Search from base to derived, collect all functions and modifiers and
-	// the 'implemented' flag.
+	// update proxies.
 	for (ContractDefinition const* contract: boost::adaptors::reverse(_contract.annotation().linearizedBaseContracts))
 	{
 		for (VariableDeclaration const* v: contract->stateVariables())
 			if (v->isPartOfExternalInterface())
-				registerFunction(*v, TypeProvider::function(*v), true);
+				registerProxy(OverrideProxy(v));
 
 		for (FunctionDefinition const* function: contract->definedFunctions())
 			if (!function->isConstructor())
-				registerFunction(
+				registerProxy(OverrideProxy(function));
-					*function,
+
-					TypeProvider::function(*function)->asCallableFunction(false),
+		for (ModifierDefinition const* modifier: contract->functionModifiers())
-					function->isImplemented()
+			registerProxy(OverrideProxy(modifier));
-				);
 	}
 
 	// Set to not fully implemented if at least one flag is false.
-	// Note that `_contract.annotation().unimplementedFunctions` has already been
+	// Note that `_contract.annotation().unimplementedDeclarations` has already been
 	// pre-filled by `checkBaseConstructorArguments`.
-	for (auto const& it: functions)
+	for (auto const& proxy: proxies)
-		for (auto const& funAndFlag: it.second)
+		if (proxy.unimplemented())
-			if (!funAndFlag.second)
+			_contract.annotation().unimplementedDeclarations.push_back(proxy.declaration());
-			{
-				FunctionDefinition const* function = dynamic_cast<FunctionDefinition const*>(&funAndFlag.first->declaration());
-				solAssert(function, "");
-				_contract.annotation().unimplementedFunctions.push_back(function);
-				break;
-			}
 
 	if (_contract.abstract())
 	{
@@ -221,15 +209,16 @@ void ContractLevelChecker::checkAbstractFunctions(ContractDefinition const& _con
 	if (
 		_contract.contractKind() == ContractKind::Contract &&
 		!_contract.abstract() &&
-		!_contract.annotation().unimplementedFunctions.empty()
+		!_contract.annotation().unimplementedDeclarations.empty()
 	)
 	{
 		SecondarySourceLocation ssl;
-		for (auto function: _contract.annotation().unimplementedFunctions)
+		for (auto declaration: _contract.annotation().unimplementedDeclarations)
-			ssl.append("Missing implementation:", function->location());
+			ssl.append("Missing implementation: ", declaration->location());
 		m_errorReporter.typeError(_contract.location(), ssl,
 			"Contract \"" + _contract.annotation().canonicalName
 			+ "\" should be marked as abstract.");
+
 	}
 }
 
@@ -277,7 +266,7 @@ void ContractLevelChecker::checkBaseConstructorArguments(ContractDefinition cons
 		if (FunctionDefinition const* constructor = contract->constructor())
 			if (contract != &_contract && !constructor->parameters().empty())
 				if (!_contract.annotation().baseConstructorArguments.count(constructor))
-					_contract.annotation().unimplementedFunctions.push_back(constructor);
+					_contract.annotation().unimplementedDeclarations.push_back(constructor);
 }
 
 void ContractLevelChecker::annotateBaseConstructorArguments(

libsolidity/analysis/ContractLevelChecker.h

@@ -61,7 +61,8 @@ private:
 	void checkDuplicateEvents(ContractDefinition const& _contract);
 	template <class T>
 	void findDuplicateDefinitions(std::map<std::string, std::vector<T>> const& _definitions, std::string _message);
-	void checkAbstractFunctions(ContractDefinition const& _contract);
+	/// Checks for unimplemented functions and modifiers.
+	void checkAbstractDefinitions(ContractDefinition const& _contract);
 	/// Checks that the base constructor arguments are properly provided.
 	/// Fills the list of unimplemented functions in _contract's annotations.
 	void checkBaseConstructorArguments(ContractDefinition const& _contract);

libsolidity/analysis/ImmutableValidator.cpp

@@ -148,6 +148,7 @@ bool ImmutableValidator::analyseCallable(CallableDeclaration const& _callableDec
 			funcDef->body().accept(*this);
 	}
 	else if (ModifierDefinition const* modDef = dynamic_cast<decltype(modDef)>(&_callableDeclaration))
+		if (modDef->isImplemented())
 			modDef->body().accept(*this);
 
 	m_currentConstructor = prevConstructor;

libsolidity/analysis/OverrideChecker.cpp

@@ -327,6 +327,16 @@ ModifierType const* OverrideProxy::modifierType() const
 	}, m_item);
 }
 
+
+Declaration const* OverrideProxy::declaration() const
+{
+	return std::visit(GenericVisitor{
+		[&](FunctionDefinition const* _function) -> Declaration const* { return _function; },
+		[&](VariableDeclaration const* _variable) -> Declaration const* { return _variable; },
+		[&](ModifierDefinition const* _modifier) -> Declaration const* { return _modifier; }
+	}, m_item);
+}
+
 SourceLocation const& OverrideProxy::location() const
 {
 	return std::visit(GenericVisitor{
@@ -365,7 +375,7 @@ bool OverrideProxy::unimplemented() const
 {
 	return std::visit(GenericVisitor{
 		[&](FunctionDefinition const* _item) { return !_item->isImplemented(); },
-		[&](ModifierDefinition const*) { return false; },
+		[&](ModifierDefinition const* _item) { return !_item->isImplemented(); },
 		[&](VariableDeclaration const*) { return false; }
 	}, m_item);
 }

libsolidity/analysis/OverrideChecker.h

@@ -85,6 +85,8 @@ public:
 	FunctionType const* functionType() const;
 	ModifierType const* modifierType() const;
 
+	Declaration const* declaration() const;
+
 	langutil::SourceLocation const& location() const;
 
 	std::string astNodeName() const;

libsolidity/analysis/SyntaxChecker.cpp

@@ -141,7 +141,7 @@ bool SyntaxChecker::visit(ModifierDefinition const&)
 
 void SyntaxChecker::endVisit(ModifierDefinition const& _modifier)
 {
-	if (!m_placeholderFound)
+	if (_modifier.isImplemented() && !m_placeholderFound)
 		m_errorReporter.syntaxError(_modifier.body().location(), "Modifier body does not contain '_'.");
 	m_placeholderFound = false;
 }

libsolidity/analysis/TypeChecker.cpp

@@ -289,6 +289,12 @@ void TypeChecker::endVisit(UsingForDirective const& _usingFor)
 		m_errorReporter.fatalTypeError(_usingFor.libraryName().location(), "Library name expected.");
 }
 
+void TypeChecker::endVisit(ModifierDefinition const& _modifier)
+{
+	if (!_modifier.isImplemented() && !_modifier.virtualSemantics())
+		m_errorReporter.typeError(_modifier.location(), "Modifiers without implementation must be marked virtual.");
+}
+
 bool TypeChecker::visit(FunctionDefinition const& _function)
 {
 	bool isLibraryFunction = _function.inContractKind() == ContractKind::Library;

libsolidity/analysis/TypeChecker.h

@@ -112,6 +112,7 @@ private:
 
 	void endVisit(InheritanceSpecifier const& _inheritance) override;
 	void endVisit(UsingForDirective const& _usingFor) override;
+	void endVisit(ModifierDefinition const& _modifier) override;
 	bool visit(FunctionDefinition const& _function) override;
 	bool visit(VariableDeclaration const& _variable) override;
 	/// We need to do this manually because we want to pass the bases of the current contract in

libsolidity/ast/AST.h

@@ -969,7 +969,7 @@ private:
 /**
  * Definition of a function modifier.
  */
-class ModifierDefinition: public CallableDeclaration, public StructurallyDocumented
+class ModifierDefinition: public CallableDeclaration, public StructurallyDocumented, public ImplementationOptional
 {
 public:
 	ModifierDefinition(
@@ -980,18 +980,19 @@ public:
 		ASTPointer<ParameterList> const& _parameters,
 		bool _isVirtual,
 		ASTPointer<OverrideSpecifier> const& _overrides,
-		ASTPointer<Block> _body
+		ASTPointer<Block> const& _body
 	):
 		CallableDeclaration(_id, _location, _name, Visibility::Internal, _parameters, _isVirtual, _overrides),
 		StructurallyDocumented(_documentation),
-		m_body(std::move(_body))
+		ImplementationOptional(_body != nullptr),
+		m_body(_body)
 	{
 	}
 
 	void accept(ASTVisitor& _visitor) override;
 	void accept(ASTConstVisitor& _visitor) const override;
 
-	Block const& body() const { return *m_body; }
+	Block const& body() const { solAssert(m_body, ""); return *m_body; }
 
 	TypePointer type() const override;
 

libsolidity/ast/ASTAnnotations.h

@@ -140,8 +140,8 @@ struct StructDeclarationAnnotation: TypeDeclarationAnnotation
 
 struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, StructurallyDocumentedAnnotation
 {
-	/// List of functions without a body. Can also contain functions from base classes.
+	/// List of functions and modifiers without a body. Can also contain functions from base classes.
-	std::vector<FunctionDefinition const*> unimplementedFunctions;
+	std::vector<Declaration const*> unimplementedDeclarations;
 	/// List of all (direct and indirect) base contracts in order from derived to
 	/// base, including the contract itself.
 	std::vector<ContractDefinition const*> linearizedBaseContracts;

libsolidity/ast/ASTJsonConverter.cpp

@@ -272,7 +272,7 @@ bool ASTJsonConverter::visit(ContractDefinition const& _node)
 		make_pair("documentation", _node.documentation() ? toJson(*_node.documentation()) : Json::nullValue),
 		make_pair("contractKind", contractKind(_node.contractKind())),
 		make_pair("abstract", _node.abstract()),
-		make_pair("fullyImplemented", _node.annotation().unimplementedFunctions.empty()),
+		make_pair("fullyImplemented", _node.annotation().unimplementedDeclarations.empty()),
 		make_pair("linearizedBaseContracts", getContainerIds(_node.annotation().linearizedBaseContracts)),
 		make_pair("baseContracts", toJson(_node.baseContracts())),
 		make_pair("contractDependencies", getContainerIds(_node.annotation().contractDependencies, true)),
@@ -407,7 +407,7 @@ bool ASTJsonConverter::visit(ModifierDefinition const& _node)
 		make_pair("parameters", toJson(_node.parameterList())),
 		make_pair("virtual", _node.markedVirtual()),
 		make_pair("overrides", _node.overrides() ? toJson(*_node.overrides()) : Json::nullValue),
-		make_pair("body", toJson(_node.body()))
+		make_pair("body", _node.isImplemented() ? toJson(_node.body()) : Json::nullValue)
 	};
 	if (!_node.annotation().baseFunctions.empty())
 		attributes.emplace_back(make_pair("baseModifiers", getContainerIds(_node.annotation().baseFunctions, true)));

libsolidity/ast/ASTJsonImporter.cpp

@@ -453,7 +453,7 @@ ASTPointer<ModifierDefinition> ASTJsonImporter::createModifierDefinition(Json::V
 		createParameterList(member(_node, "parameters")),
 		memberAsBool(_node, "virtual"),
 		_node["overrides"].isNull() ? nullptr : createOverrideSpecifier(member(_node, "overrides")),
-		createBlock(member(_node, "body"))
+		_node["body"].isNull() ? nullptr: createBlock(member(_node, "body"))
 	);
 }
 

libsolidity/ast/AST_accept.h

@@ -288,6 +288,7 @@ void ModifierDefinition::accept(ASTVisitor& _visitor)
 		m_parameters->accept(_visitor);
 		if (m_overrides)
 			m_overrides->accept(_visitor);
+		if (m_body)
 			m_body->accept(_visitor);
 	}
 	_visitor.endVisit(*this);
@@ -302,6 +303,7 @@ void ModifierDefinition::accept(ASTConstVisitor& _visitor) const
 		m_parameters->accept(_visitor);
 		if (m_overrides)
 			m_overrides->accept(_visitor);
+		if (m_body)
 			m_body->accept(_visitor);
 	}
 	_visitor.endVisit(*this);

libsolidity/ast/Types.h

@@ -1390,7 +1390,6 @@ public:
 	std::string richIdentifier() const override;
 	bool operator==(Type const& _other) const override;
 	std::string toString(bool _short) const override;
-
 protected:
 	std::vector<std::tuple<std::string, TypePointer>> makeStackItems() const override { return {}; }
 private:

libsolidity/formal/SMTEncoder.cpp

@@ -194,6 +194,7 @@ void SMTEncoder::inlineModifierInvocation(ModifierInvocation const* _invocation,
 	pushCallStack({_definition, _invocation});
 	if (auto modifier = dynamic_cast<ModifierDefinition const*>(_definition))
 	{
+		if (modifier->isImplemented())
 			modifier->body().accept(*this);
 		popCallStack();
 	}

libsolidity/parsing/Parser.cpp

@@ -595,13 +595,13 @@ ASTPointer<ASTNode> Parser::parseFunctionDefinition()
 
 	ASTPointer<Block> block;
 	nodeFactory.markEndPosition();
-	if (m_scanner->currentToken() != Token::Semicolon)
+	if (m_scanner->currentToken() == Token::Semicolon)
+		m_scanner->next();
+	else
 	{
 		block = parseBlock();
 		nodeFactory.setEndPositionFromNode(block);
 	}
-	else
-		m_scanner->next(); // just consume the ';'
 	return nodeFactory.createNode<FunctionDefinition>(
 		name,
 		header.visibility,
@@ -851,9 +851,16 @@ ASTPointer<ModifierDefinition> Parser::parseModifierDefinition()
 			break;
 	}
 
-
+	ASTPointer<Block> block;
-	ASTPointer<Block> block = parseBlock();
+	nodeFactory.markEndPosition();
+	if (m_scanner->currentToken() != Token::Semicolon)
+	{
+		block = parseBlock();
 		nodeFactory.setEndPositionFromNode(block);
+	}
+	else
+		m_scanner->next(); // just consume the ';'
+
 	return nodeFactory.createNode<ModifierDefinition>(name, documentation, parameters, isVirtual, overrides, block);
 }
 

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(function_no_implementation)
 	std::vector<ASTPointer<ASTNode>> nodes = sourceUnit->nodes();
 	ContractDefinition* contract = dynamic_cast<ContractDefinition*>(nodes[1].get());
 	BOOST_REQUIRE(contract);
-	BOOST_CHECK(!contract->annotation().unimplementedFunctions.empty());
+	BOOST_CHECK(!contract->annotation().unimplementedDeclarations.empty());
 	BOOST_CHECK(!contract->definedFunctions()[0]->isImplemented());
 }
 
@@ -68,10 +68,10 @@ BOOST_AUTO_TEST_CASE(abstract_contract)
 	ContractDefinition* base = dynamic_cast<ContractDefinition*>(nodes[1].get());
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[2].get());
 	BOOST_REQUIRE(base);
-	BOOST_CHECK(!base->annotation().unimplementedFunctions.empty());
+	BOOST_CHECK(!base->annotation().unimplementedDeclarations.empty());
 	BOOST_CHECK(!base->definedFunctions()[0]->isImplemented());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(derived->annotation().unimplementedFunctions.empty());
+	BOOST_CHECK(derived->annotation().unimplementedDeclarations.empty());
 	BOOST_CHECK(derived->definedFunctions()[0]->isImplemented());
 }
 
@@ -87,9 +87,9 @@ BOOST_AUTO_TEST_CASE(abstract_contract_with_overload)
 	ContractDefinition* base = dynamic_cast<ContractDefinition*>(nodes[1].get());
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[2].get());
 	BOOST_REQUIRE(base);
-	BOOST_CHECK(!base->annotation().unimplementedFunctions.empty());
+	BOOST_CHECK(!base->annotation().unimplementedDeclarations.empty());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty());
+	BOOST_CHECK(!derived->annotation().unimplementedDeclarations.empty());
 }
 
 BOOST_AUTO_TEST_CASE(implement_abstract_via_constructor)
@@ -104,7 +104,7 @@ BOOST_AUTO_TEST_CASE(implement_abstract_via_constructor)
 	BOOST_CHECK_EQUAL(nodes.size(), 3);
 	ContractDefinition* derived = dynamic_cast<ContractDefinition*>(nodes[2].get());
 	BOOST_REQUIRE(derived);
-	BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty());
+	BOOST_CHECK(!derived->annotation().unimplementedDeclarations.empty());
 }
 
 BOOST_AUTO_TEST_CASE(function_canonical_signature)

test/libsolidity/semanticTests/modifiers/function_modifier_empty.sol

@@ -0,0 +1,17 @@
+abstract contract A {
+    function f() public mod returns (bool r) {
+        return true;
+    }
+
+    modifier mod virtual;
+}
+
+
+contract C is A {
+    modifier mod override {
+        if (false) _;
+    }
+}
+
+// ----
+// f() -> false

test/libsolidity/syntaxTests/modifiers/empty_modifier_body.sol

@@ -0,0 +1,14 @@
+abstract contract A { modifier mod(uint a) virtual;}
+contract B is A { modifier mod(uint a) override { _; } }
+
+abstract contract C {
+	modifier m virtual;
+	function f() m public {
+
+	}
+}
+contract D is C {
+	modifier m override {
+		_;
+	}
+}

test/libsolidity/syntaxTests/modifiers/empty_modifier_err.sol

@@ -0,0 +1,10 @@
+contract A {modifier m virtual;}
+
+abstract contract B {modifier m virtual;}
+contract C is B { }
+
+abstract contract D {modifier m;}
+// ----
+// TypeError: (0-32): Contract "A" should be marked as abstract.
+// TypeError: (76-95): Contract "C" should be marked as abstract.
+// TypeError: (118-129): Modifiers without implementation must be marked virtual.

test/libsolidity/syntaxTests/modifiers/unimplemented_function_and_modifier.sol

@@ -0,0 +1,31 @@
+abstract contract A {
+  function foo() public virtual;
+  function foo(uint x) virtual public returns(uint);
+  modifier mod() virtual;
+}
+
+contract B is A {
+  function foo(uint x) override public returns(uint) {return x;}
+  modifier mod() override { _; }
+}
+
+contract C is A {
+  function foo() public override {}
+  modifier mod() override { _; }
+}
+
+contract D is A {
+  function foo() public override {}
+  function foo(uint x) override public returns(uint) {return x;}
+}
+
+/* No errors */
+contract E is A {
+  function foo() public override {}
+  function foo(uint x) override public returns(uint) {return x;}
+  modifier mod() override { _;}
+}
+// ----
+// TypeError: (137-254): Contract "B" should be marked as abstract.
+// TypeError: (256-344): Contract "C" should be marked as abstract.
+// TypeError: (346-466): Contract "D" should be marked as abstract.

tools/solidityUpgrade/Upgrade060.cpp

@@ -99,7 +99,7 @@ inline string appendVirtual(FunctionDefinition const& _function)
 
 void AbstractContract::endVisit(ContractDefinition const& _contract)
 {
-	bool isFullyImplemented = _contract.annotation().unimplementedFunctions.empty();
+	bool isFullyImplemented = _contract.annotation().unimplementedDeclarations.empty();
 
 	if (
 		!isFullyImplemented &&