From e2e32d372ffb378fc2115cbf5b67ec80f77192c5 Mon Sep 17 00:00:00 2001 From: hrkrshnn Date: Wed, 15 Apr 2020 16:12:15 +0530 Subject: [PATCH] virtual modifiers (in Abstract contracts) allow empty bodies --- libsolidity/analysis/ContractLevelChecker.cpp | 65 ++++++++----------- libsolidity/analysis/ContractLevelChecker.h | 3 +- libsolidity/analysis/ImmutableValidator.cpp | 3 +- libsolidity/analysis/OverrideChecker.cpp | 12 +++- libsolidity/analysis/OverrideChecker.h | 2 + libsolidity/analysis/SyntaxChecker.cpp | 2 +- libsolidity/analysis/TypeChecker.cpp | 6 ++ libsolidity/analysis/TypeChecker.h | 1 + libsolidity/ast/AST.h | 9 +-- libsolidity/ast/ASTAnnotations.h | 4 +- libsolidity/ast/ASTJsonConverter.cpp | 4 +- libsolidity/ast/ASTJsonImporter.cpp | 2 +- libsolidity/ast/AST_accept.h | 6 +- libsolidity/ast/Types.h | 1 - libsolidity/formal/SMTEncoder.cpp | 3 +- libsolidity/parsing/Parser.cpp | 17 +++-- .../SolidityNameAndTypeResolution.cpp | 12 ++-- tools/solidityUpgrade/Upgrade060.cpp | 2 +- 18 files changed, 87 insertions(+), 67 deletions(-) diff --git a/libsolidity/analysis/ContractLevelChecker.cpp b/libsolidity/analysis/ContractLevelChecker.cpp index 27323c544..18051f99b 100644 --- a/libsolidity/analysis/ContractLevelChecker.cpp +++ b/libsolidity/analysis/ContractLevelChecker.cpp @@ -55,7 +55,7 @@ bool ContractLevelChecker::check(ContractDefinition const& _contract) checkDuplicateEvents(_contract); m_overrideChecker.check(_contract); checkBaseConstructorArguments(_contract); - checkAbstractFunctions(_contract); + checkAbstractDefinitions(_contract); checkExternalTypeClashes(_contract); checkHashCollisions(_contract); checkLibraryRequirements(_contract); @@ -156,55 +156,43 @@ void ContractLevelChecker::findDuplicateDefinitions(map> const } } -void ContractLevelChecker::checkAbstractFunctions(ContractDefinition const& _contract) +void ContractLevelChecker::checkAbstractDefinitions(ContractDefinition const& _contract) { - // Mapping from name to function definition (exactly one per argument type equality class) and - // flag to indicate whether it is fully implemented. - using FunTypeAndFlag = std::pair; - map> functions; + // Collects functions, static variable getters and modifiers. If they + // override (unimplemented) base class ones, they are replaced. + set proxies; - auto registerFunction = [&](Declaration const& _declaration, FunctionTypePointer const& _type, bool _implemented) + auto registerProxy = [&proxies](OverrideProxy const& _overrideProxy) { - auto& overloads = functions[_declaration.name()]; - auto it = find_if(overloads.begin(), overloads.end(), [&](FunTypeAndFlag const& _funAndFlag) - { - return _type->hasEqualParameterTypes(*_funAndFlag.first); - }); - if (it == overloads.end()) - overloads.emplace_back(_type, _implemented); - else if (_implemented) - it->second = true; + // Overwrite an existing proxy, if it exists. + if (!_overrideProxy.unimplemented()) + proxies.erase(_overrideProxy); + + proxies.insert(_overrideProxy); }; - // Search from base to derived, collect all functions and update - // the 'implemented' flag. + // Search from base to derived, collect all functions and modifiers and + // update proxies. for (ContractDefinition const* contract: boost::adaptors::reverse(_contract.annotation().linearizedBaseContracts)) { for (VariableDeclaration const* v: contract->stateVariables()) if (v->isPartOfExternalInterface()) - registerFunction(*v, TypeProvider::function(*v), true); + registerProxy(OverrideProxy(v)); for (FunctionDefinition const* function: contract->definedFunctions()) if (!function->isConstructor()) - registerFunction( - *function, - TypeProvider::function(*function)->asCallableFunction(false), - function->isImplemented() - ); + registerProxy(OverrideProxy(function)); + + for (ModifierDefinition const* modifier: contract->functionModifiers()) + registerProxy(OverrideProxy(modifier)); } // Set to not fully implemented if at least one flag is false. - // Note that `_contract.annotation().unimplementedFunctions` has already been + // Note that `_contract.annotation().unimplementedDeclarations` has already been // pre-filled by `checkBaseConstructorArguments`. - for (auto const& it: functions) - for (auto const& funAndFlag: it.second) - if (!funAndFlag.second) - { - FunctionDefinition const* function = dynamic_cast(&funAndFlag.first->declaration()); - solAssert(function, ""); - _contract.annotation().unimplementedFunctions.push_back(function); - break; - } + for (auto const& proxy: proxies) + if (proxy.unimplemented()) + _contract.annotation().unimplementedDeclarations.push_back(proxy.declaration()); if (_contract.abstract()) { @@ -221,15 +209,16 @@ void ContractLevelChecker::checkAbstractFunctions(ContractDefinition const& _con if ( _contract.contractKind() == ContractKind::Contract && !_contract.abstract() && - !_contract.annotation().unimplementedFunctions.empty() + !_contract.annotation().unimplementedDeclarations.empty() ) { SecondarySourceLocation ssl; - for (auto function: _contract.annotation().unimplementedFunctions) - ssl.append("Missing implementation:", function->location()); + for (auto declaration: _contract.annotation().unimplementedDeclarations) + ssl.append("Missing implementation: ", declaration->location()); m_errorReporter.typeError(_contract.location(), ssl, "Contract \"" + _contract.annotation().canonicalName + "\" should be marked as abstract."); + } } @@ -277,7 +266,7 @@ void ContractLevelChecker::checkBaseConstructorArguments(ContractDefinition cons if (FunctionDefinition const* constructor = contract->constructor()) if (contract != &_contract && !constructor->parameters().empty()) if (!_contract.annotation().baseConstructorArguments.count(constructor)) - _contract.annotation().unimplementedFunctions.push_back(constructor); + _contract.annotation().unimplementedDeclarations.push_back(constructor); } void ContractLevelChecker::annotateBaseConstructorArguments( diff --git a/libsolidity/analysis/ContractLevelChecker.h b/libsolidity/analysis/ContractLevelChecker.h index 919c35c07..736b3d3d7 100644 --- a/libsolidity/analysis/ContractLevelChecker.h +++ b/libsolidity/analysis/ContractLevelChecker.h @@ -61,7 +61,8 @@ private: void checkDuplicateEvents(ContractDefinition const& _contract); template void findDuplicateDefinitions(std::map> const& _definitions, std::string _message); - void checkAbstractFunctions(ContractDefinition const& _contract); + /// Checks for unimplemented functions and modifiers. + void checkAbstractDefinitions(ContractDefinition const& _contract); /// Checks that the base constructor arguments are properly provided. /// Fills the list of unimplemented functions in _contract's annotations. void checkBaseConstructorArguments(ContractDefinition const& _contract); diff --git a/libsolidity/analysis/ImmutableValidator.cpp b/libsolidity/analysis/ImmutableValidator.cpp index b03d314eb..598f43e50 100644 --- a/libsolidity/analysis/ImmutableValidator.cpp +++ b/libsolidity/analysis/ImmutableValidator.cpp @@ -148,7 +148,8 @@ bool ImmutableValidator::analyseCallable(CallableDeclaration const& _callableDec funcDef->body().accept(*this); } else if (ModifierDefinition const* modDef = dynamic_cast(&_callableDeclaration)) - modDef->body().accept(*this); + if (modDef->isImplemented()) + modDef->body().accept(*this); m_currentConstructor = prevConstructor; diff --git a/libsolidity/analysis/OverrideChecker.cpp b/libsolidity/analysis/OverrideChecker.cpp index 8367f9f58..91b45d33c 100644 --- a/libsolidity/analysis/OverrideChecker.cpp +++ b/libsolidity/analysis/OverrideChecker.cpp @@ -327,6 +327,16 @@ ModifierType const* OverrideProxy::modifierType() const }, m_item); } + +Declaration const* OverrideProxy::declaration() const +{ + return std::visit(GenericVisitor{ + [&](FunctionDefinition const* _function) -> Declaration const* { return _function; }, + [&](VariableDeclaration const* _variable) -> Declaration const* { return _variable; }, + [&](ModifierDefinition const* _modifier) -> Declaration const* { return _modifier; } + }, m_item); +} + SourceLocation const& OverrideProxy::location() const { return std::visit(GenericVisitor{ @@ -365,7 +375,7 @@ bool OverrideProxy::unimplemented() const { return std::visit(GenericVisitor{ [&](FunctionDefinition const* _item) { return !_item->isImplemented(); }, - [&](ModifierDefinition const*) { return false; }, + [&](ModifierDefinition const* _item) { return !_item->isImplemented(); }, [&](VariableDeclaration const*) { return false; } }, m_item); } diff --git a/libsolidity/analysis/OverrideChecker.h b/libsolidity/analysis/OverrideChecker.h index d4b96663c..9d7776028 100644 --- a/libsolidity/analysis/OverrideChecker.h +++ b/libsolidity/analysis/OverrideChecker.h @@ -85,6 +85,8 @@ public: FunctionType const* functionType() const; ModifierType const* modifierType() const; + Declaration const* declaration() const; + langutil::SourceLocation const& location() const; std::string astNodeName() const; diff --git a/libsolidity/analysis/SyntaxChecker.cpp b/libsolidity/analysis/SyntaxChecker.cpp index ba973bffa..c9ad29d33 100644 --- a/libsolidity/analysis/SyntaxChecker.cpp +++ b/libsolidity/analysis/SyntaxChecker.cpp @@ -141,7 +141,7 @@ bool SyntaxChecker::visit(ModifierDefinition const&) void SyntaxChecker::endVisit(ModifierDefinition const& _modifier) { - if (!m_placeholderFound) + if (_modifier.isImplemented() && !m_placeholderFound) m_errorReporter.syntaxError(_modifier.body().location(), "Modifier body does not contain '_'."); m_placeholderFound = false; } diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 250607fe0..a0155c9ce 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -289,6 +289,12 @@ void TypeChecker::endVisit(UsingForDirective const& _usingFor) m_errorReporter.fatalTypeError(_usingFor.libraryName().location(), "Library name expected."); } +void TypeChecker::endVisit(ModifierDefinition const& _modifier) +{ + if (!_modifier.isImplemented() && !_modifier.virtualSemantics()) + m_errorReporter.typeError(_modifier.location(), "Modifiers without implementation must be marked virtual."); +} + bool TypeChecker::visit(FunctionDefinition const& _function) { bool isLibraryFunction = _function.inContractKind() == ContractKind::Library; diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index 585f7d95f..5c327bf2e 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -112,6 +112,7 @@ private: void endVisit(InheritanceSpecifier const& _inheritance) override; void endVisit(UsingForDirective const& _usingFor) override; + void endVisit(ModifierDefinition const& _modifier) override; bool visit(FunctionDefinition const& _function) override; bool visit(VariableDeclaration const& _variable) override; /// We need to do this manually because we want to pass the bases of the current contract in diff --git a/libsolidity/ast/AST.h b/libsolidity/ast/AST.h index 926d4e74a..3c79912c2 100644 --- a/libsolidity/ast/AST.h +++ b/libsolidity/ast/AST.h @@ -969,7 +969,7 @@ private: /** * Definition of a function modifier. */ -class ModifierDefinition: public CallableDeclaration, public StructurallyDocumented +class ModifierDefinition: public CallableDeclaration, public StructurallyDocumented, public ImplementationOptional { public: ModifierDefinition( @@ -980,18 +980,19 @@ public: ASTPointer const& _parameters, bool _isVirtual, ASTPointer const& _overrides, - ASTPointer _body + ASTPointer const& _body ): CallableDeclaration(_id, _location, _name, Visibility::Internal, _parameters, _isVirtual, _overrides), StructurallyDocumented(_documentation), - m_body(std::move(_body)) + ImplementationOptional(_body != nullptr), + m_body(_body) { } void accept(ASTVisitor& _visitor) override; void accept(ASTConstVisitor& _visitor) const override; - Block const& body() const { return *m_body; } + Block const& body() const { solAssert(m_body, ""); return *m_body; } TypePointer type() const override; diff --git a/libsolidity/ast/ASTAnnotations.h b/libsolidity/ast/ASTAnnotations.h index 2589103cb..0ed54897a 100644 --- a/libsolidity/ast/ASTAnnotations.h +++ b/libsolidity/ast/ASTAnnotations.h @@ -140,8 +140,8 @@ struct StructDeclarationAnnotation: TypeDeclarationAnnotation struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, StructurallyDocumentedAnnotation { - /// List of functions without a body. Can also contain functions from base classes. - std::vector unimplementedFunctions; + /// List of functions and modifiers without a body. Can also contain functions from base classes. + std::vector unimplementedDeclarations; /// List of all (direct and indirect) base contracts in order from derived to /// base, including the contract itself. std::vector linearizedBaseContracts; diff --git a/libsolidity/ast/ASTJsonConverter.cpp b/libsolidity/ast/ASTJsonConverter.cpp index c9d3e1389..9f61184be 100644 --- a/libsolidity/ast/ASTJsonConverter.cpp +++ b/libsolidity/ast/ASTJsonConverter.cpp @@ -272,7 +272,7 @@ bool ASTJsonConverter::visit(ContractDefinition const& _node) make_pair("documentation", _node.documentation() ? toJson(*_node.documentation()) : Json::nullValue), make_pair("contractKind", contractKind(_node.contractKind())), make_pair("abstract", _node.abstract()), - make_pair("fullyImplemented", _node.annotation().unimplementedFunctions.empty()), + make_pair("fullyImplemented", _node.annotation().unimplementedDeclarations.empty()), make_pair("linearizedBaseContracts", getContainerIds(_node.annotation().linearizedBaseContracts)), make_pair("baseContracts", toJson(_node.baseContracts())), make_pair("contractDependencies", getContainerIds(_node.annotation().contractDependencies, true)), @@ -407,7 +407,7 @@ bool ASTJsonConverter::visit(ModifierDefinition const& _node) make_pair("parameters", toJson(_node.parameterList())), make_pair("virtual", _node.markedVirtual()), make_pair("overrides", _node.overrides() ? toJson(*_node.overrides()) : Json::nullValue), - make_pair("body", toJson(_node.body())) + make_pair("body", _node.isImplemented() ? toJson(_node.body()) : Json::nullValue) }; if (!_node.annotation().baseFunctions.empty()) attributes.emplace_back(make_pair("baseModifiers", getContainerIds(_node.annotation().baseFunctions, true))); diff --git a/libsolidity/ast/ASTJsonImporter.cpp b/libsolidity/ast/ASTJsonImporter.cpp index 7d388877b..e4538059d 100644 --- a/libsolidity/ast/ASTJsonImporter.cpp +++ b/libsolidity/ast/ASTJsonImporter.cpp @@ -453,7 +453,7 @@ ASTPointer ASTJsonImporter::createModifierDefinition(Json::V createParameterList(member(_node, "parameters")), memberAsBool(_node, "virtual"), _node["overrides"].isNull() ? nullptr : createOverrideSpecifier(member(_node, "overrides")), - createBlock(member(_node, "body")) + _node["body"].isNull() ? nullptr: createBlock(member(_node, "body")) ); } diff --git a/libsolidity/ast/AST_accept.h b/libsolidity/ast/AST_accept.h index bc0123d51..e597ce46b 100644 --- a/libsolidity/ast/AST_accept.h +++ b/libsolidity/ast/AST_accept.h @@ -288,7 +288,8 @@ void ModifierDefinition::accept(ASTVisitor& _visitor) m_parameters->accept(_visitor); if (m_overrides) m_overrides->accept(_visitor); - m_body->accept(_visitor); + if (m_body) + m_body->accept(_visitor); } _visitor.endVisit(*this); } @@ -302,7 +303,8 @@ void ModifierDefinition::accept(ASTConstVisitor& _visitor) const m_parameters->accept(_visitor); if (m_overrides) m_overrides->accept(_visitor); - m_body->accept(_visitor); + if (m_body) + m_body->accept(_visitor); } _visitor.endVisit(*this); } diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h index c1f66df4d..974bae4a3 100644 --- a/libsolidity/ast/Types.h +++ b/libsolidity/ast/Types.h @@ -1390,7 +1390,6 @@ public: std::string richIdentifier() const override; bool operator==(Type const& _other) const override; std::string toString(bool _short) const override; - protected: std::vector> makeStackItems() const override { return {}; } private: diff --git a/libsolidity/formal/SMTEncoder.cpp b/libsolidity/formal/SMTEncoder.cpp index 12c9971dc..9003d26ce 100644 --- a/libsolidity/formal/SMTEncoder.cpp +++ b/libsolidity/formal/SMTEncoder.cpp @@ -194,7 +194,8 @@ void SMTEncoder::inlineModifierInvocation(ModifierInvocation const* _invocation, pushCallStack({_definition, _invocation}); if (auto modifier = dynamic_cast(_definition)) { - modifier->body().accept(*this); + if (modifier->isImplemented()) + modifier->body().accept(*this); popCallStack(); } else if (auto function = dynamic_cast(_definition)) diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp index befd3c696..c6da9a55b 100644 --- a/libsolidity/parsing/Parser.cpp +++ b/libsolidity/parsing/Parser.cpp @@ -595,13 +595,13 @@ ASTPointer Parser::parseFunctionDefinition() ASTPointer block; nodeFactory.markEndPosition(); - if (m_scanner->currentToken() != Token::Semicolon) + if (m_scanner->currentToken() == Token::Semicolon) + m_scanner->next(); + else { block = parseBlock(); nodeFactory.setEndPositionFromNode(block); } - else - m_scanner->next(); // just consume the ';' return nodeFactory.createNode( name, header.visibility, @@ -851,9 +851,16 @@ ASTPointer Parser::parseModifierDefinition() break; } + ASTPointer block; + nodeFactory.markEndPosition(); + if (m_scanner->currentToken() != Token::Semicolon) + { + block = parseBlock(); + nodeFactory.setEndPositionFromNode(block); + } + else + m_scanner->next(); // just consume the ';' - ASTPointer block = parseBlock(); - nodeFactory.setEndPositionFromNode(block); return nodeFactory.createNode(name, documentation, parameters, isVirtual, overrides, block); } diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index b66b5aad6..26d9ca989 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(function_no_implementation) std::vector> nodes = sourceUnit->nodes(); ContractDefinition* contract = dynamic_cast(nodes[1].get()); BOOST_REQUIRE(contract); - BOOST_CHECK(!contract->annotation().unimplementedFunctions.empty()); + BOOST_CHECK(!contract->annotation().unimplementedDeclarations.empty()); BOOST_CHECK(!contract->definedFunctions()[0]->isImplemented()); } @@ -68,10 +68,10 @@ BOOST_AUTO_TEST_CASE(abstract_contract) ContractDefinition* base = dynamic_cast(nodes[1].get()); ContractDefinition* derived = dynamic_cast(nodes[2].get()); BOOST_REQUIRE(base); - BOOST_CHECK(!base->annotation().unimplementedFunctions.empty()); + BOOST_CHECK(!base->annotation().unimplementedDeclarations.empty()); BOOST_CHECK(!base->definedFunctions()[0]->isImplemented()); BOOST_REQUIRE(derived); - BOOST_CHECK(derived->annotation().unimplementedFunctions.empty()); + BOOST_CHECK(derived->annotation().unimplementedDeclarations.empty()); BOOST_CHECK(derived->definedFunctions()[0]->isImplemented()); } @@ -87,9 +87,9 @@ BOOST_AUTO_TEST_CASE(abstract_contract_with_overload) ContractDefinition* base = dynamic_cast(nodes[1].get()); ContractDefinition* derived = dynamic_cast(nodes[2].get()); BOOST_REQUIRE(base); - BOOST_CHECK(!base->annotation().unimplementedFunctions.empty()); + BOOST_CHECK(!base->annotation().unimplementedDeclarations.empty()); BOOST_REQUIRE(derived); - BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty()); + BOOST_CHECK(!derived->annotation().unimplementedDeclarations.empty()); } BOOST_AUTO_TEST_CASE(implement_abstract_via_constructor) @@ -104,7 +104,7 @@ BOOST_AUTO_TEST_CASE(implement_abstract_via_constructor) BOOST_CHECK_EQUAL(nodes.size(), 3); ContractDefinition* derived = dynamic_cast(nodes[2].get()); BOOST_REQUIRE(derived); - BOOST_CHECK(!derived->annotation().unimplementedFunctions.empty()); + BOOST_CHECK(!derived->annotation().unimplementedDeclarations.empty()); } BOOST_AUTO_TEST_CASE(function_canonical_signature) diff --git a/tools/solidityUpgrade/Upgrade060.cpp b/tools/solidityUpgrade/Upgrade060.cpp index 9ed7cbc74..f5fc4bdf5 100644 --- a/tools/solidityUpgrade/Upgrade060.cpp +++ b/tools/solidityUpgrade/Upgrade060.cpp @@ -99,7 +99,7 @@ inline string appendVirtual(FunctionDefinition const& _function) void AbstractContract::endVisit(ContractDefinition const& _contract) { - bool isFullyImplemented = _contract.annotation().unimplementedFunctions.empty(); + bool isFullyImplemented = _contract.annotation().unimplementedDeclarations.empty(); if ( !isFullyImplemented &&