Merge pull request #3821 from ethereum/warn-constructor-override

Warn constructor override

Changelog.md

@@ -9,6 +9,7 @@ Features:
  * Optimizer: Remove useless ``SWAP1`` instruction preceding a commutative instruction (such as ``ADD``, ``MUL``, etc).
  * Optimizer: Replace comparison operators (``LT``, ``GT``, etc) with opposites if preceded by ``SWAP1``, e.g. ``SWAP1 LT`` is replaced with ``GT``.
  * Optimizer: Optimize across ``mload`` if ``msize()`` is not used.
+ * 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.

docs/contracts.rst

@@ -1034,9 +1034,12 @@ the base constructors. This can be done in two ways::
         constructor(uint _x) public { x = _x; }
     }
 
-    contract Derived is Base(7) {
+    contract Derived1 is Base(7) {
-        constructor(uint _y) Base(_y * _y) public {
+        constructor(uint _y) public {}
-        }
+    }
+
+    contract Derived2 is Base {
+        constructor(uint _y) Base(_y * _y) public {}
     }
 
 One way is directly in the inheritance list (``is Base(7)``).  The other is in
@@ -1046,8 +1049,9 @@ do it is more convenient if the constructor argument is a
 constant and defines the behaviour of the contract or
 describes it. The second way has to be used if the
 constructor arguments of the base depend on those of the
-derived contract. If, as in this silly example, both places
+derived contract. Arguments have to be given either in the
-are used, the modifier-style argument takes precedence.
+inheritance list or in modifier-style in the derived constuctor.
+Specifying arguments in both places is an error.
 
 .. index:: ! inheritance;multiple, ! linearization, ! C3 linearization
 

libsolidity/analysis/TypeChecker.cpp

@@ -101,7 +101,7 @@ bool TypeChecker::visit(ContractDefinition const& _contract)
 	checkContractDuplicateEvents(_contract);
 	checkContractIllegalOverrides(_contract);
 	checkContractAbstractFunctions(_contract);
-	checkContractAbstractConstructors(_contract);
+	checkContractBaseConstructorArguments(_contract);
 
 	FunctionDefinition const* function = _contract.constructor();
 	if (function)
@@ -291,42 +291,90 @@ void TypeChecker::checkContractAbstractFunctions(ContractDefinition const& _cont
 			}
 }
 
-void TypeChecker::checkContractAbstractConstructors(ContractDefinition const& _contract)
+void TypeChecker::checkContractBaseConstructorArguments(ContractDefinition const& _contract)
 {
-	set<ContractDefinition const*> argumentsNeeded;
-	// check that we get arguments for all base constructors that need it.
-	// If not mark the contract as abstract (not fully implemented)
-
 	vector<ContractDefinition const*> const& bases = _contract.annotation().linearizedBaseContracts;
-	for (ContractDefinition const* contract: bases)
-		if (FunctionDefinition const* constructor = contract->constructor())
-			if (contract != &_contract && !constructor->parameters().empty())
-				argumentsNeeded.insert(contract);
 
+	// Determine the arguments that are used for the base constructors.
 	for (ContractDefinition const* contract: bases)
 	{
 		if (FunctionDefinition const* constructor = contract->constructor())
 			for (auto const& modifier: constructor->modifiers())
 			{
-				auto baseContract = dynamic_cast<ContractDefinition const*>(
+				auto baseContract = dynamic_cast<ContractDefinition const*>(&dereference(*modifier->name()));
-					&dereference(*modifier->name())
+				if (baseContract && baseContract->constructor() && !modifier->arguments().empty())
-				);
+					annotateBaseConstructorArguments(_contract, baseContract->constructor(), modifier.get());
-				if (baseContract)
-					argumentsNeeded.erase(baseContract);
 			}
 
-
 		for (ASTPointer<InheritanceSpecifier> const& base: contract->baseContracts())
 		{
 			auto baseContract = dynamic_cast<ContractDefinition const*>(&dereference(base->name()));
 			solAssert(baseContract, "");
-			if (base->arguments() && !base->arguments()->empty())
+
-				argumentsNeeded.erase(baseContract);
+			if (baseContract->constructor() && base->arguments() && !base->arguments()->empty())
+				annotateBaseConstructorArguments(_contract, baseContract->constructor(), base.get());
 		}
 	}
-	if (!argumentsNeeded.empty())
+
-		for (ContractDefinition const* contract: argumentsNeeded)
+	// check that we get arguments for all base constructors that need it.
-			_contract.annotation().unimplementedFunctions.push_back(contract->constructor());
+	// If not mark the contract as abstract (not fully implemented)
+	for (ContractDefinition const* contract: bases)
+		if (FunctionDefinition const* constructor = contract->constructor())
+			if (contract != &_contract && !constructor->parameters().empty())
+				if (!_contract.annotation().baseConstructorArguments.count(constructor))
+					_contract.annotation().unimplementedFunctions.push_back(constructor);
+}
+
+void TypeChecker::annotateBaseConstructorArguments(
+	ContractDefinition const& _currentContract,
+	FunctionDefinition const* _baseConstructor,
+	ASTNode const* _argumentNode
+)
+{
+	bool const v050 = _currentContract.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);
+
+	solAssert(_baseConstructor, "");
+	solAssert(_argumentNode, "");
+
+	auto insertionResult = _currentContract.annotation().baseConstructorArguments.insert(
+		std::make_pair(_baseConstructor, _argumentNode)
+	);
+	if (!insertionResult.second)
+	{
+		ASTNode const* previousNode = insertionResult.first->second;
+
+		SourceLocation const* mainLocation = nullptr;
+		SecondarySourceLocation ssl;
+	
+		if (
+			_currentContract.location().contains(previousNode->location()) ||
+			_currentContract.location().contains(_argumentNode->location())
+		)
+		{
+			mainLocation = &previousNode->location();
+			ssl.append("Second constructor call is here:", _argumentNode->location());
+		}
+		else
+		{
+			mainLocation = &_currentContract.location();
+			ssl.append("First constructor call is here: ", _argumentNode->location());
+			ssl.append("Second constructor call is here: ", previousNode->location());
+		}
+
+		if (v050)
+			m_errorReporter.declarationError(
+				*mainLocation,
+				ssl,
+				"Base constructor arguments given twice."
+			);
+		else
+			m_errorReporter.warning(
+				*mainLocation,
+				"Base constructor arguments given twice.",
+				ssl
+			);
+	}
+
 }
 
 void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contract)

