diff options
17 files changed, 158 insertions, 59 deletions
diff --git a/Changelog.md b/Changelog.md index d1e199b7..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. 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/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index a252742d..1d8fd82d 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,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*>( - &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; + + 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) 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; }; } 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/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..5483d5d7 --- /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: Base constructor arguments given twice. 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. |