Merge pull request #417 from corp-gp/improve_dependency_validator

Improve dependency validator's performance

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
 

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)

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

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