refactoring: exhaustive NestedWithin checks

When checking dependencies between statements, we need to check all
combinations of To and From addresses.

internal/refactoring/move_execute.go

@@ -200,10 +200,13 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph {
 	for dependerI := range stmts {
 		depender := &stmts[dependerI]
 		for dependeeI := range stmts {
+			if dependerI == dependeeI {
+				// skip comparing the statement to itself
+				continue
+			}
 			dependee := &stmts[dependeeI]
-			dependeeTo := dependee.To
+
-			dependerFrom := depender.From
+			if statementDependsOn(depender, dependee) {
-			if dependerFrom.CanChainFrom(dependeeTo) || dependerFrom.NestedWithin(dependeeTo) {
 				g.Connect(dag.BasicEdge(depender, dependee))
 			}
 		}
@@ -212,6 +215,36 @@ func buildMoveStatementGraph(stmts []MoveStatement) *dag.AcyclicGraph {
 	return g
 }
 
+// statementDependsOn returns true if statement a depends on statement b;
+// i.e. statement b must be executed before statement a.
+func statementDependsOn(a, b *MoveStatement) bool {
+	// chain-able moves are simple, as on the destination of one move could be
+	// equal to the source of another.
+	if a.From.CanChainFrom(b.To) {
+		return true
+	}
+
+	// Statement nesting in more complex, as we have 8 possible combinations to
+	// assess. Here we list all combinations, along with the statement which
+	// must be executed first when one address is nested within another.
+	// A.From  IsNestedWithin  B.From => A
+	// A.From  IsNestedWithin  B.To   => B
+	// A.To    IsNestedWithin  B.From => A
+	// A.To    IsNestedWithin  B.To   => B
+	// B.From  IsNestedWithin  A.From => B
+	// B.From  IsNestedWithin  A.To   => A
+	// B.To    IsNestedWithin  A.From => B
+	// B.To    IsNestedWithin  A.To   => A
+	//
+	// Since we are only interested in checking if A depends on B, we only need
+	// to check the 4 possibilities above which result in B being executed
+	// first.
+	return a.From.NestedWithin(b.To) ||
+		a.To.NestedWithin(b.To) ||
+		b.From.NestedWithin(a.From) ||
+		b.To.NestedWithin(a.From)
+}
+
 // MoveResults describes the outcome of an ApplyMoves call.
 type MoveResults struct {
 	// Changes is a map from the unique keys of the final new resource

internal/refactoring/move_execute_test.go

@@ -491,118 +491,120 @@ func TestApplyMoves(t *testing.T) {
 				`foo.to[0]`,
 			},
 		},
-
+		"move resource and containing module": {
-		// FIXME: This test seems to flap between the result the test case
+			[]MoveStatement{
-		// currently records and the "more intuitive" results included inline,
+				testMoveStatement(t, "", "module.boo", "module.bar[0]"),
-		// which suggests we have a missing edge in our move dependency graph.
+				testMoveStatement(t, "boo", "foo.from", "foo.to"),
-		// (The MoveResults commented out below predates some changes to that
+			},
-		// struct, so will need updating once we uncomment this test.)
+			states.BuildState(func(s *states.SyncState) {
-		/*
+				s.SetResourceInstanceCurrent(
-			"move module and then move resource into it": {
+					instAddrs["module.boo.foo.from"],
-				[]MoveStatement{
+					&states.ResourceInstanceObjectSrc{
-					testMoveStatement(t, "", "module.bar[0]", "module.boo"),
+						Status:    states.ObjectReady,
-					testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
+						AttrsJSON: []byte(`{}`),
+					},
+					providerAddr,
+				)
+			}),
+			MoveResults{
+				Changes: map[addrs.UniqueKey]MoveSuccess{
+					instAddrs["module.bar[0].foo.to"].UniqueKey(): {
+						From: instAddrs["module.boo.foo.from"],
+						To:   instAddrs["module.bar[0].foo.to"],
+					},
 				},
-				states.BuildState(func(s *states.SyncState) {
+				Blocked: map[addrs.UniqueKey]MoveBlocked{},
-					s.SetResourceInstanceCurrent(
+			},
+			[]string{
+				`module.bar[0].foo.to`,
+			},
+		},
+
+		"move module and then move resource into it": {
+			[]MoveStatement{
+				testMoveStatement(t, "", "module.bar[0]", "module.boo"),
+				testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
+			},
+			states.BuildState(func(s *states.SyncState) {
+				s.SetResourceInstanceCurrent(
+					instAddrs["module.bar[0].foo.to"],
+					&states.ResourceInstanceObjectSrc{
+						Status:    states.ObjectReady,
+						AttrsJSON: []byte(`{}`),
+					},
+					providerAddr,
+				)
+				s.SetResourceInstanceCurrent(
+					instAddrs["foo.from"],
+					&states.ResourceInstanceObjectSrc{
+						Status:    states.ObjectReady,
+						AttrsJSON: []byte(`{}`),
+					},
+					providerAddr,
+				)
+			}),
+			MoveResults{
+				Changes: map[addrs.UniqueKey]MoveSuccess{
+					instAddrs["module.boo.foo.from"].UniqueKey(): {
+						instAddrs["foo.from"],
+						instAddrs["module.boo.foo.from"],
+					},
+					instAddrs["module.boo.foo.to"].UniqueKey(): {
 						instAddrs["module.bar[0].foo.to"],
-						&states.ResourceInstanceObjectSrc{
+						instAddrs["module.boo.foo.to"],
-							Status:    states.ObjectReady,
-							AttrsJSON: []byte(`{}`),
-						},
-						providerAddr,
-					)
-					s.SetResourceInstanceCurrent(
-						instAddrs["foo.from"],
-						&states.ResourceInstanceObjectSrc{
-							Status:    states.ObjectReady,
-							AttrsJSON: []byte(`{}`),
-						},
-						providerAddr,
-					)
-				}),
-				MoveResults{
-					// FIXME: This result is counter-intuitive, because ApplyMoves
-					// handled the resource move first and then the module move
-					// collided with it. It would be arguably more intuitive to
-					// complete the module move first and then let the "smaller"
-					// resource move merge into it.
-					// (The arguably-more-intuitive results are commented out
-					// in the maps below.)
-
-					Changes: map[addrs.UniqueKey]MoveSuccess{
-						//instAddrs["module.boo.foo.to"].UniqueKey():   instAddrs["module.bar[0].foo.to"],
-						//instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"],
-						instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"],
-					},
-					Blocked: map[addrs.UniqueKey]MoveBlocked{
-						// intuitive result: nothing blocked
-						instAddrs["module.bar[0].foo.to"].Module.UniqueKey(): instAddrs["module.boo.foo.from"].Module,
 					},
 				},
-				[]string{
+				Blocked: map[addrs.UniqueKey]MoveBlocked{},
-					//`foo.from`,
-					//`module.boo.foo.from`,
-					`module.bar[0].foo.to`,
-					`module.boo.foo.from`,
-				},
 			},
-		*/
+			[]string{
+				`module.boo.foo.from`,
+				`module.boo.foo.to`,
+			},
+		},
 
-		// FIXME: This test seems to flap between the result the test case
+		"module move collides with resource move": {
-		// currently records and the "more intuitive" results included inline,
+			[]MoveStatement{
-		// which suggests we have a missing edge in our move dependency graph.
+				testMoveStatement(t, "", "module.bar[0]", "module.boo"),
-		// (The MoveResults commented out below predates some changes to that
+				testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
-		// struct, so will need updating once we uncomment this test.)
+			},
-		/*
+			states.BuildState(func(s *states.SyncState) {
-			"module move collides with resource move": {
+				s.SetResourceInstanceCurrent(
-				[]MoveStatement{
+					instAddrs["module.bar[0].foo.from"],
-					testMoveStatement(t, "", "module.bar[0]", "module.boo"),
+					&states.ResourceInstanceObjectSrc{
-					testMoveStatement(t, "", "foo.from", "module.boo.foo.from"),
+						Status:    states.ObjectReady,
-				},
+						AttrsJSON: []byte(`{}`),
-				states.BuildState(func(s *states.SyncState) {
+					},
-					s.SetResourceInstanceCurrent(
+					providerAddr,
+				)
+				s.SetResourceInstanceCurrent(
+					instAddrs["foo.from"],
+					&states.ResourceInstanceObjectSrc{
+						Status:    states.ObjectReady,
+						AttrsJSON: []byte(`{}`),
+					},
+					providerAddr,
+				)
+			}),
+			MoveResults{
+				Changes: map[addrs.UniqueKey]MoveSuccess{
+
+					instAddrs["module.boo.foo.from"].UniqueKey(): {
 						instAddrs["module.bar[0].foo.from"],
-						&states.ResourceInstanceObjectSrc{
+						instAddrs["module.boo.foo.from"],
-							Status:    states.ObjectReady,
-							AttrsJSON: []byte(`{}`),
-						},
-						providerAddr,
-					)
-					s.SetResourceInstanceCurrent(
-						instAddrs["foo.from"],
-						&states.ResourceInstanceObjectSrc{
-							Status:    states.ObjectReady,
-							AttrsJSON: []byte(`{}`),
-						},
-						providerAddr,
-					)
-				}),
-				MoveResults{
-					// FIXME: This result is counter-intuitive, because ApplyMoves
-					// handled the resource move first and then it was the
-					// module move that collided, whereas it would arguably have
-					// been better to let the module take priority and have only
-					// the one resource move be ignored due to the collision.
-					// (The arguably-more-intuitive results are commented out
-					// in the maps below.)
-					Changes: map[addrs.UniqueKey]MoveSuccess{
-						//instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["module.bar[0].foo.from"],
-						instAddrs["module.boo.foo.from"].UniqueKey(): instAddrs["foo.from"],
-					},
-					Blocked: map[addrs.UniqueKey]MoveBlocked{
-						//instAddrs["foo.from"].UniqueKey(): instAddrs["module.bar[0].foo.from"],
-						instAddrs["module.bar[0].foo.from"].Module.UniqueKey(): instAddrs["module.boo.foo.from"].Module,
 					},
 				},
-				[]string{
+				Blocked: map[addrs.UniqueKey]MoveBlocked{
-					//`foo.from`,
+					instAddrs["foo.from"].ContainingResource().UniqueKey(): {
-					//`module.boo.foo.from`,
+						Actual: instAddrs["foo.from"].ContainingResource(),
-					`module.bar[0].foo.from`,
+						Wanted: instAddrs["module.boo.foo.from"].ContainingResource(),
-					`module.boo.foo.from`,
+					},
 				},
 			},
-		*/
+			[]string{
+				`foo.from`,
+				`module.boo.foo.from`,
+			},
+		},
 	}
 
 	for name, test := range tests {