Detect circular references for library and free functions

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -389,7 +389,6 @@ void NameAndTypeResolver::linearizeBaseContracts(ContractDefinition& _contract)
 	if (result.empty())
 		m_errorReporter.fatalTypeError(5005_error, _contract.location(), "Linearization of inheritance graph impossible");
 	_contract.annotation().linearizedBaseContracts = result;
-	_contract.annotation().contractDependencies.insert(result.begin() + 1, result.end());
 }
 
 template <class T>

libsolidity/analysis/TypeChecker.cpp

@@ -2662,24 +2662,6 @@ void TypeChecker::endVisit(NewExpression const& _newExpression)
 		if (contract->abstract())
 			m_errorReporter.typeError(4614_error, _newExpression.location(), "Cannot instantiate an abstract contract.");
 
-		if (m_currentContract)
-		{
-			// TODO this is not properly detecting creation-cycles if they go through
-			// internal library functions or free functions. It will be caught at
-			// code generation time, but it would of course be better to catch it here.
-			m_currentContract->annotation().contractDependencies.insert(contract);
-			solAssert(
-				!contract->annotation().linearizedBaseContracts.empty(),
-				"Linearized base contracts not yet available."
-			);
-			if (contractDependenciesAreCyclic(*m_currentContract))
-				m_errorReporter.typeError(
-					4579_error,
-					_newExpression.location(),
-					"Circular reference for contract creation (cannot create instance of derived or same contract)."
-				);
-		}
-
 		_newExpression.annotation().type = FunctionType::newExpressionType(*contract);
 		_newExpression.annotation().isPure = false;
 	}
@@ -2953,21 +2935,6 @@ bool TypeChecker::visit(MemberAccess const& _memberAccess)
 					_memberAccess.location(),
 					"\"runtimeCode\" is not available for contracts containing immutable variables."
 				);
-			if (m_currentContract)
-			{
-				// TODO in the same way as with ``new``,
-				// this is not properly detecting creation-cycles if they go through
-				// internal library functions or free functions. It will be caught at
-				// code generation time, but it would of course be better to catch it here.
-
-				m_currentContract->annotation().contractDependencies.insert(&accessedContractType.contractDefinition());
-				if (contractDependenciesAreCyclic(*m_currentContract))
-					m_errorReporter.typeError(
-						4224_error,
-						_memberAccess.location(),
-						"Circular reference for contract code access."
-					);
-			}
 		}
 		else if (magicType->kind() == MagicType::Kind::MetaType && memberName == "name")
 			annotation.isPure = true;
@@ -3455,22 +3422,6 @@ void TypeChecker::checkErrorAndEventParameters(CallableDeclaration const& _calla
 	}
 }
 
