Improved code generation for simple modifiers.

libsolidity/analysis/SyntaxChecker.cpp

@@ -179,6 +179,7 @@ bool SyntaxChecker::visit(PragmaDirective const& _pragma)
 bool SyntaxChecker::visit(ModifierDefinition const&)
 {
 	m_placeholderFound = false;
+	m_modifierIsSimple = true;
 	return true;
 }
 
@@ -186,6 +187,17 @@ void SyntaxChecker::endVisit(ModifierDefinition const& _modifier)
 {
 	if (_modifier.isImplemented() && !m_placeholderFound)
 		m_errorReporter.syntaxError(2883_error, _modifier.body().location(), "Modifier body does not contain '_'.");
+
+	if (!m_placeholderFound)
+		m_modifierIsSimple = false;
+	if (_modifier.isImplemented() && (
+		_modifier.body().statements().empty() ||
+		!dynamic_cast<PlaceholderStatement const*>(_modifier.body().statements().back().get())
+	))
+		m_modifierIsSimple = false;
+	if (m_modifierIsSimple)
+		_modifier.annotation().simpleModifier = true;
+
 	m_placeholderFound = false;
 }
 
@@ -266,6 +278,12 @@ bool SyntaxChecker::visit(Break const& _breakStatement)
 	return true;
 }
 
+bool SyntaxChecker::visit(Return const&)
+{
+	m_modifierIsSimple = false;
+	return true;
+}
+
 bool SyntaxChecker::visit(Throw const& _throwStatement)
 {
 	m_errorReporter.syntaxError(
@@ -357,6 +375,10 @@ bool SyntaxChecker::visit(PlaceholderStatement const& _placeholder)
 			"The placeholder statement \"_\" cannot be used inside an \"unchecked\" block."
 		);
 
+	// Two placeholders make it non-simple.
+	if (m_placeholderFound)
+		m_modifierIsSimple = false;
+
 	m_placeholderFound = true;
 	return true;
 }

libsolidity/analysis/SyntaxChecker.h

@@ -77,6 +77,7 @@ private:
 
 	bool visit(Continue const& _continueStatement) override;
 	bool visit(Break const& _breakStatement) override;
+	bool visit(Return const& _returnStatement) override;
 
 	bool visit(Throw const& _throwStatement) override;
 
@@ -100,6 +101,9 @@ private:
 
 	/// Flag that indicates whether a function modifier actually contains '_'.
 	bool m_placeholderFound = false;
+	/// Indicates that the current modifier is "simple" which means it does not contain
+	/// a "return" statement and the only placeholder is the last statement.
+	bool m_modifierIsSimple = false;
 
 	/// Flag that indicates whether some version pragma was present.
 	bool m_versionPragmaFound = false;

libsolidity/ast/ASTAnnotations.h

@@ -192,6 +192,9 @@ struct ErrorDefinitionAnnotation: CallableDeclarationAnnotation, StructurallyDoc
 
 struct ModifierDefinitionAnnotation: CallableDeclarationAnnotation, StructurallyDocumentedAnnotation
 {
+	/// If true, indicates that the modifier does not contain a "return" statement and only
+	/// a single `_` (placeholder) as the last statament. This means it can be called as a function.
+	bool simpleModifier = false;
 };
 
 struct VariableDeclarationAnnotation: DeclarationAnnotation, StructurallyDocumentedAnnotation

libsolidity/codegen/CompilerContext.cpp

@@ -601,6 +601,12 @@ evmasm::AssemblyItem CompilerContext::FunctionCompilationQueue::entryLabel(
 			params = CompilerUtils::sizeOnStack(functionType.parameterTypes());
 			returns = CompilerUtils::sizeOnStack(functionType.returnParameterTypes());
 		}
+		else if (auto const* modifier = dynamic_cast<ModifierDefinition const*>(&_declaration))
+		{
+			solAssert(modifier->annotation().simpleModifier, "Can only call simple modifiers as functions.");
+			params = CompilerUtils::sizeOnStack(modifier->parameters());
+		}
+		/// TODO What else can be there apart from function and modifiers?
 
 		// some name that cannot clash with yul function names.
 		string labelName = "@" + _declaration.name() + "_" + to_string(_declaration.id());

