Fix fallback constructor

Changelog.md

@@ -4,7 +4,8 @@ Features:
  * SMTChecker: Integration with CVC4 SMT solver
 
 Bugfixes:
-
+ * Type Checker: Do not complain about new-style constructor and fallback function to have the same name.
+ * Type Checker: Detect multiple constructor declarations in the new syntax and old syntax.
 
 ### 0.4.22 (2018-04-16)
 

libsolidity/analysis/TypeChecker.cpp

@@ -109,39 +109,28 @@ bool TypeChecker::visit(ContractDefinition const& _contract)
 			m_errorReporter.typeError(function->location(), "Constructor must be public or internal.");
 	}
 
-	FunctionDefinition const* fallbackFunction = nullptr;
 	for (FunctionDefinition const* function: _contract.definedFunctions())
-	{
 		if (function->isFallback())
 		{
-			if (fallbackFunction)
-			{
-				m_errorReporter.declarationError(function->location(), "Only one fallback function is allowed.");
-			}
-			else
-			{
-				fallbackFunction = function;
-				if (_contract.isLibrary())
-					m_errorReporter.typeError(fallbackFunction->location(), "Libraries cannot have fallback functions.");
-				if (function->stateMutability() != StateMutability::NonPayable && function->stateMutability() != StateMutability::Payable)
-					m_errorReporter.typeError(
-						function->location(),
-						"Fallback function must be payable or non-payable, but is \"" +
-						stateMutabilityToString(function->stateMutability()) +
-						"\"."
-				);
-				if (!fallbackFunction->parameters().empty())
-					m_errorReporter.typeError(fallbackFunction->parameterList().location(), "Fallback function cannot take parameters.");
-				if (!fallbackFunction->returnParameters().empty())
-					m_errorReporter.typeError(fallbackFunction->returnParameterList()->location(), "Fallback function cannot return values.");
-				if (
-					_contract.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050) &&
-					fallbackFunction->visibility() != FunctionDefinition::Visibility::External
-				)
-					m_errorReporter.typeError(fallbackFunction->location(), "Fallback function must be defined as \"external\".");
-			}
+			if (_contract.isLibrary())
+				m_errorReporter.typeError(function->location(), "Libraries cannot have fallback functions.");
+			if (function->stateMutability() != StateMutability::NonPayable && function->stateMutability() != StateMutability::Payable)
+				m_errorReporter.typeError(
+					function->location(),
+					"Fallback function must be payable or non-payable, but is \"" +
+					stateMutabilityToString(function->stateMutability()) +
+					"\"."
+			);
+			if (!function->parameters().empty())
+				m_errorReporter.typeError(function->parameterList().location(), "Fallback function cannot take parameters.");
+			if (!function->returnParameters().empty())
+				m_errorReporter.typeError(function->returnParameterList()->location(), "Fallback function cannot return values.");
+			if (
+				_contract.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050) &&
+				function->visibility() != FunctionDefinition::Visibility::External
+			)
+				m_errorReporter.typeError(function->location(), "Fallback function must be defined as \"external\".");
 		}
-	}
 
 	for (auto const& n: _contract.subNodes())
 		if (!visited.count(n.get()))
@@ -172,25 +161,34 @@ void TypeChecker::checkContractDuplicateFunctions(ContractDefinition const& _con
 	/// Checks that two functions with the same name defined in this contract have different
 	/// argument types and that there is at most one constructor.
 	map<string, vector<FunctionDefinition const*>> functions;
+	FunctionDefinition const* constructor = nullptr;
+	FunctionDefinition const* fallback = nullptr;
 	for (FunctionDefinition const* function: _contract.definedFunctions())
-		functions[function->name()].push_back(function);
-
-	// Constructor
-	if (functions[_contract.name()].size() > 1)
-	{
-		SecondarySourceLocation ssl;
-		auto it = ++functions[_contract.name()].begin();
-		for (; it != functions[_contract.name()].end(); ++it)
-			ssl.append("Another declaration is here:", (*it)->location());
-
-		string msg = "More than one constructor defined.";
-		ssl.limitSize(msg);
-		m_errorReporter.declarationError(
-			functions[_contract.name()].front()->location(),
-			ssl,
-			msg
-		);
-	}
+		if (function->isConstructor())
+		{
+			if (constructor)
+				m_errorReporter.declarationError(
+					function->location(),
+					SecondarySourceLocation().append("Another declaration is here:", constructor->location()),
+					"More than one constructor defined."
+				);
+			constructor = function;
+		}
+		else if (function->isFallback())
+		{
+			if (fallback)
+				m_errorReporter.declarationError(
+					function->location(),
+					SecondarySourceLocation().append("Another declaration is here:", fallback->location()),
+					"Only one fallback function is allowed."
+				);
+			fallback = function;
+		}
+		else
+		{
+			solAssert(!function->name().empty(), "");
+			functions[function->name()].push_back(function);
+		}
 
 	findDuplicateDefinitions(functions, "Function with same name and arguments defined twice.");
 }

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -243,16 +243,6 @@ BOOST_AUTO_TEST_CASE(assignment_to_struct)
 	CHECK_SUCCESS(text);
 }
 
