Merge pull request #30286 from hashicorp/jbardin/dag

dag: minor cleanup

internal/dag/dag.go

@@ -2,7 +2,6 @@ package dag
 
 import (
 	"fmt"
-	"sort"
 	"strings"
 
 	"github.com/hashicorp/terraform/internal/tfdiags"
@@ -89,9 +88,7 @@ func (g *AcyclicGraph) Root() (Vertex, error) {
 // same graph with only a single edge between A and B, and a single edge
 // between B and C.
 //
-// The graph must be valid for this operation to behave properly. If
+// The graph must be free of cycles for this operation to behave properly.
-// Validate() returns an error, the behavior is undefined and the results
-// will likely be unexpected.
 //
 // Complexity: O(V(V+E)), or asymptotically O(VE)
 func (g *AcyclicGraph) TransitiveReduction() {
@@ -146,6 +143,8 @@ func (g *AcyclicGraph) Validate() error {
 	return err
 }
 
+// Cycles reports any cycles between graph nodes.
+// Self-referencing nodes are not reported, and must be detected separately.
 func (g *AcyclicGraph) Cycles() [][]Vertex {
 	var cycles [][]Vertex
 	for _, cycle := range StronglyConnected(&g.Graph) {
@@ -181,6 +180,8 @@ type vertexAtDepth struct {
 
 // DepthFirstWalk does a depth-first walk of the graph starting from
 // the vertices in start.
+// The algorithm used here does not do a complete topological sort. To ensure
+// correct overall ordering run TransitiveReduction first.
 func (g *AcyclicGraph) DepthFirstWalk(start Set, f DepthWalkFunc) error {
 	seen := make(map[Vertex]struct{})
 	frontier := make([]*vertexAtDepth, 0, len(start))
@@ -218,51 +219,10 @@ func (g *AcyclicGraph) DepthFirstWalk(start Set, f DepthWalkFunc) error {
 	return nil
 }
 
-// SortedDepthFirstWalk does a depth-first walk of the graph starting from
-// the vertices in start, always iterating the nodes in a consistent order.
-func (g *AcyclicGraph) SortedDepthFirstWalk(start []Vertex, f DepthWalkFunc) error {
-	seen := make(map[Vertex]struct{})
-	frontier := make([]*vertexAtDepth, len(start))
-	for i, v := range start {
-		frontier[i] = &vertexAtDepth{
-			Vertex: v,
-			Depth:  0,
-		}
-	}
-	for len(frontier) > 0 {
-		// Pop the current vertex
-		n := len(frontier)
-		current := frontier[n-1]
-		frontier = frontier[:n-1]
-
-		// Check if we've seen this already and return...
-		if _, ok := seen[current.Vertex]; ok {
-			continue
-		}
-		seen[current.Vertex] = struct{}{}
-
-		// Visit the current node
-		if err := f(current.Vertex, current.Depth); err != nil {
-			return err
-		}
-
-		// Visit targets of this in a consistent order.
-		targets := AsVertexList(g.downEdgesNoCopy(current.Vertex))
-		sort.Sort(byVertexName(targets))
-
-		for _, t := range targets {
-			frontier = append(frontier, &vertexAtDepth{
-				Vertex: t,
-				Depth:  current.Depth + 1,
-			})
-		}
-	}
-
-	return nil
-}
-
 // ReverseDepthFirstWalk does a depth-first walk _up_ the graph starting from
 // the vertices in start.
+// The algorithm used here does not do a complete topological sort. To ensure
+// correct overall ordering run TransitiveReduction first.
 func (g *AcyclicGraph) ReverseDepthFirstWalk(start Set, f DepthWalkFunc) error {
 	seen := make(map[Vertex]struct{})
 	frontier := make([]*vertexAtDepth, 0, len(start))
@@ -299,55 +259,3 @@ func (g *AcyclicGraph) ReverseDepthFirstWalk(start Set, f DepthWalkFunc) error {
 
 	return nil
 }
-
-// SortedReverseDepthFirstWalk does a depth-first walk _up_ the graph starting from
-// the vertices in start, always iterating the nodes in a consistent order.
-func (g *AcyclicGraph) SortedReverseDepthFirstWalk(start []Vertex, f DepthWalkFunc) error {
-	seen := make(map[Vertex]struct{})
-	frontier := make([]*vertexAtDepth, len(start))
-	for i, v := range start {
-		frontier[i] = &vertexAtDepth{
-			Vertex: v,
-			Depth:  0,
-		}
-	}
-	for len(frontier) > 0 {
-		// Pop the current vertex
-		n := len(frontier)
-		current := frontier[n-1]
-		frontier = frontier[:n-1]
-
-		// Check if we've seen this already and return...
-		if _, ok := seen[current.Vertex]; ok {
-			continue
-		}
-		seen[current.Vertex] = struct{}{}
-
-		// Add next set of targets in a consistent order.
-		targets := AsVertexList(g.upEdgesNoCopy(current.Vertex))
-		sort.Sort(byVertexName(targets))
-		for _, t := range targets {
-			frontier = append(frontier, &vertexAtDepth{
-				Vertex: t,
-				Depth:  current.Depth + 1,
-			})
-		}
-
-		// Visit the current node
-		if err := f(current.Vertex, current.Depth); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-// byVertexName implements sort.Interface so a list of Vertices can be sorted
-// consistently by their VertexName
-type byVertexName []Vertex
-
-func (b byVertexName) Len() int      { return len(b) }
-func (b byVertexName) Swap(i, j int) { b[i], b[j] = b[j], b[i] }
-func (b byVertexName) Less(i, j int) bool {
-	return VertexName(b[i]) < VertexName(b[j])
-}

internal/dag/dag_test.go

@@ -99,6 +99,38 @@ func TestAyclicGraphTransReduction_more(t *testing.T) {
 	}
 }
 
+func TestAyclicGraphTransReduction_multipleRoots(t *testing.T) {
+	var g AcyclicGraph
+	g.Add(1)
+	g.Add(2)
+	g.Add(3)
+	g.Add(4)
+	g.Connect(BasicEdge(1, 2))
+	g.Connect(BasicEdge(1, 3))
+	g.Connect(BasicEdge(1, 4))
+	g.Connect(BasicEdge(2, 3))
+	g.Connect(BasicEdge(2, 4))
+	g.Connect(BasicEdge(3, 4))
+
+	g.Add(5)
+	g.Add(6)
+	g.Add(7)
+	g.Add(8)
+	g.Connect(BasicEdge(5, 6))
+	g.Connect(BasicEdge(5, 7))
+	g.Connect(BasicEdge(5, 8))
+	g.Connect(BasicEdge(6, 7))
+	g.Connect(BasicEdge(6, 8))
+	g.Connect(BasicEdge(7, 8))
+	g.TransitiveReduction()
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testGraphTransReductionMultipleRootsStr)
+	if actual != expected {
+		t.Fatalf("bad: %s", actual)
+	}
+}
+
 // use this to simulate slow sort operations
 type counter struct {
 	Name  string
@@ -392,7 +424,10 @@ func TestAcyclicGraph_ReverseDepthFirstWalk_WithRemoval(t *testing.T) {
 
 	var visits []Vertex
 	var lock sync.Mutex
-	err := g.SortedReverseDepthFirstWalk([]Vertex{1}, func(v Vertex, d int) error {
+	root := make(Set)
+	root.Add(1)
+
+	err := g.ReverseDepthFirstWalk(root, func(v Vertex, d int) error {
 		lock.Lock()
 		defer lock.Unlock()
 		visits = append(visits, v)
@@ -426,3 +461,20 @@ const testGraphTransReductionMoreStr = `
   4
 4
 `
+
+const testGraphTransReductionMultipleRootsStr = `
+1
+  2
+2
+  3
+3
+  4
+4
+5
+  6
+6
+  7
+7
+  8
+8
+`