libsolidity/analysis/TypeChecker.h

@@ -73,7 +73,12 @@ private:
 	void checkFunctionOverride(FunctionDefinition const& function, FunctionDefinition const& super);
 	void overrideError(FunctionDefinition const& function, FunctionDefinition const& super, std::string message);
 	void checkContractAbstractFunctions(ContractDefinition const& _contract);
-	void checkContractAbstractConstructors(ContractDefinition const& _contract);
+	void checkContractBaseConstructorArguments(ContractDefinition const& _contract);
+	void annotateBaseConstructorArguments(
+		ContractDefinition const& _currentContract,
+		FunctionDefinition const* _baseConstructor,
+		ASTNode const* _argumentNode
+	);
 	/// Checks that different functions with external visibility end up having different
 	/// external argument types (i.e. different signature).
 	void checkContractExternalTypeClashes(ContractDefinition const& _contract);

libsolidity/ast/ASTAnnotations.h

@@ -90,6 +90,9 @@ struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, DocumentedAnnota
 	/// List of contracts this contract creates, i.e. which need to be compiled first.
 	/// Also includes all contracts from @a linearizedBaseContracts.
 	std::set<ContractDefinition const*> contractDependencies;
+	/// Mapping containing the nodes that define the arguments for base constructors.
+	/// These can either be inheritance specifiers or modifier invocations.
+	std::map<FunctionDefinition const*, ASTNode const*> baseConstructorArguments;
 };
 
 struct FunctionDefinitionAnnotation: ASTAnnotation, DocumentedAnnotation

libsolidity/codegen/ContractCompiler.cpp

@@ -135,34 +135,13 @@ void ContractCompiler::appendInitAndConstructorCode(ContractDefinition const& _c
 {
 	solAssert(!_contract.isLibrary(), "Tried to initialize library.");
 	CompilerContext::LocationSetter locationSetter(m_context, _contract);
-	// Determine the arguments that are used for the base constructors.
-	std::vector<ContractDefinition const*> const& bases = _contract.annotation().linearizedBaseContracts;
-	for (ContractDefinition const* contract: bases)
-	{
-		if (FunctionDefinition const* constructor = contract->constructor())
-			for (auto const& modifier: constructor->modifiers())
-			{
-				auto baseContract = dynamic_cast<ContractDefinition const*>(
-					modifier->name()->annotation().referencedDeclaration
-				);
-				if (baseContract && !modifier->arguments().empty())
-					if (m_baseArguments.count(baseContract->constructor()) == 0)
-						m_baseArguments[baseContract->constructor()] = &modifier->arguments();
-			}
 
-		for (ASTPointer<InheritanceSpecifier> const& base: contract->baseContracts())
+	m_baseArguments = &_contract.annotation().baseConstructorArguments;
-		{
-			ContractDefinition const* baseContract = dynamic_cast<ContractDefinition const*>(
-				base->name().annotation().referencedDeclaration
-			);
-			solAssert(baseContract, "");
 
-			if (!m_baseArguments.count(baseContract->constructor()) && base->arguments() && !base->arguments()->empty())
-				m_baseArguments[baseContract->constructor()] = base->arguments();
-		}
-	}
 	// Initialization of state variables in base-to-derived order.
-	for (ContractDefinition const* contract: boost::adaptors::reverse(bases))
+	for (ContractDefinition const* contract: boost::adaptors::reverse(
+		_contract.annotation().linearizedBaseContracts
+	))
 		initializeStateVariables(*contract);
 
 	if (FunctionDefinition const* constructor = _contract.constructor())
@@ -236,8 +215,14 @@ void ContractCompiler::appendBaseConstructor(FunctionDefinition const& _construc
 	FunctionType constructorType(_constructor);
 	if (!constructorType.parameterTypes().empty())
 	{
-		solAssert(m_baseArguments.count(&_constructor), "");
+		solAssert(m_baseArguments, "");
-		std::vector<ASTPointer<Expression>> const* arguments = m_baseArguments[&_constructor];
+		solAssert(m_baseArguments->count(&_constructor), "");
+		std::vector<ASTPointer<Expression>> const* arguments = nullptr;
+		ASTNode const* baseArgumentNode = m_baseArguments->at(&_constructor);
+		if (auto inheritanceSpecifier = dynamic_cast<InheritanceSpecifier const*>(baseArgumentNode))
+			arguments = inheritanceSpecifier->arguments();
+		else if (auto modifierInvocation = dynamic_cast<ModifierInvocation const*>(baseArgumentNode))
+			arguments = &modifierInvocation->arguments();
 		solAssert(arguments, "");
 		solAssert(arguments->size() == constructorType.parameterTypes().size(), "");
 		for (unsigned i = 0; i < arguments->size(); ++i)

libsolidity/codegen/ContractCompiler.h

@@ -135,7 +135,7 @@ private:
 	FunctionDefinition const* m_currentFunction = nullptr;
 	unsigned m_stackCleanupForReturn = 0; ///< this number of stack elements need to be removed before jump to m_returnTag
 	// arguments for base constructors, filled in derived-to-base order
-	std::map<FunctionDefinition const*, std::vector<ASTPointer<Expression>> const*> m_baseArguments;
+	std::map<FunctionDefinition const*, ASTNode const*> const* m_baseArguments;
 };
 
 }

test/libsolidity/SolidityEndToEndTest.cpp

@@ -5191,7 +5191,7 @@ BOOST_AUTO_TEST_CASE(pass_dynamic_arguments_to_the_base)
 			}
 			uint public m_i;
 		}
