check for nested module index changes

Changing only the index on a nested module will cause all nested moves
to create cycles, since their full addresses will match both the From
and To addresses. When building the dependency graph, check if the
parent is only changing the index of the containing module, and prevent
the backwards edge for the move.

internal/addrs/move_endpoint_module.go

@@ -705,10 +705,10 @@ func (r AbsResourceInstance) MoveDestination(fromMatch, toMatch *MoveEndpointInM
 	}
 }
 
-// IsModuleMoveReIndex takes the from and to endpoints from a move statement,
+// IsModuleReIndex takes the From and To endpoints from a single move
-// and returns true if the only changes are to module indexes, and all
+// statement, and returns true if the only changes are to module indexes, and
-// non-absolute paths remain the same.
+// all non-absolute paths remain the same.
-func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool {
+func (from *MoveEndpointInModule) IsModuleReIndex(to *MoveEndpointInModule) bool {
 	// The statements must originate from the same module.
 	if !from.module.Equal(to.module) {
 		panic("cannot compare move expressions from different modules")
@@ -718,37 +718,21 @@ func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool {
 	case AbsModuleCall:
 		switch t := to.relSubject.(type) {
 		case ModuleInstance:
-			if len(t) != 1 {
+			// Generate a synthetic module to represent the full address of
-				// An AbsModuleCall only ever has one segment, so the
+			// the module call. We're not actually comparing indexes, so the
-				// ModuleInstance length must match.
+			// instance doesn't matter.
-				return false
+			callAddr := f.Instance(NoKey).Module()
-			}
+			return callAddr.Equal(t.Module())
-
-			return f.Call.Name == t[0].Name
 		}
 
 	case ModuleInstance:
 		switch t := to.relSubject.(type) {
 		case AbsModuleCall:
-			if len(f) != 1 {
+			callAddr := t.Instance(NoKey).Module()
-				return false
+			return callAddr.Equal(f.Module())
-			}
-
-			return f[0].Name == t.Call.Name
 
 		case ModuleInstance:
-			// We must have the same number of segments, and the names must all
+			return t.Module().Equal(f.Module())
-			// match in order for this to solely be an index change operation.
-			if len(f) != len(t) {
-				return false
-			}
-
-			for i := range f {
-				if f[i].Name != t[i].Name {
-					return false
-				}
-			}
-			return true
 		}
 	}
 

internal/addrs/move_endpoint_module_test.go

@@ -1686,6 +1686,25 @@ func TestIsModuleMoveReIndex(t *testing.T) {
 			},
 			expect: false,
 		},
+
+		{
+			from: AbsModuleCall{
+				Module: mustParseModuleInstanceStr(`module.bar[0]`),
+				Call:   ModuleCall{Name: "baz"},
+			},
+			to:     mustParseModuleInstanceStr(`module.bar.module.baz[0]`),
+			expect: true,
+		},
+
+		{
+			from: mustParseModuleInstanceStr(`module.bar.module.baz[0]`),
+			to: AbsModuleCall{
+				Module: mustParseModuleInstanceStr(`module.bar[0]`),
+				Call:   ModuleCall{Name: "baz"},
+			},
+			expect: true,
+		},
+
 		{
 			from:   mustParseModuleInstanceStr(`module.baz`),
 			to:     mustParseModuleInstanceStr(`module.bar.module.baz[0]`),
@@ -1709,7 +1728,7 @@ func TestIsModuleMoveReIndex(t *testing.T) {
 					relSubject: test.to,
 				}
 
-				if got := IsModuleMoveReIndex(from, to); got != test.expect {
+				if got := from.IsModuleReIndex(to); got != test.expect {
 					t.Errorf("expected %t, got %t", test.expect, got)
 				}
 			},

internal/refactoring/move_execute.go

@@ -238,11 +238,31 @@ func statementDependsOn(a, b *MoveStatement) bool {
 	//
 	// 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.
+	// first. If we're there's no dependency at all we can return immediately.
-	return a.From.NestedWithin(b.To) ||
+	if !(a.From.NestedWithin(b.To) || a.To.NestedWithin(b.To) ||
-		a.To.NestedWithin(b.To) ||
+		b.From.NestedWithin(a.From) || b.To.NestedWithin(a.From)) {
-		b.From.NestedWithin(a.From) ||
+		return false
-		b.To.NestedWithin(a.From)
+	}
+
+	// If a nested move has a dependency, we need to rule out the possibility
+	// that this is a move inside a module only changing indexes. If an
+	// ancestor module is only changing the index of a nested module, any
+	// nested move statements are going to match both the From and To address
+	// when the base name is not changing, causing a cycle in the order of
+	// operations.
+
+	// if A is not declared in an ancestor module, then we can't be nested
+	// within a module index change.
+	if len(a.To.Module()) >= len(b.To.Module()) {
+		return true
+	}
+	// We only want the nested move statement to depend on the outer module
+	// move, so we only test this in the reverse direction.
+	if a.From.IsModuleReIndex(a.To) {
+		return false
+	}
+
+	return true
 }
 
 // MoveResults describes the outcome of an ApplyMoves call.

internal/refactoring/move_validate_test.go

@@ -404,6 +404,58 @@ Each resource can have moved from only one source resource.`,
 			},
 			WantError: `Resource type mismatch: This statement declares a move from test.nonexist1[0] to other.single, which is a resource instance of a different type.`,
 		},
+		"crossing nested statements": {
+			// overlapping nested moves will result in a cycle.
+			Statements: []MoveStatement{
+				makeTestMoveStmt(t, ``,
+					`module.nonexist.test.single`,
+					`module.count[0].test.count[0]`,
+				),
+				makeTestMoveStmt(t, ``,
+					`module.nonexist`,
+					`module.count[0]`,
+				),
+			},
+			WantError: `Cyclic dependency in move statements: The following chained move statements form a cycle, and so there is no final location to move objects to:
+  - test:1,1: module.nonexist → module.count[0]
+  - test:1,1: module.nonexist.test.single → module.count[0].test.count[0]
+
+A 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.`,
+		},
+		"fully contained nested statements": {
+			// we have to avoid a cycle because the nested moves appear in both
+			// the from and to address of the parent when only the module index
+			// is changing.
+			Statements: []MoveStatement{
+				makeTestMoveStmt(t, `count`,
+					`test.count`,
+					`test.count[0]`,
+				),
+				makeTestMoveStmt(t, ``,
+					`module.count`,
+					`module.count[0]`,
+				),
+			},
+		},
+		"double fully contained nested statements": {
+			// we have to avoid a cycle because the nested moves appear in both
+			// the from and to address of the parent when only the module index
+			// is changing.
+			Statements: []MoveStatement{
+				makeTestMoveStmt(t, `count`,
+					`module.count`,
+					`module.count[0]`,
+				),
+				makeTestMoveStmt(t, `count.count`,
+					`test.count`,
+					`test.count[0]`,
+				),
+				makeTestMoveStmt(t, ``,
+					`module.count`,
+					`module.count[0]`,
+				),
+			},
+		},
 	}
 
 	for name, test := range tests {