Merge pull request #30253 from hashicorp/jbardin/move-graph

cleanup some move graph handling

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,6 +205,14 @@ 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() {
+		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
@@ -227,6 +240,22 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts
 			),
 		))
 	}
+
+	// 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,
+				"Self reference in move statements",
+				fmt.Sprintf(
+					"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(),
+				),
+			))
+		}
 	}
 
 	return diags