cleanup some move graph handling

Create a separate `validateMoveStatementGraph` function so that
`ValidateMoves` and `ApplyMoves` both check the same conditions. Since
we're not using the builtin `graph.Validate` method, because we may have
multiple roots and want better cycle diagnostics, we need to add checks
for self references too. While multiple roots are an error enforced by
`Validate` for the concurrent walk, they are OK when using
`TransitiveReduction` and `ReverseDepthFirstWalk`, so we can skip that
check.

Apply moves must first use `TransitiveReduction` to reduce the graph,
otherwise nodes may be skipped if they are passed over by a transitive
edge.

internal/refactoring/move_execute.go

@@ -31,6 +31,10 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults {
 		Blocked: make(map[addrs.UniqueKey]MoveBlocked),
 	}
 
+	if len(stmts) == 0 {
+		return ret
+	}
+
 	// The methodology here is to construct a small graph of all of the move
 	// statements where the edges represent where a particular statement
 	// is either chained from or nested inside the effect of another statement.
@@ -39,13 +43,18 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults {
 
 	g := buildMoveStatementGraph(stmts)
 
-	// If there are any cycles in the graph then we'll not take any action
-	// at all. The separate validation step should detect this and return
-	// an error.
-	if len(g.Cycles()) != 0 {
+	// If the graph is not valid the we will not take any action at all. The
+	// separate validation step should detect this and return an error.
+	if diags := validateMoveStatementGraph(g); diags.HasErrors() {
+		log.Printf("[ERROR] ApplyMoves: %s", diags.ErrWithWarnings())
 		return ret
 	}
 
+	// The graph must be reduced in order for ReverseDepthFirstWalk to work
+	// correctly, since it is built from following edges and can skip over
+	// dependencies if there is a direct edge to a transitive dependency.
+	g.TransitiveReduction()
+
 	// The starting nodes are the ones that don't depend on any other nodes.
 	startNodes := make(dag.Set, len(stmts))
 	for _, v := range g.Vertices() {

internal/refactoring/move_validate.go

@@ -8,6 +8,7 @@ import (
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
+	"github.com/hashicorp/terraform/internal/dag"
 	"github.com/hashicorp/terraform/internal/instances"
 	"github.com/hashicorp/terraform/internal/tfdiags"
 )
@@ -31,6 +32,10 @@ import (
 func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts instances.Set) tfdiags.Diagnostics {
 	var diags tfdiags.Diagnostics
 
+	if len(stmts) == 0 {
+		return diags
+	}
+
 	g := buildMoveStatementGraph(stmts)
 
 	// We need to track the absolute versions of our endpoint addresses in
@@ -200,30 +205,54 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts
 	// validation rules above where we can make better suggestions, and so
 	// we'll use a cycle report only as a last resort.
 	if !diags.HasErrors() {
-		for _, cycle := range g.Cycles() {
-			// Reporting cycles is awkward because there isn't any definitive
-			// way to decide which of the objects in the cycle is the cause of
-			// the problem. Therefore we'll just list them all out and leave
-			// the user to figure it out. :(
-			stmtStrs := make([]string, 0, len(cycle))
-			for _, stmtI := range cycle {
-				// move statement graph nodes are pointers to move statements
-				stmt := stmtI.(*MoveStatement)
-				stmtStrs = append(stmtStrs, fmt.Sprintf(
-					"\n  - %s: %s → %s",
-					stmt.DeclRange.StartString(),
-					stmt.From.String(),
-					stmt.To.String(),
-				))
-			}
-			sort.Strings(stmtStrs) // just to make the order deterministic
+		diags = diags.Append(validateMoveStatementGraph(g))
+	}
 
+	return diags
+}
+
+func validateMoveStatementGraph(g *dag.AcyclicGraph) tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+	for _, cycle := range g.Cycles() {
+		// Reporting cycles is awkward because there isn't any definitive
+		// way to decide which of the objects in the cycle is the cause of
+		// the problem. Therefore we'll just list them all out and leave
+		// the user to figure it out. :(
+		stmtStrs := make([]string, 0, len(cycle))
+		for _, stmtI := range cycle {
+			// move statement graph nodes are pointers to move statements
+			stmt := stmtI.(*MoveStatement)
+			stmtStrs = append(stmtStrs, fmt.Sprintf(
+				"\n  - %s: %s → %s",
+				stmt.DeclRange.StartString(),
+				stmt.From.String(),
+				stmt.To.String(),
+			))
+		}
+		sort.Strings(stmtStrs) // just to make the order deterministic
+
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Cyclic dependency in move statements",
+			fmt.Sprintf(
+				"The following chained move statements form a cycle, and so there is no final location to move objects to:%s\n\nA chain of move statements must end with an address that doesn't appear in any other statements, and which typically also refers to an object still declared in the configuration.",
+				strings.Join(stmtStrs, ""),
+			),
+		))
+	}
+
+	// Look for cycles to self.
+	// A user shouldn't be able to create self-references, but we cannot
+	// correctly process a graph with them.
+	for _, e := range g.Edges() {
+		src := e.Source()
+		if src == e.Target() {
 			diags = diags.Append(tfdiags.Sourceless(
 				tfdiags.Error,
-				"Cyclic dependency in move statements",
+				"Self reference in move statements",
 				fmt.Sprintf(
-					"The following chained move statements form a cycle, and so there is no final location to move objects to:%s\n\nA chain of move statements must end with an address that doesn't appear in any other statements, and which typically also refers to an object still declared in the configuration.",
-					strings.Join(stmtStrs, ""),
+					"The move statement %s refers to itself the move dependency graph, which is invalid. This is a bug in Terraform; please report it!",
+					src.(*MoveStatement).Name(),
 				),
 			))
 		}