diff --git a/lib/packwerk/graph.rb b/lib/packwerk/graph.rb index 8008deae8..4659ffd01 100644 --- a/lib/packwerk/graph.rb +++ b/lib/packwerk/graph.rb @@ -1,82 +1,41 @@ # typed: true # frozen_string_literal: true +require "tsort" + module Packwerk # A general implementation of a graph data structure with the ability to check for - and list - cycles. class Graph + include TSort + extend T::Sig sig do params( - # The edges of the graph; An edge being represented as an Array of two nodes. - edges: T::Array[T::Array[T.any(String, Integer, NilClass)]] + # The edges of the graph; represented as an Hash of Arrays. + edges: T::Hash[T.any(String, Integer, NilClass), T::Array[T.any(String, Integer, NilClass)]] ).void end def initialize(edges) - @edges = edges.uniq - @cycles = Set.new - process + @edges = edges end def cycles - @cycles.dup + @cycles ||= strongly_connected_components.reject { _1.size == 1 } end def acyclic? - @cycles.empty? - end - - private - - def nodes - @edges.flatten.uniq - end - - def process - # See https://en.wikipedia.org/wiki/Topological_sorting#Depth-first_search - @processed ||= begin - nodes.each { |node| visit(node) } - true - end - end - - def visit(node, visited_nodes: Set.new, path: []) - # Already visited, short circuit to avoid unnecessary processing - return if visited_nodes.include?(node) - - # We've returned to a node that we've already visited, so we've found a cycle! - if path.include?(node) - # Filter out the part of the path that isn't a cycle. For example, with the following path: - # - # a -> b -> c -> d -> b - # - # "a" isn't part of the cycle. The cycle should only appear once in the path, so we reject - # everything from the beginning to the first instance of the current node. - add_cycle(path.drop_while { |n| n != node }) - return - end - - path << node - neighbours(node).each do |neighbour| - visit(neighbour, visited_nodes: visited_nodes, path: path) - end - path.pop - ensure - visited_nodes << node + cycles.empty? end - def neighbours(node) - @edges - .lazy - .select { |src, _dst| src == node } - .map { |_src, dst| dst } + private def tsort_each_node(&block) + @edges.each_key(&block) end - def add_cycle(cycle) - # Ensure that the lexicographically smallest item is the first one labeled in a cycle - min_node = cycle.min - cycle.rotate! until cycle.first == min_node + EMPTY_ARRAY = [].freeze + private_constant :EMPTY_ARRAY - @cycles << cycle + private def tsort_each_child(node, &block) + (@edges[node] || EMPTY_ARRAY).each(&block) end end diff --git a/lib/packwerk/validators/dependency_validator.rb b/lib/packwerk/validators/dependency_validator.rb index 5430f67bc..41b577616 100644 --- a/lib/packwerk/validators/dependency_validator.rb +++ b/lib/packwerk/validators/dependency_validator.rb @@ -65,10 +65,11 @@ def check_package_manifest_syntax(configuration) sig { params(package_set: PackageSet).returns(Validator::Result) } def check_acyclic_graph(package_set) - edges = package_set.flat_map do |package| - package.dependencies.map do |dependency| - [package.name, package_set.fetch(dependency)&.name] - end + edges = package_set.to_h do |package| + [ + package.name, + package.dependencies.map { |dependency| package_set.fetch(dependency)&.name }, + ] end dependency_graph = Graph.new(edges) diff --git a/test/unit/packwerk/dependency_validator_test.rb b/test/unit/packwerk/dependency_validator_test.rb index 07d000813..72b8ed2c1 100644 --- a/test/unit/packwerk/dependency_validator_test.rb +++ b/test/unit/packwerk/dependency_validator_test.rb @@ -25,7 +25,7 @@ class DependencyValidatorTest < Minitest::Test refute result.ok? assert_match(/Expected the package dependency graph to be acyclic/, result.error_value) - assert_match %r{components/sales → components/timeline → components/sales}, result.error_value + assert_match %r{components/timeline → components/sales → components/timeline}, result.error_value end test "returns error when config contains invalid package dependency" do diff --git a/test/unit/packwerk/graph_test.rb b/test/unit/packwerk/graph_test.rb index 2de0c784d..1df817633 100644 --- a/test/unit/packwerk/graph_test.rb +++ b/test/unit/packwerk/graph_test.rb @@ -6,13 +6,13 @@ module Packwerk class GraphTest < Minitest::Test test "#acyclic? returns true for a directed acyclic graph" do - graph = Graph.new([[1, 2], [1, 3], [2, 4], [3, 4]]) + graph = Graph.new({ 1 => [2, 3], 2 => [4], 3 => [4] }) assert_predicate graph, :acyclic? end test "#acyclic? returns false for a cyclic graph" do - graph = Graph.new([[1, 2], [2, 3], [3, 1]]) + graph = Graph.new({ 1 => [2], 2 => [3], 3 => [1] }) refute_predicate graph, :acyclic? end @@ -27,23 +27,23 @@ class GraphTest < Minitest::Test # | | # +- 6 <- # - graph = Graph.new([[1, 2], [2, 3], [3, 2], [1, 4], [4, 5], [5, 6], [6, 4]]) + graph = Graph.new({ 1 => [2, 4], 2 => [3], 3 => [2], 4 => [5], 5 => [6], 6 => [4] }) assert_equal [[2, 3], [4, 5, 6]], graph.cycles.sort end test "#cycles returns overlapping cycles in a graph" do - graph = Graph.new([[1, 2], [2, 3], [1, 4], [4, 3], [3, 1]]) + graph = Graph.new({ 1 => [2, 4], 2 => [3], 3 => [1], 4 => [3] }) - assert_equal [[1, 2, 3], [1, 4, 3]], graph.cycles.sort + assert_equal [[1, 2, 3, 4]], graph.cycles.sort end test "#cycles returns cycles in a graph with disjoint subgraphs" do - graph = Graph.new([ - [1, 2], [2, 3], [3, 1], - [4, 5], [4, 6], [5, 7], [6, 7], - [8, 9], [9, 8], [8, 10], [10, 11], [8, 11], - ]) + graph = Graph.new({ + 1 => [2], 2 => [3], 3 => [1], + 4 => [5, 6], 5 => [7], 6 => [7], + 8 => [9, 10, 11], 9 => [8], 10 => [11], + }) assert_equal [[1, 2, 3], [8, 9]], graph.cycles.sort end