-		contract Derived is Base(2) {
+		contract Derived is Base {
 			function Derived(uint i) Base(i)
 			{}
 		}
@@ -5211,10 +5211,10 @@ BOOST_AUTO_TEST_CASE(pass_dynamic_arguments_to_the_base_base)
 			}
 			uint public m_i;
 		}
-		contract Base1 is Base(3) {
+		contract Base1 is Base {
 			function Base1(uint k) Base(k*k) {}
 		}
-		contract Derived is Base(3), Base1(2) {
+		contract Derived is Base, Base1 {
 			function Derived(uint i) Base(i) Base1(i)
 			{}
 		}
@@ -5235,7 +5235,7 @@ BOOST_AUTO_TEST_CASE(pass_dynamic_arguments_to_the_base_base_with_gap)
 			uint public m_i;
 		}
 		contract Base1 is Base(3) {}
-		contract Derived is Base(2), Base1 {
+		contract Derived is Base, Base1 {
 			function Derived(uint i) Base(i) {}
 		}
 		contract Final is Derived(4) {

test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol

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

test/libsolidity/syntaxTests/inheritance/base_arguments_multiple_inheritance.sol

@@ -0,0 +1,9 @@
+contract Base {
+    constructor(uint) public { }
+}
+contract Base1 is Base(3) {}
+contract Derived is Base, Base1 {
+    constructor(uint i) Base(i) public {}
+}
+// ----
+// Warning: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor.sol

@@ -0,0 +1,5 @@
+contract A { constructor(uint) public { } }
+contract B is A(2) { constructor() public {  } }
+contract C is B { constructor() A(3) public {  } }
+// ----
+// Warning: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor_V050.sol

@@ -0,0 +1,7 @@
+pragma experimental "v0.5.0";
+
+contract A { constructor(uint) public { } }
+contract B is A(2) { constructor() public {  } }
+contract C is B { constructor() A(3) public {  } }
+// ----
+// DeclarationError: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base.sol

@@ -0,0 +1,4 @@
+contract A { constructor(uint) public { } }
+contract B is A(2) { constructor() A(3) public {  } }
+// ----
+// Warning: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_V050.sol

@@ -0,0 +1,6 @@
+pragma experimental "v0.5.0";
+
+contract A { constructor(uint) public { } }
+contract B is A(2) { constructor() A(3) public {  } }
+// ----
+// DeclarationError: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi.sol

@@ -0,0 +1,7 @@
+contract C { constructor(uint) public {} }
+contract A is C(2) {}
+contract B is C(2) {}
+contract D is A, B { constructor() C(3) public {} }
+// ----
+// Warning: Base constructor arguments given twice.
+// Warning: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor.sol

@@ -0,0 +1,6 @@
+contract C { constructor(uint) public {} }
+contract A is C(2) {}
+contract B is C(2) {}
+contract D is A, B {}
+// ----
+// Warning: Base constructor arguments given twice.

test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor_modifier_style.sol

@@ -0,0 +1,6 @@
+contract C { constructor(uint) public {} }
+contract A is C { constructor() C(2) public {} }
+contract B is C { constructor() C(2) public {} }
+contract D is A, B { }
+// ----
+// Warning: Base constructor arguments given twice.