libsolidity/codegen/ContractCompiler.cpp

@@ -705,6 +705,62 @@ bool ContractCompiler::visit(FunctionDefinition const& _function)
 	return false;
 }
 
+bool ContractCompiler::visit(ModifierDefinition const& _modifier)
+{
+	solAssert(_modifier.isImplemented(), "");
+	solAssert(_modifier.annotation().simpleModifier, "Direct visit to non-simple modifier.");
+
+	CompilerContext::LocationSetter locationSetter(m_context, _modifier);
+
+	m_context.startFunction(_modifier);
+
+	// stack upon entry: [return address] [arg0] [arg1] ... [argn]
+
+	unsigned parametersSize = CompilerUtils::sizeOnStack(_modifier.parameters());
+	// adding 1 for return address.
+	m_context.setStackOffset(static_cast<int>(parametersSize) + 1);
+
+	for (ASTPointer<VariableDeclaration> const& variable: _modifier.parameters())
+	{
+		m_context.addVariable(*variable, parametersSize);
+		parametersSize -= variable->annotation().type->sizeOnStack();
+	}
+
+	solAssert(m_returnTags.empty(), "");
+	m_breakTags.clear();
+	m_continueTags.clear();
+	m_currentFunction = nullptr;
+	m_scopeStackHeight.clear();
+	m_modifierDepth = 0;
+	m_context.setModifierDepth(0);
+	m_context.setArithmetic(Arithmetic::Checked);
+
+	m_context.setUseABICoderV2(*_modifier.sourceUnit().annotation().useABICoderV2);
+
+	// This should not be used because we should not have a return statement.
+	solAssert(m_returnTags.empty(), "");
+
+	solAssert(m_ignoredPlaceholders == 0);
+	_modifier.body().accept(*this);
+	solAssert(m_ignoredPlaceholders == 1);
+	m_ignoredPlaceholders = 0;
+
+	m_context.setModifierDepth(0);
+
+	CompilerUtils(m_context).popStackSlots(CompilerUtils::sizeOnStack(_modifier.parameters()));
+
+	for (ASTPointer<VariableDeclaration> const& variable: _modifier.parameters())
+		m_context.removeVariable(*variable);
+
+	solAssert(m_context.numberOfLocalVariables() == 0, "");
+
+	m_context.appendJump(evmasm::AssemblyItem::JumpType::OutOfFunction);
+	solAssert(m_context.stackHeight() == 0);
+
+	return false;
+}
+
+
 bool ContractCompiler::visit(InlineAssembly const& _inlineAssembly)
 {
 	unsigned startStackHeight = m_context.stackHeight();
@@ -1280,6 +1336,8 @@ bool ContractCompiler::visit(Break const& _breakStatement)
 
 bool ContractCompiler::visit(Return const& _return)
 {
+	solAssert(m_currentFunction, "Used return inside simple modifier.");
+
 	CompilerContext::LocationSetter locationSetter(m_context, _return);
 	if (Expression const* expression = _return.expression())
 	{
@@ -1381,12 +1439,20 @@ bool ContractCompiler::visit(ExpressionStatement const& _expressionStatement)
 
 bool ContractCompiler::visit(PlaceholderStatement const& _placeholderStatement)
 {
-	StackHeightChecker checker(m_context);
+	if (m_currentFunction)
-	CompilerContext::LocationSetter locationSetter(m_context, _placeholderStatement);
+	{
-	solAssert(m_context.arithmetic() == Arithmetic::Checked, "Placeholder cannot be used inside checked block.");
+		StackHeightChecker checker(m_context);
-	appendModifierOrFunctionCode();
+		CompilerContext::LocationSetter locationSetter(m_context, _placeholderStatement);
-	solAssert(m_context.arithmetic() == Arithmetic::Checked, "Arithmetic not reset to 'checked'.");
+		solAssert(m_context.arithmetic() == Arithmetic::Checked, "Placeholder cannot be used inside checked block.");
-	checker.check();
+		appendModifierOrFunctionCode();
+		solAssert(m_context.arithmetic() == Arithmetic::Checked, "Arithmetic not reset to 'checked'.");
+		checker.check();
+	}
+	else
+		// We are generating code for a simple modifier, so we just register the placeholder but
+		// do not generate any code.
+		m_ignoredPlaceholders++;
+
 	return true;
 }
 
@@ -1416,6 +1482,8 @@ void ContractCompiler::endVisit(Block const& _block)
 
 void ContractCompiler::appendMissingFunctions()
 {
+	// Note that `function` can also be a modifier, but if it is a modifier,
+	// it is a "simple" modifier, i.e. no `return` and only a single `_` at the very end.
 	while (Declaration const* function = m_context.nextFunctionToCompile())
 	{
 		m_context.setStackOffset(0);
@@ -1465,18 +1533,37 @@ void ContractCompiler::appendModifierOrFunctionCode()
 				modifierInvocation->arguments() ? *modifierInvocation->arguments() : std::vector<ASTPointer<Expression>>();
 
 			solAssert(modifier.parameters().size() == modifierArguments.size(), "");
-			for (unsigned i = 0; i < modifier.parameters().size(); ++i)
-			{
-				m_context.addVariable(*modifier.parameters()[i]);
-				addedVariables.push_back(modifier.parameters()[i].get());
-				compileExpression(
-					*modifierArguments[i],
-					modifier.parameters()[i]->annotation().type
-				);
-			}
 
-			stackSurplus = CompilerUtils::sizeOnStack(modifier.parameters());
+			if (modifier.annotation().simpleModifier)
-			codeBlock = &modifier.body();
+			{
+				AssemblyItem returnLabel = m_context.pushNewTag();
+				for (unsigned i = 0; i < modifier.parameters().size(); ++i)
+					compileExpression(
+						*modifierArguments[i],
+						modifier.parameters()[i]->annotation().type
+					);
+				m_context.appendJumpTo(
+					m_context.functionEntryLabel(modifier),
+					evmasm::AssemblyItem::JumpType::IntoFunction
+				);
+				m_context << returnLabel.tag();
+				m_context.adjustStackOffset(-static_cast<int>(CompilerUtils::sizeOnStack(modifier.parameters())) - 1);
+				appendModifierOrFunctionCode();
+			}
+			else
+			{
+				for (unsigned i = 0; i < modifier.parameters().size(); ++i)
+				{
+					m_context.addVariable(*modifier.parameters()[i]);
+					addedVariables.push_back(modifier.parameters()[i].get());
+					compileExpression(
+						*modifierArguments[i],
+						modifier.parameters()[i]->annotation().type
+					);
+				}
+				stackSurplus = CompilerUtils::sizeOnStack(modifier.parameters());
+				codeBlock = &modifier.body();
+			}
 		}
 	}
 

libsolidity/codegen/ContractCompiler.h

@@ -105,6 +105,7 @@ private:
 
 	bool visit(VariableDeclaration const& _variableDeclaration) override;
 	bool visit(FunctionDefinition const& _function) override;
+	bool visit(ModifierDefinition const& _modifier) override;
 	bool visit(InlineAssembly const& _inlineAssembly) override;
 	bool visit(TryStatement const& _tryStatement) override;
 	void handleCatch(std::vector<ASTPointer<TryCatchClause>> const& _catchClauses);
@@ -155,6 +156,8 @@ private:
 	/// Tag to jump to for a "return" statement and the stack height after freeing the local function or modifier variables.
 	/// Needs to be stacked because of modifiers.
 	std::vector<std::pair<evmasm::AssemblyItem, unsigned>> m_returnTags;
+	/// Number of placeholder statements that were ignored since we generated code for a simple modifier.
+	size_t m_ignoredPlaceholders = 0;
 	unsigned m_modifierDepth = 0;
 	FunctionDefinition const* m_currentFunction = nullptr;