-bool TypeChecker::contractDependenciesAreCyclic(
-	ContractDefinition const& _contract,
-	std::set<ContractDefinition const*> const& _seenContracts
-) const
-{
-	// Naive depth-first search that remembers nodes already seen.
-	if (_seenContracts.count(&_contract))
-		return true;
-	set<ContractDefinition const*> seen(_seenContracts);
-	seen.insert(&_contract);
-	for (auto const* c: _contract.annotation().contractDependencies)
-		if (contractDependenciesAreCyclic(*c, seen))
-			return true;
-	return false;
-}
-
 Declaration const& TypeChecker::dereference(Identifier const& _identifier) const
 {
 	solAssert(!!_identifier.annotation().referencedDeclaration, "Declaration not stored.");

libsolidity/analysis/TypeChecker.h

@@ -157,11 +157,6 @@ private:
 
 	void checkErrorAndEventParameters(CallableDeclaration const& _callable);
 
-	bool contractDependenciesAreCyclic(
-		ContractDefinition const& _contract,
-		std::set<ContractDefinition const*> const& _seenContracts = std::set<ContractDefinition const*>()
-	) const;
-
 	/// @returns the referenced declaration and throws on error.
 	Declaration const& dereference(Identifier const& _identifier) const;
 	/// @returns the referenced declaration and throws on error.

libsolidity/ast/AST.h

@@ -65,24 +65,7 @@ class ASTConstVisitor;
 class ASTNode: private boost::noncopyable
 {
 public:
-	struct CompareByID
-	{
-		using is_transparent = void;
-
-		bool operator()(ASTNode const* _lhs, ASTNode const* _rhs) const
-		{
-			return _lhs->id() < _rhs->id();
-		}
-		bool operator()(ASTNode const* _lhs, int64_t _rhs) const
-		{
-			return _lhs->id() < _rhs;
-		}
-		bool operator()(int64_t _lhs, ASTNode const* _rhs) const
-		{
-			return _lhs < _rhs->id();
-		}
-	};
-
+	using CompareByID = frontend::ASTCompareByID<ASTNode>;
 	using SourceLocation = langutil::SourceLocation;
 
 	explicit ASTNode(int64_t _id, SourceLocation _location);

libsolidity/ast/ASTAnnotations.h

@@ -158,9 +158,6 @@ struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, StructurallyDocu
 	/// List of all (direct and indirect) base contracts in order from derived to
 	/// base, including the contract itself.
 	std::vector<ContractDefinition const*> linearizedBaseContracts;
-	/// 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;
@@ -168,6 +165,10 @@ struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, StructurallyDocu
 	SetOnce<std::shared_ptr<CallGraph const>> creationCallGraph;
 	/// A graph with edges representing calls between functions that may happen in a deployed contract.
 	SetOnce<std::shared_ptr<CallGraph const>> deployedCallGraph;
+
+	/// List of contracts whose bytecode is referenced by this contract, e.g. through "new".
+	/// The Value represents the ast node that referenced the contract.
+	std::map<ContractDefinition const*, ASTNode const*, ASTCompareByID<ContractDefinition>> contractDependencies;
 };
 
 struct CallableDeclarationAnnotation: DeclarationAnnotation

libsolidity/ast/ASTForward.h

@@ -100,6 +100,25 @@ class StructuredDocumentation;
 
 class VariableScope;
 
+template <class T>
+struct ASTCompareByID
+{
+	using is_transparent = void;
+
+	bool operator()(T const* _lhs, T const* _rhs) const
+	{
+		return _lhs->id() < _rhs->id();
+	}
+	bool operator()(T const* _lhs, int64_t _rhs) const
+	{
+		return _lhs->id() < _rhs;
+	}
+	bool operator()(int64_t _lhs, T const* _rhs) const
+	{
+		return _lhs < _rhs->id();
+	}
+};
+
 // Used as pointers to AST nodes, to be replaced by more clever pointers, e.g. pointers which do
 // not do reference counting but point to a special memory area that is completely released
 // explicitly.

libsolidity/ast/ASTJsonConverter.cpp

@@ -41,7 +41,9 @@
 #include <algorithm>
 #include <limits>
 #include <type_traits>
+#include <range/v3/view/map.hpp>
 
+using namespace ranges;
 using namespace std;
 using namespace solidity::langutil;
 
@@ -269,7 +271,7 @@ bool ASTJsonConverter::visit(ContractDefinition const& _node)
 		make_pair("contractKind", contractKind(_node.contractKind())),
 		make_pair("abstract", _node.abstract()),
 		make_pair("baseContracts", toJson(_node.baseContracts())),
-		make_pair("contractDependencies", getContainerIds(_node.annotation().contractDependencies, true)),
+		make_pair("contractDependencies", getContainerIds(_node.annotation().contractDependencies | views::keys)),
 		make_pair("nodes", toJson(_node.subNodes())),
 		make_pair("scope", idOrNull(_node.scope()))
 	};

libsolidity/ast/CallGraph.h

@@ -63,7 +63,7 @@ struct CallGraph
 
 	/// Contracts that need to be compiled before this one can be compiled.
 	/// The value is the ast node that created the dependency.
-	std::map<ContractDefinition const*, ASTNode const*, ASTNode::CompareByID> bytecodeDependency;
+	std::map<ContractDefinition const*, ASTNode const*, ASTCompareByID<ContractDefinition>> bytecodeDependency;
 
 	/// Events that may get emitted by functions present in the graph.
 	std::set<EventDefinition const*, ASTNode::CompareByID> emittedEvents;

libsolidity/interface/CompilerStack.cpp

@@ -71,11 +71,15 @@
 #include <libsolutil/SwarmHash.h>
 #include <libsolutil/IpfsHash.h>
 #include <libsolutil/JSON.h>
+#include <libsolutil/Algorithms.h>
 
 #include <json/json.h>
 
-#include <boost/algorithm/string/replace.hpp>
 #include <utility>
+#include <map>
+#include <range/v3/view/concat.hpp>
+
+#include <boost/algorithm/string/replace.hpp>
 
 using namespace std;
 using namespace solidity;
@@ -104,6 +108,94 @@ CompilerStack::~CompilerStack()
 	TypeProvider::reset();
 }
 