-BOOST_AUTO_TEST_CASE(returns_in_constructor)
-{
-	char const* text = R"(
-		contract test {
-			function test() public returns (uint a) { }
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Non-empty \"returns\" directive for constructor.");
-}
-
 BOOST_AUTO_TEST_CASE(forward_function_reference)
 {
 	char const* text = R"(
@@ -869,26 +859,6 @@ BOOST_AUTO_TEST_CASE(complex_inheritance)
 	CHECK_SUCCESS(text);
 }
 
-BOOST_AUTO_TEST_CASE(constructor_visibility)
-{
-	// The constructor of a base class should not be visible in the derived class
-	char const* text = R"(
-		contract A { function A() public { } }
-		contract B is A { function f() public { A x = A(0); } }
-	)";
-	CHECK_SUCCESS(text);
-}
-
-BOOST_AUTO_TEST_CASE(overriding_constructor)
-{
-	// It is fine to "override" constructor of a base class since it is invisible
-	char const* text = R"(
-		contract A { function A() public { } }
-		contract B is A { function A() public returns (uint8 r) {} }
-	)";
-	CHECK_SUCCESS(text);
-}
-
 BOOST_AUTO_TEST_CASE(missing_base_constructor_arguments)
 {
 	char const* text = R"(
@@ -907,35 +877,6 @@ BOOST_AUTO_TEST_CASE(base_constructor_arguments_override)
 	CHECK_SUCCESS(text);
 }
 
-BOOST_AUTO_TEST_CASE(new_constructor_syntax)
-{
-	char const* text = R"(
-		contract A { constructor() public {} }
-	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);
-}
-
-BOOST_AUTO_TEST_CASE(old_constructor_syntax)
-{
-	char const* text = R"(
-		contract A { function A() public {} }
-	)";
-	CHECK_WARNING(
-		text,
-		"Defining constructors as functions with the same name as the contract is deprecated."
-	);
-
-	text = R"(
-		pragma experimental "v0.5.0";
-		contract A { function A() public {} }
-	)";
-	CHECK_ERROR(
-		text,
-		SyntaxError,
-		"Functions are not allowed to have the same name as the contract."
-	);
-}
-
 BOOST_AUTO_TEST_CASE(implicit_derived_to_base_conversion)
 {
 	char const* text = R"(
@@ -1199,7 +1140,6 @@ BOOST_AUTO_TEST_CASE(fallback_function_twice)
 		}
 	)";
 	CHECK_ERROR_ALLOW_MULTI(text, DeclarationError, (vector<string>{
-		"Function with same name and arguments defined twice.",
 		"Only one fallback function is"
 	}));
 }
@@ -2588,17 +2528,6 @@ BOOST_AUTO_TEST_CASE(override_changes_return_types)
 	CHECK_ERROR(sourceCode, TypeError, "Overriding function return types differ");
 }
 
-BOOST_AUTO_TEST_CASE(multiple_constructors)
-{
-	char const* sourceCode = R"(
-		contract test {
-			function test(uint a) public { }
-			function test() public {}
-		}
-	)";
-	CHECK_ERROR(sourceCode, DeclarationError, "More than one constructor defined");
-}
-
 BOOST_AUTO_TEST_CASE(equal_overload)
 {
 	char const* sourceCode = R"(
@@ -3122,19 +3051,6 @@ BOOST_AUTO_TEST_CASE(library_having_variables)
 	CHECK_ERROR(text, TypeError, "Library cannot have non-constant state variables");
 }
 
