Merge pull request #3853 from ethereum/modifierStyleWithoutParentheses

Modifier style constructor calls without parentheses are an error.

9 files changed, 52 insertions, 21 deletions

diff --git a/Changelog.md b/Changelog.md
index 3b8cba1d..eb35652f 100644
--- a/Changelog.md
+++ b/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.
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index 1d8fd82d..abe57778 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/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,8 +304,24 @@ 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())
-					annotateBaseConstructorArguments(_contract, baseContract->constructor(), modifier.get());
+				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(),
diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h
index bc85349b..ae253f0c 100644
--- a/libsolidity/ast/AST.h
+++ b/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;
 };
 
 /**
diff --git a/libsolidity/ast/ASTJsonConverter.cpp b/libsolidity/ast/ASTJsonConverter.cpp
index 94932eca..95ba3089 100644
--- a/libsolidity/ast/ASTJsonConverter.cpp
+++ b/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;
 }
diff --git a/libsolidity/ast/AST_accept.h b/libsolidity/ast/AST_accept.h
index dac414fc..aeff6e4a 100644
--- a/libsolidity/ast/AST_accept.h
+++ b/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);
 }
diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp
index 3ca0b69d..5cb37103 100644
--- a/libsolidity/codegen/ContractCompiler.cpp
+++ b/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
 				);
 			}
diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp
index 9a7731d8..18ef740a 100644
--- a/libsolidity/parsing/Parser.cpp
+++ b/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()
diff --git a/test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol
index 1f580b1d..ce9d5f5f 100644
--- a/test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol
+++ b/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 {  } }
diff --git a/test/libsolidity/syntaxTests/inheritance/disallow_modifier_style_without_parentheses.sol b/test/libsolidity/syntaxTests/inheritance/disallow_modifier_style_without_parentheses.sol
new file mode 100644
index 00000000..0c159a22
--- /dev/null
+++ b/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.