Merge pull request #4215 from ethereum/constructorWorkaround

Disallow legacy constructor

Changelog.md

@@ -24,7 +24,7 @@ Breaking Changes:
  * General: Disallow the ``throw`` statement. This was already the case in the experimental 0.5.0 mode.
  * General: Disallow the ``years`` unit denomination (was already deprecated in 0.4.24)
  * General: Introduce ``emit`` as a keyword instead of parsing it as identifier.
- * General: New keywords: ``calldata``
+ * General: New keywords: ``calldata`` and ``constructor``
  * General: New reserved keywords: ``alias``, ``apply``, ``auto``, ``copyof``, ``define``, ``immutable``,
    ``implements``, ``macro``, ``mutable``, ``override``, ``partial``, ``promise``, ``reference``, ``sealed``,
    ``sizeof``, ``supports``, ``typedef`` and ``unchecked``.

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -710,10 +710,6 @@ void DeclarationRegistrationHelper::registerDeclaration(Declaration& _declaratio
 		dynamic_cast<EventDefinition const*>(m_currentScope)
 	)
 		warnAboutShadowing = false;
-	// Do not warn about the constructor shadowing the contract.
-	if (auto fun = dynamic_cast<FunctionDefinition const*>(&_declaration))
-		if (fun->isConstructor())
-			warnAboutShadowing = false;
 
 	// Register declaration as inactive if we are in block scope.
 	bool inactive =

libsolidity/analysis/SyntaxChecker.cpp

@@ -200,6 +200,14 @@ bool SyntaxChecker::visit(PlaceholderStatement const&)
 bool SyntaxChecker::visit(ContractDefinition const& _contract)
 {
 	m_isInterface = _contract.contractKind() == ContractDefinition::ContractKind::Interface;
+
+	ASTString const& contractName = _contract.name();
+	for (FunctionDefinition const* function: _contract.definedFunctions())
+		if (function->name() == contractName)
+			m_errorReporter.syntaxError(function->location(),
+				"Functions are not allowed to have the same name as the contract. "
+				"If you intend this to be a constructor, use \"constructor(...) { ... }\" to define it."
+			);
 	return true;
 }
 
@@ -216,21 +224,6 @@ bool SyntaxChecker::visit(FunctionDefinition const& _function)
 		);
 	}
 
-	if (_function.isOldStyleConstructor())
-	{
-		if (v050)
-			m_errorReporter.syntaxError(
-				_function.location(),
-				"Functions are not allowed to have the same name as the contract. "
-				"If you intend this to be a constructor, use \"constructor(...) { ... }\" to define it."
-			);
-		else
-			m_errorReporter.warning(
-				_function.location(),
-				"Defining constructors as functions with the same name as the contract is deprecated. "
-				"Use \"constructor(...) { ... }\" instead."
-			);
-	}
 	if (!_function.isImplemented() && !_function.modifiers().empty())
 	{
 		if (v050)
@@ -238,11 +231,6 @@ bool SyntaxChecker::visit(FunctionDefinition const& _function)
 		else
 			m_errorReporter.warning(_function.location(), "Modifiers of functions without implementation are ignored." );
 	}
-	if (_function.name() == "constructor")
-		m_errorReporter.warning(_function.location(),
-			"This function is named \"constructor\" but is not the constructor of the contract. "
-			"If you intend this to be a constructor, use \"constructor(...) { ... }\" without the \"function\" keyword to define it."
-		);
 	return true;
 }
 

libsolidity/ast/AST.h

@@ -614,7 +614,6 @@ public:
 
 	StateMutability stateMutability() const { return m_stateMutability; }
 	bool isConstructor() const { return m_isConstructor; }
-	bool isOldStyleConstructor() const { return m_isConstructor && !name().empty(); }
 	bool isFallback() const { return !m_isConstructor && name().empty(); }
 	bool isPayable() const { return m_stateMutability == StateMutability::Payable; }
 	std::vector<ASTPointer<ModifierInvocation>> const& modifiers() const { return m_functionModifiers; }

libsolidity/parsing/Parser.cpp

