Merge pull request #3853 from ethereum/modifierStyleWithoutParentheses

Modifier style constructor calls without parentheses are an error.

Changelog.md

@@ -12,7 +12,8 @@ Features:
  * Static Analyzer: Error on duplicated super constructor calls as experimental 0.5.0 feature.
  * Syntax Checker: Issue warning for empty structs (or error as experimental 0.5.0 feature).
  * General: Introduce new constructor syntax using the ``constructor`` keyword as experimental 0.5.0 feature.
- * Inheritance: Error when using empty parenthesis for base class constructors that require arguments as experimental 0.5.0 feature.
+ * Inheritance: Error when using empty parentheses for base class constructors that require arguments as experimental 0.5.0 feature.
+ * Inheritance: Error when using no parentheses in modifier-style constructor calls as experimental 0.5.0 feature.
 
 Bugfixes:
  * Code Generator: Allow ``block.blockhash`` without being called.

libsolidity/analysis/TypeChecker.cpp

@@ -293,6 +293,8 @@ void TypeChecker::checkContractAbstractFunctions(ContractDefinition const& _cont
 
 void TypeChecker::checkContractBaseConstructorArguments(ContractDefinition const& _contract)
 {
+	bool const v050 = _contract.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);
+
 	vector<ContractDefinition const*> const& bases = _contract.annotation().linearizedBaseContracts;
 
 	// Determine the arguments that are used for the base constructors.
@@ -302,9 +304,25 @@ void TypeChecker::checkContractBaseConstructorArguments(ContractDefinition const
 			for (auto const& modifier: constructor->modifiers())
 			{
 				auto baseContract = dynamic_cast<ContractDefinition const*>(&dereference(*modifier->name()));
-				if (baseContract && baseContract->constructor() && !modifier->arguments().empty())
+				if (modifier->arguments())
+				{
+					if (baseContract && baseContract->constructor())
 						annotateBaseConstructorArguments(_contract, baseContract->constructor(), modifier.get());
 				}
+				else
+				{
+					if (v050)
+						m_errorReporter.declarationError(
+							modifier->location(),
+							"Modifier-style base constructor call without arguments."
+						);
+					else
+						m_errorReporter.warning(
+							modifier->location(),
+							"Modifier-style base constructor call without arguments."
+						);
+				}
+			}
 
 		for (ASTPointer<InheritanceSpecifier> const& base: contract->baseContracts())
 		{
@@ -804,7 +822,8 @@ void TypeChecker::visitManually(
 	vector<ContractDefinition const*> const& _bases
 )
 {
-	std::vector<ASTPointer<Expression>> const& arguments = _modifier.arguments();
+	std::vector<ASTPointer<Expression>> const& arguments =
+		_modifier.arguments() ? *_modifier.arguments() : std::vector<ASTPointer<Expression>>();
 	for (ASTPointer<Expression> const& argument: arguments)
 		argument->accept(*this);
 	_modifier.name()->accept(*this);
@@ -842,7 +861,7 @@ void TypeChecker::visitManually(
 		);
 		return;
 	}
-	for (size_t i = 0; i < _modifier.arguments().size(); ++i)
+	for (size_t i = 0; i < arguments.size(); ++i)
 		if (!type(*arguments[i])->isImplicitlyConvertibleTo(*type(*(*parameters)[i])))
 			m_errorReporter.typeError(
 				arguments[i]->location(),

libsolidity/ast/AST.h

@@ -762,19 +762,22 @@ public:
 	ModifierInvocation(
 		SourceLocation const& _location,
 		ASTPointer<Identifier> const& _name,
-		std::vector<ASTPointer<Expression>> _arguments
+		std::unique_ptr<std::vector<ASTPointer<Expression>>> _arguments
 	):
-		ASTNode(_location), m_modifierName(_name), m_arguments(_arguments) {}
+		ASTNode(_location), m_modifierName(_name), m_arguments(std::move(_arguments)) {}
 
 	virtual void accept(ASTVisitor& _visitor) override;
 	virtual void accept(ASTConstVisitor& _visitor) const override;
 
 	ASTPointer<Identifier> const& name() const { return m_modifierName; }
-	std::vector<ASTPointer<Expression>> const& arguments() const { return m_arguments; }
+	// Returns nullptr if no argument list was given (``mod``).
+	// If an argument list is given (``mod(...)``), the arguments are returned
+	// as a vector of expressions. Note that this vector can be empty (``mod()``).
+	std::vector<ASTPointer<Expression>> const* arguments() const { return m_arguments.get(); }
 
 private:
 	ASTPointer<Identifier> m_modifierName;
-	std::vector<ASTPointer<Expression>> m_arguments;
+	std::unique_ptr<std::vector<ASTPointer<Expression>>> m_arguments;
 };
 
 /**

libsolidity/ast/ASTJsonConverter.cpp

@@ -268,7 +268,7 @@ bool ASTJsonConverter::visit(InheritanceSpecifier const& _node)
 {
 	setJsonNode(_node, "InheritanceSpecifier", {
 		make_pair("baseName", toJson(_node.name())),
-		make_pair("arguments", _node.arguments() ? toJson(*_node.arguments()) : Json::Value(Json::arrayValue))
+		make_pair("arguments", _node.arguments() ? toJson(*_node.arguments()) : Json::nullValue)
 	});
 	return false;
 }
@@ -378,7 +378,7 @@ bool ASTJsonConverter::visit(ModifierInvocation const& _node)
 {
 	setJsonNode(_node, "ModifierInvocation", {
 		make_pair("modifierName", toJson(*_node.name())),
-		make_pair("arguments", toJson(_node.arguments()))
+		make_pair("arguments", _node.arguments() ? toJson(*_node.arguments()) : Json::nullValue)
 	});
 	return false;
 }

libsolidity/ast/AST_accept.h

@@ -264,7 +264,8 @@ void ModifierInvocation::accept(ASTVisitor& _visitor)
 	if (_visitor.visit(*this))
 	{
 		m_modifierName->accept(_visitor);
-		listAccept(m_arguments, _visitor);
+		if (m_arguments)
+			listAccept(*m_arguments, _visitor);
 	}
 	_visitor.endVisit(*this);
 }
@@ -274,7 +275,8 @@ void ModifierInvocation::accept(ASTConstVisitor& _visitor) const
 	if (_visitor.visit(*this))
 	{
 		m_modifierName->accept(_visitor);
-		listAccept(m_arguments, _visitor);
+		if (m_arguments)
+			listAccept(*m_arguments, _visitor);
 	}
 	_visitor.endVisit(*this);
 }

libsolidity/codegen/ContractCompiler.cpp

@@ -222,7 +222,7 @@ void ContractCompiler::appendBaseConstructor(FunctionDefinition const& _construc
 		if (auto inheritanceSpecifier = dynamic_cast<InheritanceSpecifier const*>(baseArgumentNode))
 			arguments = inheritanceSpecifier->arguments();
 		else if (auto modifierInvocation = dynamic_cast<ModifierInvocation const*>(baseArgumentNode))
-			arguments = &modifierInvocation->arguments();
+			arguments = modifierInvocation->arguments();
 		solAssert(arguments, "");
 		solAssert(arguments->size() == constructorType.parameterTypes().size(), "");
 		for (unsigned i = 0; i < arguments->size(); ++i)
@@ -897,13 +897,16 @@ void ContractCompiler::appendModifierOrFunctionCode()
 			);
 			ModifierDefinition const& modifier = m_context.resolveVirtualFunctionModifier(nonVirtualModifier);
 			CompilerContext::LocationSetter locationSetter(m_context, modifier);
-			solAssert(modifier.parameters().size() == modifierInvocation->arguments().size(), "");
+			std::vector<ASTPointer<Expression>> const& modifierArguments =
+				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(
-					*modifierInvocation->arguments()[i],
+					*modifierArguments[i],
 					modifier.parameters()[i]->annotation().type
 				);
 			}

libsolidity/parsing/Parser.cpp

@@ -701,17 +701,17 @@ ASTPointer<ModifierInvocation> Parser::parseModifierInvocation()
 	RecursionGuard recursionGuard(*this);
 	ASTNodeFactory nodeFactory(*this);
 	ASTPointer<Identifier> name(parseIdentifier());
-	vector<ASTPointer<Expression>> arguments;
+	unique_ptr<vector<ASTPointer<Expression>>> arguments;
 	if (m_scanner->currentToken() == Token::LParen)
 	{
 		m_scanner->next();
-		arguments = parseFunctionCallListArguments();
+		arguments.reset(new vector<ASTPointer<Expression>>(parseFunctionCallListArguments()));
 		nodeFactory.markEndPosition();
 		expectToken(Token::RParen);
 	}
 	else
 		nodeFactory.setEndPositionFromNode(name);
-	return nodeFactory.createNode<ModifierInvocation>(name, arguments);
+	return nodeFactory.createNode<ModifierInvocation>(name, move(arguments));
 }
 
 ASTPointer<Identifier> Parser::parseIdentifier()

test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol

@@ -1,3 +1,2 @@
 contract A { constructor() public { } }
-contract B1 is A { constructor() A() public {  } }
-contract B2 is A { constructor() A public {  } }
+contract B is A { constructor() A() public {  } }

test/libsolidity/syntaxTests/inheritance/disallow_modifier_style_without_parentheses.sol

@@ -0,0 +1,4 @@
+contract A { constructor() public { } }
+contract B is A { constructor() A public {  } }
+// ----
+// Warning: Modifier-style base constructor call without arguments.