-BOOST_AUTO_TEST_CASE(library_constructor)
-{
-	char const* text = R"(
-		library Lib {
-			function Lib();
-		}
-	)";
-	CHECK_ERROR_ALLOW_MULTI(text, TypeError, (vector<std::string>{
-		"Constructor cannot be defined in libraries.",
-		"Constructor must be implemented if declared."
-	}));
-}
-
 BOOST_AUTO_TEST_CASE(valid_library)
 {
 	char const* text = R"(
@@ -5030,38 +4946,6 @@ BOOST_AUTO_TEST_CASE(unsatisfied_version)
 	BOOST_CHECK(searchErrorMessage(*sourceAndError.second.front(), "Source file requires different compiler version"));
 }
 
-BOOST_AUTO_TEST_CASE(invalid_constructor_statemutability)
-{
-	char const* text = R"(
-		contract test {
-			function test() constant {}
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Constructor must be payable or non-payable");
-	text = R"(
-		contract test {
-			function test() view {}
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Constructor must be payable or non-payable");
-	text = R"(
-		contract test {
-			function test() pure {}
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Constructor must be payable or non-payable");
-}
-
-BOOST_AUTO_TEST_CASE(external_constructor)
-{
-	char const* text = R"(
-		contract test {
-			function test() external {}
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Constructor must be public or internal.");
-}
-
 BOOST_AUTO_TEST_CASE(invalid_array_as_statement)
 {
 	char const* text = R"(
@@ -5608,50 +5492,6 @@ BOOST_AUTO_TEST_CASE(assignment_to_constant)
 	CHECK_ERROR(text, TypeError, "Cannot assign to a constant variable.");
 }
 
-BOOST_AUTO_TEST_CASE(inconstructible_internal_constructor)
-{
-	char const* text = R"(
-		contract C {
-			function C() internal {}
-		}
-		contract D {
-			function f() public { var x = new C(); }
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Contract with internal constructor cannot be created directly.");
-}
-
-BOOST_AUTO_TEST_CASE(inconstructible_internal_constructor_inverted)
-{
-	// Previously, the type information for A was not yet available at the point of
-	// "new A".
-	char const* text = R"(
-		contract B {
-			A a;
-			function B() public {
-				a = new A(this);
-			}
-		}
-		contract A {
-			function A(address a) internal {}
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Contract with internal constructor cannot be created directly.");
-}
-
-BOOST_AUTO_TEST_CASE(constructible_internal_constructor)
-{
-	char const* text = R"(
-		contract C {
-			function C() internal {}
-		}
-		contract D is C {
-			function D() public { }
-		}
-	)";
-	CHECK_SUCCESS(text);
-}
-
 BOOST_AUTO_TEST_CASE(return_structs)
 {
 	char const* text = R"(
@@ -5813,19 +5653,6 @@ BOOST_AUTO_TEST_CASE(interface)
 	CHECK_SUCCESS(text);
 }
 
-BOOST_AUTO_TEST_CASE(interface_constructor)
-{
-	char const* text = R"(
-		interface I {
-			function I();
-		}
-	)";
-	CHECK_ERROR_ALLOW_MULTI(text, TypeError, (std::vector<std::string>{
-		"Constructor cannot be defined in interfaces",
-		"Constructor must be implemented if declared.",
-	}));
-}
-
 BOOST_AUTO_TEST_CASE(interface_functions)
 {
 	char const* text = R"(
@@ -6907,16 +6734,6 @@ BOOST_AUTO_TEST_CASE(builtin_reject_value)
 	CHECK_ERROR(text, TypeError, "Member \"value\" not found or not visible after argument-dependent lookup");
 }
 
-BOOST_AUTO_TEST_CASE(constructor_without_implementation)
-{
-	char const* text = R"(
-		contract C {
-			function C();
-		}
-	)";
-	CHECK_ERROR(text, TypeError, "Constructor must be implemented if declared.");
-}
-
 BOOST_AUTO_TEST_CASE(large_storage_array_fine)
 {
 	char const* text = R"(

test/libsolidity/syntaxTests/constructor/constructible_internal_constructor_new.sol

@@ -0,0 +1,6 @@
+contract C {
+	constructor() internal {}
+}
+contract D is C {
+	constructor() public { }
+}

test/libsolidity/syntaxTests/constructor/constructible_internal_constructor_old.sol

@@ -0,0 +1,9 @@
+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_new.sol

@@ -0,0 +1 @@
+contract A { constructor() public {} }

test/libsolidity/syntaxTests/constructor/constructor_old.sol

@@ -0,0 +1,3 @@
+contract A { function A() public {} }
+// ----
+// Warning: (13-35): Defining constructors as functions with the same name as the contract is deprecated. Use "constructor(...) { ... }" instead.