core: Simplify our idea of "root node" and require it for DynamicExpand

The graph walking mechanism is specified as requiring a graph with a single
root, which in practice means there's exactly one node in the graph
which doesn't have any dependencies.

However, we previously weren't verifying that invariant is true for
subgraphs returned from DynamicExpand. It was working anyway, but it's not
ideal to be relying on a behavior that isn't guaranteed by our underlying
infrastructure.

We also previously had the RootTransformer being a bit clever and trying
to avoid adding a new node if there is already only a single graph with
no dependencies. That special case isn't particularly valuable since
there's no harm in turning a one-node graph into a two-node graph with
an explicit separate root node, and doing that allows us to assume that
the root node is always present and is always exactly terraform.rootNode.

Many existing DynamicExpand implementations were not producing valid
graphs and were previously getting away with it. All of them now produce
properly-rooted graphs that should pass validation, and we will guarantee
that with an explicit check of the DynamicExpand return value before we
try to walk that subgraph. For good measure we also verify that the root
node is exactly terraform.rootNode, even though that isn't strictly
required by our graph walker, just to help us catch potential future bugs
where a DynamicExpand implementation neglects to add our singleton root
node.

internal/terraform/graph.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"fmt"
 	"log"
 	"strings"
 
@@ -88,6 +89,28 @@ func (g *Graph) walk(walker GraphWalker) tfdiags.Diagnostics {
 				return
 			}
 			if g != nil {
+				// The subgraph should always be valid, per our normal acyclic
+				// graph validation rules.
+				if err := g.Validate(); err != nil {
+					diags = diags.Append(tfdiags.Sourceless(
+						tfdiags.Error,
+						"Graph node has invalid dynamic subgraph",
+						fmt.Sprintf("The internal logic for %q generated an invalid dynamic subgraph: %s.\n\nThis is a bug in Terraform. Please report it!", dag.VertexName(v), err),
+					))
+					return
+				}
+				// If we passed validation then there is exactly one root node.
+				// That root node should always be "rootNode", the singleton
+				// root node value.
+				if n, err := g.Root(); err != nil || n != dag.Vertex(rootNode) {
+					diags = diags.Append(tfdiags.Sourceless(
+						tfdiags.Error,
+						"Graph node has invalid dynamic subgraph",
+						fmt.Sprintf("The internal logic for %q generated an invalid dynamic subgraph: the root node is %T, which is not a suitable root node type.\n\nThis is a bug in Terraform. Please report it!", dag.VertexName(v), v),
+					))
+					return
+				}
+
 				// Walk the subgraph
 				log.Printf("[TRACE] vertex %q: entering dynamic subgraph", dag.VertexName(v))
 				subDiags := g.walk(walker)

internal/terraform/node_local.go

@@ -73,6 +73,7 @@ func (n *nodeExpandLocal) DynamicExpand(ctx EvalContext) (*Graph, error) {
 		log.Printf("[TRACE] Expanding local: adding %s as %T", o.Addr.String(), o)
 		g.Add(o)
 	}
+	addRootNodeToGraph(&g)
 	return &g, nil
 }
 

internal/terraform/node_module_variable.go

@@ -50,6 +50,7 @@ func (n *nodeExpandModuleVariable) DynamicExpand(ctx EvalContext) (*Graph, error
 		}
 		g.Add(o)
 	}
+	addRootNodeToGraph(&g)
 	return &g, nil
 }
 

internal/terraform/node_output.go

@@ -113,6 +113,7 @@ func (n *nodeExpandOutput) DynamicExpand(ctx EvalContext) (*Graph, error) {
 		log.Printf("[TRACE] Expanding output: adding %s as %T", o.Addr.String(), o)
 		g.Add(o)
 	}
+	addRootNodeToGraph(&g)
 
 	if checkableAddrs != nil {
 		checkState := ctx.Checks()

internal/terraform/node_resource_apply.go

@@ -49,6 +49,7 @@ func (n *nodeExpandApplyableResource) DynamicExpand(ctx EvalContext) (*Graph, er
 			Addr:                 n.Addr.Resource.Absolute(module),
 		})
 	}
+	addRootNodeToGraph(&g)
 
 	return &g, nil
 }

internal/terraform/node_resource_import.go

@@ -176,11 +176,7 @@ func (n *graphNodeImportState) DynamicExpand(ctx EvalContext) (*Graph, error) {
 		})
 	}
 
-	// Root transform for a single root
-	t := &RootTransformer{}
-	if err := t.Transform(g); err != nil {
-		return nil, err
-	}
+	addRootNodeToGraph(g)
 
 	// Done!
 	return g, diags.Err()

internal/terraform/node_resource_plan.go

@@ -171,6 +171,8 @@ func (n *nodeExpandPlannableResource) DynamicExpand(ctx EvalContext) (*Graph, er
 		checkState.ReportCheckableObjects(n.NodeAbstractResource.Addr, instAddrs)
 	}
 
+	addRootNodeToGraph(&g)
+
 	return &g, diags.ErrWithWarnings()
 }
 

internal/terraform/transform_root.go

@@ -10,41 +10,48 @@ const rootNodeName = "root"
 type RootTransformer struct{}
 
 func (t *RootTransformer) Transform(g *Graph) error {
-	// If we already have a good root, we're done
-	if _, err := g.Root(); err == nil {
-		return nil
-	}
+	addRootNodeToGraph(g)
+	return nil
+}
 
