From a8ca96cd3ec0b2ed1017f5463676e88e5ae15ad7 Mon Sep 17 00:00:00 2001 From: Mathias Baumann Date: Tue, 17 Dec 2019 14:56:06 +0000 Subject: [PATCH] Refactor PostTypeChecker into multiple classes per usecase --- libsolidity/analysis/PostTypeChecker.cpp | 199 +++++++++++++++-------- libsolidity/analysis/PostTypeChecker.h | 40 ++++- 2 files changed, 159 insertions(+), 80 deletions(-) diff --git a/libsolidity/analysis/PostTypeChecker.cpp b/libsolidity/analysis/PostTypeChecker.cpp index 40c4f22d8..0647a0c64 100644 --- a/libsolidity/analysis/PostTypeChecker.cpp +++ b/libsolidity/analysis/PostTypeChecker.cpp @@ -31,106 +31,163 @@ using namespace dev; using namespace langutil; using namespace dev::solidity; - bool PostTypeChecker::check(ASTNode const& _astRoot) { _astRoot.accept(*this); return Error::containsOnlyWarnings(m_errorReporter.errors()); } -bool PostTypeChecker::visit(ContractDefinition const&) +bool PostTypeChecker::visit(ContractDefinition const& _contractDefinition) { - solAssert(!m_currentConstVariable, ""); - solAssert(m_constVariableDependencies.empty(), ""); - return true; + return callVisit(_contractDefinition); } -void PostTypeChecker::endVisit(ContractDefinition const&) +void PostTypeChecker::endVisit(ContractDefinition const& _contractDefinition) { - solAssert(!m_currentConstVariable, ""); - for (auto declaration: m_constVariables) - if (auto identifier = findCycle(*declaration)) - m_errorReporter.typeError( - declaration->location(), - "The value of the constant " + declaration->name() + - " has a cyclic dependency via " + identifier->name() + "." - ); - - m_constVariables.clear(); - m_constVariableDependencies.clear(); + callEndVisit(_contractDefinition); } void PostTypeChecker::endVisit(OverrideSpecifier const& _overrideSpecifier) { - for (ASTPointer const& override: _overrideSpecifier.overrides()) - { - Declaration const* decl = override->annotation().referencedDeclaration; - solAssert(decl, "Expected declaration to be resolved."); - - if (dynamic_cast(decl)) - continue; - - TypeType const* actualTypeType = dynamic_cast(decl->type()); - - m_errorReporter.typeError( - override->location(), - "Expected contract but got " + - actualTypeType->actualType()->toString(true) + - "." - ); - } + callEndVisit(_overrideSpecifier); } bool PostTypeChecker::visit(VariableDeclaration const& _variable) { - solAssert(!m_currentConstVariable, ""); - if (_variable.isConstant()) - { - m_currentConstVariable = &_variable; - m_constVariables.push_back(&_variable); - } - return true; + return callVisit(_variable); } void PostTypeChecker::endVisit(VariableDeclaration const& _variable) { - if (_variable.isConstant()) - { - solAssert(m_currentConstVariable == &_variable, ""); - m_currentConstVariable = nullptr; - } + callEndVisit(_variable); } bool PostTypeChecker::visit(Identifier const& _identifier) { - if (m_currentConstVariable) - if (auto var = dynamic_cast(_identifier.annotation().referencedDeclaration)) - if (var->isConstant()) - m_constVariableDependencies[m_currentConstVariable].insert(var); - return true; + return callVisit(_identifier); } -VariableDeclaration const* PostTypeChecker::findCycle(VariableDeclaration const& _startingFrom) + +namespace { - auto visitor = [&](VariableDeclaration const& _variable, CycleDetector& _cycleDetector, size_t _depth) - { - if (_depth >= 256) - m_errorReporter.fatalDeclarationError(_variable.location(), "Variable definition exhausting cyclic dependency validator."); +struct ConstStateVarCircularReferenceChecker: public PostTypeChecker::Checker +{ + ConstStateVarCircularReferenceChecker(ErrorReporter& _errorReporter): + Checker(_errorReporter) {} - // Iterating through the dependencies needs to be deterministic and thus cannot - // depend on the memory layout. - // Because of that, we sort by AST node id. - vector dependencies( - m_constVariableDependencies[&_variable].begin(), - m_constVariableDependencies[&_variable].end() - ); - sort(dependencies.begin(), dependencies.end(), [](VariableDeclaration const* _a, VariableDeclaration const* _b) -> bool + bool visit(ContractDefinition const&) override + { + solAssert(!m_currentConstVariable, ""); + solAssert(m_constVariableDependencies.empty(), ""); + return true; + } + + void endVisit(ContractDefinition const&) override + { + solAssert(!m_currentConstVariable, ""); + for (auto declaration: m_constVariables) + if (auto identifier = findCycle(*declaration)) + m_errorReporter.typeError( + declaration->location(), + "The value of the constant " + declaration->name() + + " has a cyclic dependency via " + identifier->name() + "." + ); + + m_constVariables.clear(); + m_constVariableDependencies.clear(); + } + + bool visit(VariableDeclaration const& _variable) override + { + solAssert(!m_currentConstVariable, ""); + if (_variable.isConstant()) { - return _a->id() < _b->id(); - }); - for (auto v: dependencies) - if (_cycleDetector.run(*v)) - return; - }; - return CycleDetector(visitor).run(_startingFrom); + m_currentConstVariable = &_variable; + m_constVariables.push_back(&_variable); + } + return true; + } + + void endVisit(VariableDeclaration const& _variable) override + { + if (_variable.isConstant()) + { + solAssert(m_currentConstVariable == &_variable, ""); + m_currentConstVariable = nullptr; + } + } + + bool visit(Identifier const& _identifier) override + { + if (m_currentConstVariable) + if (auto var = dynamic_cast(_identifier.annotation().referencedDeclaration)) + if (var->isConstant()) + m_constVariableDependencies[m_currentConstVariable].insert(var); + return true; + } + + VariableDeclaration const* findCycle(VariableDeclaration const& _startingFrom) + { + auto visitor = [&](VariableDeclaration const& _variable, CycleDetector& _cycleDetector, size_t _depth) + { + if (_depth >= 256) + m_errorReporter.fatalDeclarationError(_variable.location(), "Variable definition exhausting cyclic dependency validator."); + + // Iterating through the dependencies needs to be deterministic and thus cannot + // depend on the memory layout. + // Because of that, we sort by AST node id. + vector dependencies( + m_constVariableDependencies[&_variable].begin(), + m_constVariableDependencies[&_variable].end() + ); + sort(dependencies.begin(), dependencies.end(), [](VariableDeclaration const* _a, VariableDeclaration const* _b) -> bool + { + return _a->id() < _b->id(); + }); + for (auto v: dependencies) + if (_cycleDetector.run(*v)) + return; + }; + return CycleDetector(visitor).run(_startingFrom); + } + +private: + VariableDeclaration const* m_currentConstVariable = nullptr; + std::map> m_constVariableDependencies; + std::vector m_constVariables; ///< Required for determinism. +}; + +struct OverrideSpecifierChecker: public PostTypeChecker::Checker +{ + OverrideSpecifierChecker(ErrorReporter& _errorReporter): + Checker(_errorReporter) {} + + void endVisit(OverrideSpecifier const& _overrideSpecifier) override + { + for (ASTPointer const& override: _overrideSpecifier.overrides()) + { + Declaration const* decl = override->annotation().referencedDeclaration; + solAssert(decl, "Expected declaration to be resolved."); + + if (dynamic_cast(decl)) + continue; + + TypeType const* actualTypeType = dynamic_cast(decl->type()); + + m_errorReporter.typeError( + override->location(), + "Expected contract but got " + + actualTypeType->actualType()->toString(true) + + "." + ); + } + } +}; +} + + +PostTypeChecker::PostTypeChecker(langutil::ErrorReporter& _errorReporter): m_errorReporter(_errorReporter) +{ + m_checkers.push_back(make_shared(_errorReporter)); + m_checkers.push_back(make_shared(_errorReporter)); } diff --git a/libsolidity/analysis/PostTypeChecker.h b/libsolidity/analysis/PostTypeChecker.h index b4441b395..d89885ade 100644 --- a/libsolidity/analysis/PostTypeChecker.h +++ b/libsolidity/analysis/PostTypeChecker.h @@ -38,20 +38,30 @@ namespace solidity * This module performs analyses on the AST that are done after type checking and assignments of types: * - whether there are circular references in constant state variables * - whether override specifiers are actually contracts - * @TODO factor out each use-case into an individual class (but do the traversal only once) + * + * When adding a new checker, make sure a visitor that forwards calls that your + * checker uses exists in PostTypeChecker. Add missing ones. + * + * The return value for the visit function of a checker is ignored, all nodes + * will always be visited. */ class PostTypeChecker: private ASTConstVisitor { public: + struct Checker: public ASTConstVisitor + { + Checker(langutil::ErrorReporter& _errorReporter): + m_errorReporter(_errorReporter) {} + protected: + langutil::ErrorReporter& m_errorReporter; + }; + /// @param _errorReporter provides the error logging functionality. - PostTypeChecker(langutil::ErrorReporter& _errorReporter): m_errorReporter(_errorReporter) {} + PostTypeChecker(langutil::ErrorReporter& _errorReporter); bool check(ASTNode const& _astRoot); private: - /// Adds a new error to the list of errors. - void typeError(langutil::SourceLocation const& _location, std::string const& _description); - bool visit(ContractDefinition const& _contract) override; void endVisit(ContractDefinition const& _contract) override; void endVisit(OverrideSpecifier const& _overrideSpecifier) override; @@ -61,13 +71,25 @@ private: bool visit(Identifier const& _identifier) override; - VariableDeclaration const* findCycle(VariableDeclaration const& _startingFrom); + template + bool callVisit(T const& _node) + { + for (auto& checker: m_checkers) + checker->visit(_node); + + return true; + } + + template + void callEndVisit(T const& _node) + { + for (auto& checker: m_checkers) + checker->endVisit(_node); + } langutil::ErrorReporter& m_errorReporter; - VariableDeclaration const* m_currentConstVariable = nullptr; - std::vector m_constVariables; ///< Required for determinism. - std::map> m_constVariableDependencies; + std::vector> m_checkers; }; }