Refactor PostTypeChecker into multiple classes per usecase

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, "");
+	return callVisit(_contractDefinition);
-	solAssert(m_constVariableDependencies.empty(), "");
-	return true;
 }
 
-void PostTypeChecker::endVisit(ContractDefinition const&)
+void PostTypeChecker::endVisit(ContractDefinition const& _contractDefinition)
 {
-	solAssert(!m_currentConstVariable, "");
+	callEndVisit(_contractDefinition);
-	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();
 }
 
 void PostTypeChecker::endVisit(OverrideSpecifier const& _overrideSpecifier)
 {
-	for (ASTPointer<UserDefinedTypeName> const& override: _overrideSpecifier.overrides())
+	callEndVisit(_overrideSpecifier);
-	{
-		Declaration const* decl  = override->annotation().referencedDeclaration;
-		solAssert(decl, "Expected declaration to be resolved.");
-
-		if (dynamic_cast<ContractDefinition const*>(decl))
-			continue;
-
-		TypeType const* actualTypeType = dynamic_cast<TypeType const*>(decl->type());
-
-		m_errorReporter.typeError(
-			override->location(),
-			"Expected contract but got " +
-			actualTypeType->actualType()->toString(true) +
-			"."
-		);
-	}
 }
 
 bool PostTypeChecker::visit(VariableDeclaration const& _variable)
 {
-	solAssert(!m_currentConstVariable, "");
+	return callVisit(_variable);
-	if (_variable.isConstant())
-	{
-		m_currentConstVariable = &_variable;
-		m_constVariables.push_back(&_variable);
-	}
-	return true;
 }
 
 void PostTypeChecker::endVisit(VariableDeclaration const& _variable)
 {
-	if (_variable.isConstant())
+	callEndVisit(_variable);
-	{
-		solAssert(m_currentConstVariable == &_variable, "");
-		m_currentConstVariable = nullptr;
-	}
 }
 
 bool PostTypeChecker::visit(Identifier const& _identifier)
 {
-	if (m_currentConstVariable)
+	return callVisit(_identifier);
-		if (auto var = dynamic_cast<VariableDeclaration const*>(_identifier.annotation().referencedDeclaration))
-			if (var->isConstant())
-				m_constVariableDependencies[m_currentConstVariable].insert(var);
-	return true;
 }
 
-VariableDeclaration const* PostTypeChecker::findCycle(VariableDeclaration const& _startingFrom)
+
+namespace
 {
-	auto visitor = [&](VariableDeclaration const& _variable, CycleDetector<VariableDeclaration>& _cycleDetector, size_t _depth)
+struct ConstStateVarCircularReferenceChecker: public PostTypeChecker::Checker
-	{
+{
-		if (_depth >= 256)
+	ConstStateVarCircularReferenceChecker(ErrorReporter& _errorReporter):
-			m_errorReporter.fatalDeclarationError(_variable.location(), "Variable definition exhausting cyclic dependency validator.");
+		Checker(_errorReporter) {}
 
-		// Iterating through the dependencies needs to be deterministic and thus cannot
+	bool visit(ContractDefinition const&) override
-		// depend on the memory layout.
+	{
-		// Because of that, we sort by AST node id.
+		solAssert(!m_currentConstVariable, "");
-		vector<VariableDeclaration const*> dependencies(
+		solAssert(m_constVariableDependencies.empty(), "");
-			m_constVariableDependencies[&_variable].begin(),
+		return true;
-			m_constVariableDependencies[&_variable].end()
+	}
-		);
+
-		sort(dependencies.begin(), dependencies.end(), [](VariableDeclaration const* _a, VariableDeclaration const* _b) -> bool
+	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();
+			m_currentConstVariable = &_variable;
-		});
+			m_constVariables.push_back(&_variable);
-		for (auto v: dependencies)
+		}
-			if (_cycleDetector.run(*v))
+		return true;
-				return;
+	}
-	};
+
-	return CycleDetector<VariableDeclaration>(visitor).run(_startingFrom);
+	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<VariableDeclaration const*>(_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<VariableDeclaration>& _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<VariableDeclaration const*> 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<VariableDeclaration>(visitor).run(_startingFrom);
+	}
+
+private:
+	VariableDeclaration const* m_currentConstVariable = nullptr;
+	std::map<VariableDeclaration const*, std::set<VariableDeclaration const*>> m_constVariableDependencies;
+	std::vector<VariableDeclaration const*> m_constVariables; ///< Required for determinism.
+};
+
+struct OverrideSpecifierChecker: public PostTypeChecker::Checker
+{
+	OverrideSpecifierChecker(ErrorReporter& _errorReporter):
+		Checker(_errorReporter) {}
+
+	void endVisit(OverrideSpecifier const& _overrideSpecifier) override
+	{
+		for (ASTPointer<UserDefinedTypeName> const& override: _overrideSpecifier.overrides())
+		{
+			Declaration const* decl  = override->annotation().referencedDeclaration;
+			solAssert(decl, "Expected declaration to be resolved.");
+
+			if (dynamic_cast<ContractDefinition const*>(decl))
+				continue;
+
+			TypeType const* actualTypeType = dynamic_cast<TypeType const*>(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<ConstStateVarCircularReferenceChecker>(_errorReporter));
+	m_checkers.push_back(make_shared<OverrideSpecifierChecker>(_errorReporter));
 }

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 <class T>
+	bool callVisit(T const& _node)
+	{
+		for (auto& checker: m_checkers)
+			checker->visit(_node);
+
+		return true;
+	}
+
+	template <class T>
+	void callEndVisit(T const& _node)
+	{
+		for (auto& checker: m_checkers)
+			checker->endVisit(_node);
+	}
 
 	langutil::ErrorReporter& m_errorReporter;
 
-	VariableDeclaration const* m_currentConstVariable = nullptr;
+	std::vector<std::shared_ptr<Checker>> m_checkers;
-	std::vector<VariableDeclaration const*> m_constVariables; ///< Required for determinism.
-	std::map<VariableDeclaration const*, std::set<VariableDeclaration const*>> m_constVariableDependencies;
 };
 
 }