-	// We intentionally add a graphNodeRoot value -- rather than a pointer to
-	// one -- so that all root nodes will coalesce together if two graphs
-	// are merged. Each distinct node value can only be in a graph once,
-	// so adding another graphNodeRoot value to the same graph later will
-	// be a no-op and all of the edges from root nodes will coalesce together
-	// under Graph.Subsume.
+// addRootNodeToGraph modifies the given graph in-place so that it has a root
+// node if it didn't already have one and so that any other node which doesn't
+// already depend on something will depend on that root node.
+//
+// After this function returns, the graph will have only one node that doesn't
+// depend on any other nodes.
+func addRootNodeToGraph(g *Graph) {
+	// We always add the root node. This is a singleton so if it's already
+	// in the graph this will do nothing and just retain the existing root node.
 	//
-	// It's important to retain this coalescing guarantee under future
-	// maintenence.
-	var root graphNodeRoot
-	g.Add(root)
+	// Note that rootNode is intentionally added by value and not by pointer
+	// so that all root nodes will be equal to one another and therefore
+	// coalesce when two valid graphs get merged together into a single graph.
+	g.Add(rootNode)
 
-	// We initially make the root node depend on every node except itself.
-	// If the caller subsequently runs transitive reduction on the graph then
-	// it's typical for some of these edges to then be removed.
+	// Everything that doesn't already depend on at least one other node will
+	// depend on the root node, except the root node itself.
 	for _, v := range g.Vertices() {
-		if v == root {
+		if v == dag.Vertex(rootNode) {
 			continue
 		}
 
 		if g.UpEdges(v).Len() == 0 {
-			g.Connect(dag.BasicEdge(root, v))
+			g.Connect(dag.BasicEdge(rootNode, v))
 		}
 	}
-
-	return nil
 }
 
 type graphNodeRoot struct{}
 
+// rootNode is the singleton value representing all root graph nodes.
+//
+// The root node for all graphs should be this value directly, and in particular
+// _not_ a pointer to this value. Using the value directly here means that
+// multiple root nodes will always coalesce together when subsuming one graph
+// into another.
+var rootNode graphNodeRoot
+
 func (n graphNodeRoot) Name() string {
 	return rootNodeName
 }

internal/terraform/transform_root_test.go

@@ -8,50 +8,78 @@ import (
 )
 
 func TestRootTransformer(t *testing.T) {
-	mod := testModule(t, "transform-root-basic")
+	t.Run("many nodes", func(t *testing.T) {
+		mod := testModule(t, "transform-root-basic")
 
-	g := Graph{Path: addrs.RootModuleInstance}
-	{
-		tf := &ConfigTransformer{Config: mod}
-		if err := tf.Transform(&g); err != nil {
-			t.Fatalf("err: %s", err)
+		g := Graph{Path: addrs.RootModuleInstance}
+		{
+			tf := &ConfigTransformer{Config: mod}
+			if err := tf.Transform(&g); err != nil {
+				t.Fatalf("err: %s", err)
+			}
 		}
-	}
 
-	{
-		transform := &MissingProviderTransformer{}
-		if err := transform.Transform(&g); err != nil {
-			t.Fatalf("err: %s", err)
+		{
+			transform := &MissingProviderTransformer{}
+			if err := transform.Transform(&g); err != nil {
+				t.Fatalf("err: %s", err)
+			}
 		}
-	}
 
-	{
-		transform := &ProviderTransformer{}
-		if err := transform.Transform(&g); err != nil {
-			t.Fatalf("err: %s", err)
+		{
+			transform := &ProviderTransformer{}
+			if err := transform.Transform(&g); err != nil {
+				t.Fatalf("err: %s", err)
+			}
+		}
+
+		{
+			transform := &RootTransformer{}
+			if err := transform.Transform(&g); err != nil {
+				t.Fatalf("err: %s", err)
+			}
+		}
+
+		actual := strings.TrimSpace(g.String())
+		expected := strings.TrimSpace(testTransformRootBasicStr)
+		if actual != expected {
+			t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)
 		}
-	}
 
-	{
-		transform := &RootTransformer{}
-		if err := transform.Transform(&g); err != nil {
+		root, err := g.Root()
+		if err != nil {
 			t.Fatalf("err: %s", err)
 		}
-	}
+		if _, ok := root.(graphNodeRoot); !ok {
+			t.Fatalf("bad: %#v", root)
+		}
+	})
+
+	t.Run("only one initial node", func(t *testing.T) {
+		g := Graph{Path: addrs.RootModuleInstance}
+		g.Add("foo")
+		addRootNodeToGraph(&g)
+		got := strings.TrimSpace(g.String())
+		want := strings.TrimSpace(`
+foo
+root
+  foo
+`)
+		if got != want {
+			t.Errorf("wrong final graph\ngot:\n%s\nwant:\n%s", got, want)
+		}
+	})
 
-	actual := strings.TrimSpace(g.String())
-	expected := strings.TrimSpace(testTransformRootBasicStr)
-	if actual != expected {
-		t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)
-	}
+	t.Run("graph initially empty", func(t *testing.T) {
+		g := Graph{Path: addrs.RootModuleInstance}
+		addRootNodeToGraph(&g)
+		got := strings.TrimSpace(g.String())
+		want := `root`
+		if got != want {
+			t.Errorf("wrong final graph\ngot:\n%s\nwant:\n%s", got, want)
+		}
+	})
 
-	root, err := g.Root()
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if _, ok := root.(graphNodeRoot); !ok {
-		t.Fatalf("bad: %#v", root)
-	}
 }
 
 const testTransformRootBasicStr = `