@@ -239,13 +239,10 @@ ASTPointer<ContractDefinition> Parser::parseContractDefinition(Token::Value _exp
 		Token::Value currentTokenValue = m_scanner->currentToken();
 		if (currentTokenValue == Token::RBrace)
 			break;
-		else if (
-			currentTokenValue == Token::Function ||
-			(currentTokenValue == Token::Identifier && m_scanner->currentLiteral() == "constructor")
-		)
+		else if (currentTokenValue == Token::Function || currentTokenValue == Token::Constructor)
 			// This can be a function or a state variable of function type (especially
 			// complicated to distinguish fallback function from function type state variable)
-			subNodes.push_back(parseFunctionDefinitionOrFunctionTypeStateVariable(name.get()));
+			subNodes.push_back(parseFunctionDefinitionOrFunctionTypeStateVariable());
 		else if (currentTokenValue == Token::Struct)
 			subNodes.push_back(parseStructDefinition());
 		else if (currentTokenValue == Token::Enum)
@@ -340,30 +337,31 @@ StateMutability Parser::parseStateMutability(Token::Value _token)
 	return stateMutability;
 }
 
-Parser::FunctionHeaderParserResult Parser::parseFunctionHeader(
-	bool _forceEmptyName,
-	bool _allowModifiers,
-	ASTString const* _contractName
-)
+Parser::FunctionHeaderParserResult Parser::parseFunctionHeader(bool _forceEmptyName, bool _allowModifiers)
 {
 	RecursionGuard recursionGuard(*this);
 	FunctionHeaderParserResult result;
 
 	result.isConstructor = false;
 
-	if (m_scanner->currentToken() == Token::Identifier && m_scanner->currentLiteral() == "constructor")
+	if (m_scanner->currentToken() == Token::Constructor)
 		result.isConstructor = true;
 	else if (m_scanner->currentToken() != Token::Function)
 		solAssert(false, "Function or constructor expected.");
 	m_scanner->next();
 
-	if (result.isConstructor || _forceEmptyName || m_scanner->currentToken() == Token::LParen)
+	if (result.isConstructor)
 		result.name = make_shared<ASTString>();
+	else if (_forceEmptyName || m_scanner->currentToken() == Token::LParen)
+		result.name = make_shared<ASTString>();
+	else if (m_scanner->currentToken() == Token::Constructor)
+		fatalParserError(string(
+			"This function is named \"constructor\" but is not the constructor of the contract. "
+			"If you intend this to be a constructor, use \"constructor(...) { ... }\" without the \"function\" keyword to define it."
+		));
 	else
 		result.name = expectIdentifierToken();
 
-	if (!result.name->empty() && _contractName && *result.name == *_contractName)
-		result.isConstructor = true;
 
 	VarDeclParserOptions options;
 	options.allowLocationSpecifier = true;
@@ -435,7 +433,7 @@ Parser::FunctionHeaderParserResult Parser::parseFunctionHeader(
 	return result;
 }
 
-ASTPointer<ASTNode> Parser::parseFunctionDefinitionOrFunctionTypeStateVariable(ASTString const* _contractName)
+ASTPointer<ASTNode> Parser::parseFunctionDefinitionOrFunctionTypeStateVariable()
 {
 	RecursionGuard recursionGuard(*this);
 	ASTNodeFactory nodeFactory(*this);
@@ -443,7 +441,7 @@ ASTPointer<ASTNode> Parser::parseFunctionDefinitionOrFunctionTypeStateVariable(A
 	if (m_scanner->currentCommentLiteral() != "")
 		docstring = make_shared<ASTString>(m_scanner->currentCommentLiteral());
 
-	FunctionHeaderParserResult header = parseFunctionHeader(false, true, _contractName);
+	FunctionHeaderParserResult header = parseFunctionHeader(false, true);
 
 	if (
 		header.isConstructor ||

libsolidity/parsing/Parser.h

@@ -74,12 +74,8 @@ private:
 	ASTPointer<InheritanceSpecifier> parseInheritanceSpecifier();
 	Declaration::Visibility parseVisibilitySpecifier(Token::Value _token);
 	StateMutability parseStateMutability(Token::Value _token);
-	FunctionHeaderParserResult parseFunctionHeader(
-		bool _forceEmptyName,
-		bool _allowModifiers,
-		ASTString const* _contractName = nullptr
-	);
-	ASTPointer<ASTNode> parseFunctionDefinitionOrFunctionTypeStateVariable(ASTString const* _contractName);
+	FunctionHeaderParserResult parseFunctionHeader(bool _forceEmptyName, bool _allowModifiers);
+	ASTPointer<ASTNode> parseFunctionDefinitionOrFunctionTypeStateVariable();
 	ASTPointer<FunctionDefinition> parseFunctionDefinition(ASTString const* _contractName);
 	ASTPointer<StructDefinition> parseStructDefinition();
 	ASTPointer<EnumDefinition> parseEnumDefinition();

libsolidity/parsing/Token.h

@@ -144,6 +144,7 @@ namespace solidity
 	K(Assembly, "assembly", 0)                                         \
 	K(Break, "break", 0)                                               \
 	K(Constant, "constant", 0)                                         \
+	K(Constructor, "constructor", 0)                                   \
 	K(Continue, "continue", 0)                                         \
 	K(Contract, "contract", 0)                                         \
 	K(Do, "do", 0)                                                     \

test/libsolidity/syntaxTests/constructor/constructible_internal_constructor_old.sol

@@ -1,9 +0,0 @@
-contract C {
-	function C() internal {}
-}
-contract D is C {
-	function D() public {}
-}
-// ----
-// Warning: (14-38): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// Warning: (60-82): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.

test/libsolidity/syntaxTests/constructor/constructor_old.sol

@@ -1,3 +1,4 @@
 contract A { function A() public {} }
 // ----
-// Warning: (13-35): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
+// SyntaxError: (13-35): Functions are not allowed to have the same name as the contract. If you intend this to be a constructor, use "constructor(...) { ... }" to define it.
+// Warning: (13-35): This declaration shadows an existing declaration.

test/libsolidity/syntaxTests/constructor/constructor_old_050.sol

@@ -1,4 +0,0 @@
-pragma experimental "v0.5.0";
-contract A { function A() public {} }
-// ----
-// SyntaxError: (43-65): Functions are not allowed to have the same name as the contract. If you intend this to be a constructor, use "constructor(...) { ... }" to define it.

test/libsolidity/syntaxTests/constructor/constructor_state_mutability_old.sol

@@ -1,11 +0,0 @@
-contract test1 {
-    function test1() public view {}
-}
-contract test2 {
-    function test2() public pure {}
-}
-// ----
-// Warning: (21-52): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// Warning: (76-107): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (21-52): Constructor must be payable or non-payable, but is "view".
-// TypeError: (76-107): Constructor must be payable or non-payable, but is "pure".

test/libsolidity/syntaxTests/constructor/constructor_visibility_old.sol

@@ -1,13 +0,0 @@
-// The constructor of a base class should not be visible in the derived class
-contract A { function A(string memory s) public { } }
-contract B is A {
-  function f() pure public {
-    A x = A(0); // convert from address
-    string memory y = "ab";
-    A(y); // call as a function is invalid
-    x;
-  }
-}
-// ----
-// Warning: (91-129): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (251-255): Explicit type conversion not allowed from "string memory" to "contract A".

test/libsolidity/syntaxTests/constructor/constructor_without_implementation_old.sol

@@ -1,6 +0,0 @@
-contract C {
-	function C() public;
-}
-// ----
-// Warning: (14-34): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (14-34): Constructor must be implemented if declared.

test/libsolidity/syntaxTests/constructor/external_constructor_old.sol

@@ -1,6 +0,0 @@
-contract test {
-	function test() external {}
-}
-// ----
-// Warning: (17-44): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (17-44): Constructor must be public or internal.

test/libsolidity/syntaxTests/constructor/function_named_constructor.sol

@@ -2,4 +2,4 @@ contract C {
     function constructor() public;
 }
 // ----
-// Warning: (17-47): This function is named "constructor" but is not the constructor of the contract. If you intend this to be a constructor, use "constructor(...) { ... }" without the "function" keyword to define it.
+// ParserError: (26-37): This function is named "constructor" but is not the constructor of the contract. If you intend this to be a constructor, use "constructor(...) { ... }" without the "function" keyword to define it.

test/libsolidity/syntaxTests/constructor/inconstructible_internal_constructor_inverted_old.sol

@@ -1,15 +0,0 @@
-// Previously, the type information for A was not yet available at the point of
-// "new A".
-contract B {
-	A a;
-	function B() public {
-		a = new A(this);
-	}
-}
-contract A {
-	function A(address a) internal {}
-}
-// ----
-// Warning: (112-155): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// Warning: (172-205): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (140-145): Contract with internal constructor cannot be created directly.

test/libsolidity/syntaxTests/constructor/inconstructible_internal_constructor_old.sol

@@ -1,9 +0,0 @@
-contract C {
-	function C() internal {}
-}
-contract D {
-	function f() public { C x = new C(); x; }
-}
-// ----
-// Warning: (14-38): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (83-88): Contract with internal constructor cannot be created directly.

test/libsolidity/syntaxTests/constructor/interface_constructor_old.sol

@@ -1,8 +0,0 @@
-interface I {
-	function I() public;
-}
-// ----
-// Warning: (15-35): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (15-35): Functions in interfaces must be declared external.
-// TypeError: (15-35): Constructor cannot be defined in interfaces.
-// TypeError: (15-35): Constructor must be implemented if declared.

test/libsolidity/syntaxTests/constructor/library_constructor_old.sol

@@ -1,7 +0,0 @@
-library Lib {
-	function Lib() public;
-}
-// ----
-// Warning: (15-37): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (15-37): Constructor cannot be defined in libraries.
-// TypeError: (15-37): Constructor must be implemented if declared.

test/libsolidity/syntaxTests/constructor/overriding_constructor.sol

@@ -1,10 +1,10 @@
-contract A { constructor() public {} }
-contract B is A { function A() public pure returns (uint8) {} }
-contract C is A { function A() public pure returns (uint8) {} }
-contract D is B { function B() public pure returns (uint8) {} }
-contract E is D { function B() public pure returns (uint8) {} }
+contract A { function f() public {} }
+contract B is A {
+  function A() public pure returns (uint8) {}
+  function g() public {
+    A.f();
+  }
+}
 // ----
-// Warning: (57-100): This declaration shadows an existing declaration.
-// Warning: (121-164): This declaration shadows an existing declaration.
-// Warning: (185-228): This declaration shadows an existing declaration.
-// Warning: (249-292): This declaration shadows an existing declaration.
+// Warning: (58-101): This declaration shadows an existing declaration.
+// TypeError: (130-133): Member "f" not found or not visible after argument-dependent lookup in function () pure returns (uint8).

test/libsolidity/syntaxTests/constructor/returns_in_constructor_old.sol

@@ -1,6 +0,0 @@
-contract test {
-	function test() public returns (uint a) { }
-}
-// ----
-// Warning: (17-60): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// TypeError: (48-56): Non-empty "returns" directive for constructor.

test/libsolidity/syntaxTests/constructor/two_constructors_mixed.sol

@@ -1,7 +0,0 @@
-contract test {
-	function test(uint) public { }
-	constructor() public {}
-}
-// ----
-// Warning: (17-47): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// DeclarationError: (49-72): More than one constructor defined.

test/libsolidity/syntaxTests/constructor/two_constructors_old.sol

@@ -1,8 +0,0 @@
-contract test {
-	function test(uint a) public { }
-	function test() public {}
-}
-// ----
-// Warning: (17-49): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// Warning: (51-76): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.
-// DeclarationError: (51-76): More than one constructor defined.

test/libsolidity/syntaxTests/functionTypes/function_type_constructor_local.sol

@@ -5,4 +5,4 @@ contract C {
     }
 }
 // ----
-// ParserError: (118-119): Expected ';' but got identifier
+// ParserError: (104-115): Expected primary expression.