From 4e037281acaf74c907f68e6227d6aa1b8847c78d Mon Sep 17 00:00:00 2001 From: Federico Bond Date: Mon, 11 Dec 2017 18:00:15 -0300 Subject: Error on duplicated super constructor calls --- Changelog.md | 1 + docs/contracts.rst | 14 ++++++---- libsolidity/analysis/StaticAnalyzer.cpp | 32 ++++++++++++++++++++++ libsolidity/analysis/StaticAnalyzer.h | 1 + test/libsolidity/SolidityEndToEndTest.cpp | 8 +++--- .../duplicated_super_constructor_call.sol | 4 +++ .../duplicated_super_constructor_call_empty.sol | 4 +++ 7 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol diff --git a/Changelog.md b/Changelog.md index d1e199b7..94703f7d 100644 --- a/Changelog.md +++ b/Changelog.md @@ -28,6 +28,7 @@ Bugfixes: * Type System: Make external library functions accessible. * Type System: Prevent encoding of weird types. * Static Analyzer: Fix non-deterministic order of unused variable warnings. + * Static Analyzer: Error on duplicated super constructor calls. ### 0.4.21 (2018-03-07) diff --git a/docs/contracts.rst b/docs/contracts.rst index f8a44fb3..0dd9845c 100644 --- a/docs/contracts.rst +++ b/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) { - constructor(uint _y) Base(_y * _y) public { - } + contract Derived1 is Base(7) { + 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 -are used, the modifier-style argument takes precedence. +derived contract. Arguments have to be given either in the +inheritance list or in modifier-style in the derived constuctor. +Specifying arguments in both places is an error. .. index:: ! inheritance;multiple, ! linearization, ! C3 linearization diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp index 33b0e296..6c70ba6b 100644 --- a/libsolidity/analysis/StaticAnalyzer.cpp +++ b/libsolidity/analysis/StaticAnalyzer.cpp @@ -90,6 +90,38 @@ void StaticAnalyzer::endVisit(FunctionDefinition const&) m_localVarUseCount.clear(); } +bool modifierOverridesInheritanceSpecifier( + ContractDefinition const* _contract, + ModifierInvocation const& _modifier, + InheritanceSpecifier const& _specifier +) +{ + auto parent = _specifier.name().annotation().referencedDeclaration; + return _contract == parent && (!_specifier.arguments().empty() || _modifier.arguments().empty()); +} + +bool StaticAnalyzer::visit(ModifierInvocation const& _modifier) +{ + if (!m_constructor) + return true; + + if (auto contract = dynamic_cast(_modifier.name()->annotation().referencedDeclaration)) + for (auto const& specifier: m_currentContract->baseContracts()) + if (modifierOverridesInheritanceSpecifier(contract, _modifier, *specifier)) + { + SecondarySourceLocation ssl; + ssl.append("Overriden constructor call is here:", specifier->location()); + + m_errorReporter.declarationError( + _modifier.location(), + ssl, + "Duplicated super constructor call." + ); + } + + return true; +} + bool StaticAnalyzer::visit(Identifier const& _identifier) { if (m_currentFunction) diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h index 0a806bbd..e68325bc 100644 --- a/libsolidity/analysis/StaticAnalyzer.h +++ b/libsolidity/analysis/StaticAnalyzer.h @@ -57,6 +57,7 @@ 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/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index beeae786..5ed53a2f 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/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) { diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol new file mode 100644 index 00000000..95df1040 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol @@ -0,0 +1,4 @@ +contract A { constructor(uint) public { } } +contract B is A(2) { constructor() A(3) public { } } +// ---- +// DeclarationError: Duplicated super constructor call. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol new file mode 100644 index 00000000..f8024ad6 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol @@ -0,0 +1,4 @@ +contract A { constructor() public { } } +contract B is A { constructor() A() public { } } +// ---- +// DeclarationError: Duplicated super constructor call. -- cgit v1.2.3 From b8fdb666e235bb6b19f11dba7740227026111598 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Wed, 4 Apr 2018 12:28:22 +0200 Subject: Allow duplicated constructor calls, if no arguments; support for multiple inheritance; backwards compatibility. # tmp --- Changelog.md | 2 +- libsolidity/analysis/StaticAnalyzer.cpp | 50 +++++++++++++--------- ...low_empty_duplicated_super_constructor_call.sol | 3 ++ .../base_arguments_multiple_inheritance.sol | 9 ++++ .../duplicated_ancestor_constructor_call.sol | 5 +++ .../duplicated_ancestor_constructor_call_V050.sol | 7 +++ .../duplicated_super_constructor_call.sol | 2 +- .../duplicated_super_constructor_call_V050.sol | 6 +++ .../duplicated_super_constructor_call_empty.sol | 4 -- .../duplicated_super_constructor_call_multi.sol | 7 +++ 10 files changed, 69 insertions(+), 26 deletions(-) create mode 100644 test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/base_arguments_multiple_inheritance.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol delete mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol diff --git a/Changelog.md b/Changelog.md index 94703f7d..3b8cba1d 100644 --- a/Changelog.md +++ b/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. @@ -28,7 +29,6 @@ Bugfixes: * Type System: Make external library functions accessible. * Type System: Prevent encoding of weird types. * Static Analyzer: Fix non-deterministic order of unused variable warnings. - * Static Analyzer: Error on duplicated super constructor calls. ### 0.4.21 (2018-03-07) diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp index 6c70ba6b..700c8a71 100644 --- a/libsolidity/analysis/StaticAnalyzer.cpp +++ b/libsolidity/analysis/StaticAnalyzer.cpp @@ -90,33 +90,43 @@ void StaticAnalyzer::endVisit(FunctionDefinition const&) m_localVarUseCount.clear(); } -bool modifierOverridesInheritanceSpecifier( - ContractDefinition const* _contract, - ModifierInvocation const& _modifier, - InheritanceSpecifier const& _specifier -) -{ - auto parent = _specifier.name().annotation().referencedDeclaration; - return _contract == parent && (!_specifier.arguments().empty() || _modifier.arguments().empty()); -} - 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(_modifier.name()->annotation().referencedDeclaration)) - for (auto const& specifier: m_currentContract->baseContracts()) - if (modifierOverridesInheritanceSpecifier(contract, _modifier, *specifier)) + for (auto const& base: m_currentContract->annotation().linearizedBaseContracts) + for (auto const& specifier: base->baseContracts()) { - SecondarySourceLocation ssl; - ssl.append("Overriden constructor call is here:", specifier->location()); - - m_errorReporter.declarationError( - _modifier.location(), - ssl, - "Duplicated super constructor call." - ); + 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; diff --git a/test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/allow_empty_duplicated_super_constructor_call.sol new file mode 100644 index 00000000..1f580b1d --- /dev/null +++ b/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 { } } diff --git a/test/libsolidity/syntaxTests/inheritance/base_arguments_multiple_inheritance.sol b/test/libsolidity/syntaxTests/inheritance/base_arguments_multiple_inheritance.sol new file mode 100644 index 00000000..f63d0f02 --- /dev/null +++ b/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: Duplicated super constructor calls are deprecated. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol new file mode 100644 index 00000000..97f3f8ff --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.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: Duplicated super constructor calls are deprecated. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol new file mode 100644 index 00000000..933c9087 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_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: Duplicated super constructor call. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol index 95df1040..876b07ea 100644 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol @@ -1,4 +1,4 @@ contract A { constructor(uint) public { } } contract B is A(2) { constructor() A(3) public { } } // ---- -// DeclarationError: Duplicated super constructor call. +// Warning: Duplicated super constructor calls are deprecated. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol new file mode 100644 index 00000000..31a363fd --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_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: Duplicated super constructor call. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol deleted file mode 100644 index f8024ad6..00000000 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_empty.sol +++ /dev/null @@ -1,4 +0,0 @@ -contract A { constructor() public { } } -contract B is A { constructor() A() public { } } -// ---- -// DeclarationError: Duplicated super constructor call. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol new file mode 100644 index 00000000..caed18eb --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_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: Duplicated super constructor calls are deprecated. +// Warning: Duplicated super constructor calls are deprecated. -- cgit v1.2.3 From b918a105a40aa90fe9b89eecbcdfc7ac2937c141 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Thu, 5 Apr 2018 16:25:20 +0200 Subject: Move constructor argument override check to TypeChecker and reuse annotations in ContractCompiler. --- libsolidity/analysis/StaticAnalyzer.cpp | 42 ------------ libsolidity/analysis/StaticAnalyzer.h | 1 - libsolidity/analysis/TypeChecker.cpp | 75 ++++++++++++++++------ libsolidity/analysis/TypeChecker.h | 7 +- libsolidity/ast/ASTAnnotations.h | 3 + libsolidity/codegen/ContractCompiler.cpp | 39 ++++------- libsolidity/codegen/ContractCompiler.h | 2 +- .../base_arguments_multiple_inheritance.sol | 2 +- .../duplicated_ancestor_constructor_call.sol | 5 -- .../duplicated_ancestor_constructor_call_V050.sol | 7 -- .../duplicated_constructor_call/ancestor.sol | 5 ++ .../duplicated_constructor_call/ancestor_V050.sol | 7 ++ .../duplicated_constructor_call/base.sol | 4 ++ .../duplicated_constructor_call/base_V050.sol | 6 ++ .../duplicated_constructor_call/base_multi.sol | 7 ++ .../base_multi_no_constructor.sol | 6 ++ .../base_multi_no_constructor_modifier_style.sol | 6 ++ .../duplicated_super_constructor_call.sol | 4 -- .../duplicated_super_constructor_call_V050.sol | 6 -- .../duplicated_super_constructor_call_multi.sol | 7 -- 20 files changed, 118 insertions(+), 123 deletions(-) delete mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol delete mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor_V050.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_V050.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor_modifier_style.sol delete mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol delete mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol delete mode 100644 test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol 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(_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 argumentsNeeded; - // check that we get arguments for all base constructors that need it. - // If not mark the contract as abstract (not fully implemented) - vector 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( - &dereference(*modifier->name()) - ); - if (baseContract) - argumentsNeeded.erase(baseContract); + auto baseContract = dynamic_cast(&dereference(*modifier->name())); + if (baseContract && baseContract->constructor() && !modifier->arguments().empty()) + annotateBaseConstructorArguments(_contract, baseContract->constructor(), modifier.get()); } - for (ASTPointer const& base: contract->baseContracts()) { auto baseContract = dynamic_cast(&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 contractDependencies; + /// Mapping containing the nodes that define the arguments for base constructors. + /// These can either be inheritance specifiers or modifier invocations. + std::map 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 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( - modifier->name()->annotation().referencedDeclaration - ); - if (baseContract && !modifier->arguments().empty()) - if (m_baseArguments.count(baseContract->constructor()) == 0) - m_baseArguments[baseContract->constructor()] = &modifier->arguments(); - } - for (ASTPointer const& base: contract->baseContracts()) - { - ContractDefinition const* baseContract = dynamic_cast( - 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> const* arguments = m_baseArguments[&_constructor]; + solAssert(m_baseArguments, ""); + solAssert(m_baseArguments->count(&_constructor), ""); + std::vector> const* arguments = nullptr; + ASTNode const* baseArgumentNode = m_baseArguments->at(&_constructor); + if (auto inheritanceSpecifier = dynamic_cast(baseArgumentNode)) + arguments = inheritanceSpecifier->arguments(); + else if (auto modifierInvocation = dynamic_cast(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> const*> m_baseArguments; + std::map 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 index f63d0f02..5483d5d7 100644 --- 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/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol deleted file mode 100644 index 97f3f8ff..00000000 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call.sol +++ /dev/null @@ -1,5 +0,0 @@ -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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol deleted file mode 100644 index 933c9087..00000000 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_ancestor_constructor_call_V050.sol +++ /dev/null @@ -1,7 +0,0 @@ -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: Duplicated super constructor call. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor.sol new file mode 100644 index 00000000..8b1af245 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor_V050.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/ancestor_V050.sol new file mode 100644 index 00000000..6616c9a9 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base.sol new file mode 100644 index 00000000..1fb504fe --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_V050.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_V050.sol new file mode 100644 index 00000000..96eb1bb1 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi.sol new file mode 100644 index 00000000..db9ffc85 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor.sol new file mode 100644 index 00000000..fe280ad5 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor_modifier_style.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_constructor_call/base_multi_no_constructor_modifier_style.sol new file mode 100644 index 00000000..ea85aae7 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol deleted file mode 100644 index 876b07ea..00000000 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call.sol +++ /dev/null @@ -1,4 +0,0 @@ -contract A { constructor(uint) public { } } -contract B is A(2) { constructor() A(3) public { } } -// ---- -// Warning: Duplicated super constructor calls are deprecated. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol deleted file mode 100644 index 31a363fd..00000000 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_V050.sol +++ /dev/null @@ -1,6 +0,0 @@ -pragma experimental "v0.5.0"; - -contract A { constructor(uint) public { } } -contract B is A(2) { constructor() A(3) public { } } -// ---- -// DeclarationError: Duplicated super constructor call. diff --git a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol b/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol deleted file mode 100644 index caed18eb..00000000 --- a/test/libsolidity/syntaxTests/inheritance/duplicated_super_constructor_call_multi.sol +++ /dev/null @@ -1,7 +0,0 @@ -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: Duplicated super constructor calls are deprecated. -// Warning: Duplicated super constructor calls are deprecated. -- cgit v1.2.3 From 549ba801fb2a175cfb24e3c83b63030f47984e95 Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Mon, 9 Apr 2018 16:01:29 +0200 Subject: Use the most derived contract as main location in case of diamond inheritance. --- libsolidity/analysis/TypeChecker.cpp | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index c6868a0a..1d8fd82d 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -343,18 +343,33 @@ void TypeChecker::annotateBaseConstructorArguments( { ASTNode const* previousNode = insertionResult.first->second; + SourceLocation const* mainLocation = nullptr; SecondarySourceLocation ssl; - ssl.append("Second constructor call is here:", _argumentNode->location()); + + 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( - previousNode->location(), + *mainLocation, ssl, "Base constructor arguments given twice." ); else m_errorReporter.warning( - previousNode->location(), + *mainLocation, "Base constructor arguments given twice.", ssl ); -- cgit v1.2.3