Improve topologicalSort cycle breaking

pkg/graph_addons/reverse.go

@@ -9,11 +9,7 @@ func ReverseLess[K any](less func(K, K) bool) func(K, K) bool {
 	}
 }
 
-// TopologicalSort provides a stable topological ordering. Note, the following is true:
-//
-//	ReverseTopologicalSort(g, ReverseLess(less)) == reverse(TopologicalSort(g, less))
-//
-// Meaning, the reverse topological sort of a graph under a reversed less is the reverse of the topological sort.
+// TopologicalSort provides a stable topological ordering.
 func ReverseTopologicalSort[K comparable, T any](g graph.Graph[K, T], less func(K, K) bool) ([]K, error) {
 	adjacencyMap, err := g.AdjacencyMap()
 	if err != nil {

pkg/graph_addons/topology.go

@@ -25,19 +25,6 @@ func topologicalSort[K comparable](deps map[K]map[K]graph.Edge[K], less func(K,
 		return nil, nil
 	}
 
-	reverseSort := false
-	// PredecessorMap (for regular topological sort) returns a map source -> targets, so check the first edge to see
-	// if the source matches the top map's key. AdjacencyMap (for reverse topological sort) returns a map
-	// target -> sources, so if the edge's source doesn't match the top map's key, we need to invert the less function.
-	for source, ts := range deps {
-		for _, edge := range ts {
-			if edge.Source != source {
-				reverseSort = true
-			}
-			break
-		}
-	}
-
 	queue := make([]K, 0)
 	queued := make(map[K]struct{})
 	enqueue := func(vs ...K) {
@@ -61,15 +48,12 @@ func topologicalSort[K comparable](deps map[K]map[K]graph.Edge[K], less func(K,
 			remaining = append(remaining, vertex)
 		}
 		sort.Slice(remaining, func(i, j int) bool {
-			// Pick an arbitrary vertex to start the queue based first on the number of remaining deps
-			iPcount := len(deps[remaining[i]])
-			jPcount := len(deps[remaining[j]])
-			if iPcount != jPcount {
-				if reverseSort {
-					return jPcount < iPcount
-				} else {
-					return iPcount < jPcount
-				}
+			// Start based first on the number of remaining deps, prioritizing vertices with fewer deps
+			// to make it most likely to break any cycles, reducing the amount of arbitrary choices.
+			ic := len(deps[remaining[i]])
+			jc := len(deps[remaining[j]])
+			if ic != jc {
+				return ic < jc
 			}
 
 			// Tie-break using the less function on contents themselves

pkg/graph_addons/topology_test.go

@@ -19,6 +19,7 @@ func Test_topologicalSort(t *testing.T) {
 		vertices      []int
 		edges         []Edge
 		expectedOrder []int
+		reverseOrder  []int // defaults to reverse(expectedOrder)
 		shouldFail    bool
 	}{
 		"graph with 5 vertices": {
@@ -47,14 +48,19 @@ func Test_topologicalSort(t *testing.T) {
 			expectedOrder: []int{1, 2, 3, 4, 5, 6, 10, 20, 30, 40, 50, 60},
 		},
 		"graph with cycle": {
-			vertices: []int{1, 2, 3, 4},
-			edges: []Edge{ // 1 -> 3 -> 2 -> 4 ↺
-				{Source: 1, Target: 3},
-				{Source: 3, Target: 2},
-				{Source: 2, Target: 4},
-				{Source: 4, Target: 1},
+			vertices: []int{1, 2, 3, 4, 5},
+			edges: []Edge{
+				{Source: 5, Target: 1},
+
+				// 1 -> 2 -> 3 ↺ 1
+				{Source: 1, Target: 2},
+				{Source: 2, Target: 3},
+				{Source: 3, Target: 1},
+
+				{Source: 3, Target: 4},
 			},
-			expectedOrder: []int{1, 3, 2, 4},
+			expectedOrder: []int{5, 1, 2, 3, 4},
+			reverseOrder:  []int{4, 5, 3, 2, 1},
 		},
 	}
 
@@ -87,8 +93,12 @@ func Test_topologicalSort(t *testing.T) {
 			reverse, err := ReverseTopologicalSort(g, ReverseLess(less))
 			require.NoError(err)
 
-			slices.Reverse(test.expectedOrder)
-			assert.Equal(test.expectedOrder, reverse, "reverse order doesn't match")
+			if test.reverseOrder == nil {
+				test.reverseOrder = make([]int, len(test.expectedOrder))
+				copy(test.reverseOrder, test.expectedOrder)
+				slices.Reverse(test.reverseOrder)
+			}
+			assert.Equal(test.reverseOrder, reverse, "reverse order doesn't match")
 		})
 	}
 }