Move constructor argument override check to TypeChecker and reuse annotations in ContractCompiler.

7 files changed, 76 insertions, 93 deletions

diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp
index 700c8a71..33b0e296 100644
--- a/libsolidity/analysis/StaticAnalyzer.cpp
+++ b/libsolidity/analysis/StaticAnalyzer.cpp
@@ -90,48 +90,6 @@ void StaticAnalyzer::endVisit(FunctionDefinition const&)
 	m_localVarUseCount.clear();
 }
 
-bool StaticAnalyzer::visit(ModifierInvocation const& _modifier)
-{
-	if (!m_constructor)
-		return true;
-
-	bool const v050 = m_currentContract->sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);
-
-	if (auto contract = dynamic_cast<ContractDefinition const*>(_modifier.name()->annotation().referencedDeclaration))
-		for (auto const& base: m_currentContract->annotation().linearizedBaseContracts)
-			for (auto const& specifier: base->baseContracts())
-			{
-				Declaration const* parent = specifier->name().annotation().referencedDeclaration;
-				if (contract == parent && !specifier->arguments().empty())
-				{
-					if (v050)
-					{
-						SecondarySourceLocation ssl;
-						ssl.append("Second constructor call is here:", specifier->location());
-
-						m_errorReporter.declarationError(
-							_modifier.location(),
-							ssl,
-							"Duplicated super constructor call."
-						);
-					}
-					else
-					{
-						SecondarySourceLocation ssl;
-						ssl.append("Overridden constructor call is here:", specifier->location());
-
-						m_errorReporter.warning(
-							_modifier.location(),
-							"Duplicated super constructor calls are deprecated.",
-							ssl
-						);
-					}
-				}
-			}
-
-	return true;
-}
-
 bool StaticAnalyzer::visit(Identifier const& _identifier)
 {
 	if (m_currentFunction)
diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h
index e68325bc..0a806bbd 100644
--- a/libsolidity/analysis/StaticAnalyzer.h
+++ b/libsolidity/analysis/StaticAnalyzer.h
@@ -57,7 +57,6 @@ private:
 
 	virtual bool visit(FunctionDefinition const& _function) override;
 	virtual void endVisit(FunctionDefinition const& _function) override;
-	virtual bool visit(ModifierInvocation const& _modifier) override;
 
 	virtual bool visit(ExpressionStatement const& _statement) override;
 	virtual bool visit(VariableDeclaration const& _variable) override;
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
index a252742d..c6868a0a 100644
--- a/libsolidity/analysis/TypeChecker.cpp
+++ b/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,75 @@ 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*>(
-					&dereference(*modifier->name())
-				);
-				if (baseContract)
-					argumentsNeeded.erase(baseContract);
+				auto baseContract = dynamic_cast<ContractDefinition const*>(&dereference(*modifier->name()));
+				if (baseContract && baseContract->constructor() && !modifier->arguments().empty())
+					annotateBaseConstructorArguments(_contract, baseContract->constructor(), modifier.get());
 			}
 
-
 		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)
-			_contract.annotation().unimplementedFunctions.push_back(contract->constructor());
+
+	// check that we get arguments for all base constructors that need it.
+	// 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;
+
+		SecondarySourceLocation ssl;
+		ssl.append("Second constructor call is here:", _argumentNode->location());
+
+		if (v050)
+			m_errorReporter.declarationError(
+				previousNode->location(),
+				ssl,
+				"Base constructor arguments given twice."
+			);
+		else
+			m_errorReporter.warning(
+				previousNode->location(),
+				"Base constructor arguments given twice.",
+				ssl
+			);
+	}
+
 }
 
 void TypeChecker::checkContractIllegalOverrides(ContractDefinition const& _contract)
diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h
index 2ba31232..2245abd6 100644
--- a/libsolidity/analysis/TypeChecker.h
+++ b/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);
diff --git a/libsolidity/ast/ASTAnnotations.h b/libsolidity/ast/ASTAnnotations.h
index 3d4236cc..5cbe42bd 100644
--- a/libsolidity/ast/ASTAnnotations.h
+++ b/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
diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp
index d3a7e4ea..3ca0b69d 100644
--- a/libsolidity/codegen/ContractCompiler.cpp
+++ b/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())
-		{
-			ContractDefinition const* baseContract = dynamic_cast<ContractDefinition const*>(
-				base->name().annotation().referencedDeclaration
-			);
-			solAssert(baseContract, "");
+	m_baseArguments = &_contract.annotation().baseConstructorArguments;
 
-			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), "");
-		std::vector<ASTPointer<Expression>> const* arguments = m_baseArguments[&_constructor];
+		solAssert(m_baseArguments, "");
+		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)
diff --git a/libsolidity/codegen/ContractCompiler.h b/libsolidity/codegen/ContractCompiler.h
index e04a56fb..02a3452f 100644
--- a/libsolidity/codegen/ContractCompiler.h
+++ b/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;
 };
 
 }