Move constructor argument override check to TypeChecker and reuse ann…

…otations in ContractCompiler.
TGBP · Apr 9, 2018 · b918a10 · b918a10
1 parent b8fdb66
commit b918a10
Show file tree

Hide file tree

Showing 15 changed files with 95 additions and 100 deletions.
diff --git 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
@@ -57,7 +57,6 @@ class StaticAnalyzer: private ASTConstVisitor
 
 	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
@@ -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
@@ -73,7 +73,12 @@ class TypeChecker: private ASTConstVisitor
 	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
@@ -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
@@ -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
@@ -135,7 +135,7 @@ class ContractCompiler: private ASTConstVisitor
 	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;
 };
 
 }

diff --git a/test/libsolidity/syntaxTests/inheritance/base_arguments_multiple_inheritance.sol b/test/libsolidity/syntaxTests/inheritance/base_arguments_multiple_inheritance.sol
@@ -6,4 +6,4 @@ contract Derived is Base, Base1 {
     constructor(uint i) Base(i) public {}
 }
 // ----
-// Warning: Duplicated super constructor calls are deprecated.
+// Warning: Base constructor arguments given twice.
diff --git a/.../duplicated_ancestor_constructor_call.sol → .../duplicated_constructor_call/ancestor.sol b/.../duplicated_ancestor_constructor_call.sol → .../duplicated_constructor_call/ancestor.sol
@@ -2,4 +2,4 @@ contract A { constructor(uint) public { } }
 contract B is A(2) { constructor() public {  } }
 contract C is B { constructor() A(3) public {  } }
 // ----
-// Warning: Duplicated super constructor calls are deprecated.
+// Warning: Base constructor arguments given twice.
diff --git a/...icated_ancestor_constructor_call_V050.sol → ...icated_constructor_call/ancestor_V050.sol b/...icated_ancestor_constructor_call_V050.sol → ...icated_constructor_call/ancestor_V050.sol
@@ -4,4 +4,4 @@ contract A { constructor(uint) public { } }
 contract B is A(2) { constructor() public {  } }
 contract C is B { constructor() A(3) public {  } }
 // ----
-// DeclarationError: Duplicated super constructor call.
+// DeclarationError: Base constructor arguments given twice.
diff --git a/...nce/duplicated_super_constructor_call.sol → ...ance/duplicated_constructor_call/base.sol b/...nce/duplicated_super_constructor_call.sol → ...ance/duplicated_constructor_call/base.sol
@@ -1,4 +1,4 @@
 contract A { constructor(uint) public { } }
 contract B is A(2) { constructor() A(3) public {  } }
 // ----
-// Warning: Duplicated super constructor calls are deprecated.
+// Warning: Base constructor arguments given twice.
diff --git a/...uplicated_super_constructor_call_V050.sol → ...duplicated_constructor_call/base_V050.sol b/...uplicated_super_constructor_call_V050.sol → ...duplicated_constructor_call/base_V050.sol
@@ -3,4 +3,4 @@ pragma experimental "v0.5.0";
 contract A { constructor(uint) public { } }
 contract B is A(2) { constructor() A(3) public {  } }
 // ----
-// DeclarationError: Duplicated super constructor call.
+// DeclarationError: Base constructor arguments given twice.
diff --git a/...plicated_super_constructor_call_multi.sol → ...uplicated_constructor_call/base_multi.sol b/...plicated_super_constructor_call_multi.sol → ...uplicated_constructor_call/base_multi.sol
@@ -3,5 +3,5 @@ contract A is C(2) {}
 contract B is C(2) {}
 contract D is A, B { constructor() C(3) public {} }
 // ----
-// Warning: Duplicated super constructor calls are deprecated.
-// Warning: Duplicated super constructor calls are deprecated.
+// Warning: Base constructor arguments given twice.
+// Warning: Base constructor arguments given twice.
diff --git a/...olidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor.sol b/...olidity/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.
diff --git a/...ests/inheritance/duplicated_constructor_call/base_multi_no_constructor_modifier_style.sol b/...ests/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.