+void CompilerStack::createAndAssignCallGraphs()
+{
+	for (Source const* source: m_sourceOrder)
+	{
+		if (!source->ast)
+			continue;
+
+		for (ContractDefinition const* contract: ASTNode::filteredNodes<ContractDefinition>(source->ast->nodes()))
+		{
+			ContractDefinitionAnnotation& annotation =
+				m_contracts.at(contract->fullyQualifiedName()).contract->annotation();
+
+			annotation.creationCallGraph = make_unique<CallGraph>(
+				FunctionCallGraphBuilder::buildCreationGraph(*contract)
+			);
+			annotation.deployedCallGraph = make_unique<CallGraph>(
+				FunctionCallGraphBuilder::buildDeployedGraph(
+					*contract,
+					**annotation.creationCallGraph
+				)
+			);
+
+			solAssert(annotation.contractDependencies.empty(), "contractDependencies expected to be empty?!");
+
+			annotation.contractDependencies = annotation.creationCallGraph->get()->bytecodeDependency;
+
+			for (auto const& [dependencyContract, referencee]: annotation.deployedCallGraph->get()->bytecodeDependency)
+				annotation.contractDependencies.emplace(dependencyContract, referencee);
+		}
+	}
+}
+
+void CompilerStack::findAndReportCyclicContractDependencies()
+{
+	// Cycles we found, used to avoid duplicate reports for the same reference
+	set<ASTNode const*, ASTNode::CompareByID> foundCycles;
+
+	for (Source const* source: m_sourceOrder)
+	{
+		if (!source->ast)
+			continue;
+
+		for (ContractDefinition const* contractDefinition: ASTNode::filteredNodes<ContractDefinition>(source->ast->nodes()))
+		{
+			util::CycleDetector<ContractDefinition> cycleDetector{[&](
+				ContractDefinition const& _contract,
+				util::CycleDetector<ContractDefinition>& _cycleDetector,
+				size_t _depth
+			)
+			{
+				// No specific reason for exactly that number, just a limit we're unlikely to hit.
+				if (_depth >= 256)
+					m_errorReporter.fatalTypeError(
+						7864_error,
+						_contract.location(),
+						"Contract dependencies exhausting cyclic dependency validator"
+					);
+
+				for (auto& [dependencyContract, referencee]: _contract.annotation().contractDependencies)
+					if (_cycleDetector.run(*dependencyContract))
+						return;
+			}};
+
+			ContractDefinition const* cycle = cycleDetector.run(*contractDefinition);
+
+			if (!cycle)
+				continue;
+
+			ASTNode const* referencee = contractDefinition->annotation().contractDependencies.at(cycle);
+
+			if (foundCycles.find(referencee) != foundCycles.end())
+				continue;
+
+			SecondarySourceLocation secondaryLocation{};
+			secondaryLocation.append("Referenced contract is here:"s, cycle->location());
+
+			m_errorReporter.typeError(
+				7813_error,
+				referencee->location(),
+				secondaryLocation,
+				"Circular reference to contract bytecode either via \"new\" or \"type(...).creationCode\" / \"type(...).runtimeCode\"."
+			);
+
+			foundCycles.emplace(referencee);
+		}
+	}
+}
+
 std::optional<CompilerStack::Remapping> CompilerStack::parseRemapping(string const& _remapping)
 {
 	auto eq = find(_remapping.begin(), _remapping.end(), '=');
@@ -400,27 +492,11 @@ bool CompilerStack::analyze()
 			if (source->ast && !typeChecker.checkTypeRequirements(*source->ast))
 				noErrors = false;
 
+		// Create & assign callgraphs and check for contract dependency cycles
 		if (noErrors)
 		{
-			for (Source const* source: m_sourceOrder)
-				if (source->ast)
-					for (ASTPointer<ASTNode> const& node: source->ast->nodes())
-						if (auto const* contractDefinition = dynamic_cast<ContractDefinition*>(node.get()))
-						{
-							Contract& contractState = m_contracts.at(contractDefinition->fullyQualifiedName());
-
-							contractState.contract->annotation().creationCallGraph = make_unique<CallGraph>(
-								FunctionCallGraphBuilder::buildCreationGraph(
-									*contractDefinition
-								)
-							);
-							contractState.contract->annotation().deployedCallGraph = make_unique<CallGraph>(
-								FunctionCallGraphBuilder::buildDeployedGraph(
-									*contractDefinition,
-									**contractState.contract->annotation().creationCallGraph
-								)
-							);
-						}
+			createAndAssignCallGraphs();
+			findAndReportCyclicContractDependencies();
 		}
 
 		if (noErrors)
@@ -1206,7 +1282,7 @@ void CompilerStack::compileContract(
 	if (_otherCompilers.count(&_contract))
 		return;
 
-	for (auto const* dependency: _contract.annotation().contractDependencies)
+	for (auto const& [dependency, referencee]: _contract.annotation().contractDependencies)
 		compileContract(*dependency, _otherCompilers);
 
 	if (!_contract.canBeDeployed())
@@ -1292,7 +1368,7 @@ void CompilerStack::generateIR(ContractDefinition const& _contract)
 		);
 
 	string dependenciesSource;
-	for (auto const* dependency: _contract.annotation().contractDependencies)
+	for (auto const& [dependency, referencee]: _contract.annotation().contractDependencies)
 		generateIR(*dependency);
 
 	if (!_contract.canBeDeployed())

libsolidity/interface/CompilerStack.h

@@ -390,6 +390,9 @@ private:
 		mutable std::optional<std::string const> runtimeSourceMapping;
 	};
 
+	void createAndAssignCallGraphs();
+	void findAndReportCyclicContractDependencies();
+
 	/// Loads the missing sources from @a _ast (named @a _path) using the callback
 	/// @a m_readFile and stores the absolute paths of all imports in the AST annotations.
 	/// @returns the newly loaded sources.
@@ -495,6 +498,7 @@ private:
 	std::shared_ptr<GlobalContext> m_globalContext;
 	std::vector<Source const*> m_sourceOrder;
 	std::map<std::string const, Contract> m_contracts;
+
 	langutil::ErrorList m_errorList;
 	langutil::ErrorReporter m_errorReporter;
 	bool m_metadataLiteralSources = false;