Merge pull request #29943 from hashicorp/kmoe/moved-block-resource-type

refactoring: error when changing resource type during move

internal/addrs/move_endpoint.go

@@ -19,7 +19,7 @@ import (
 // prompt creating a different plan than Terraform would by default.
 //
 // To obtain a full address from a MoveEndpoint you must use
-// either the package function UnifyMoveEndpoints (to get an AbsMovable) or
+// either the package function UnifyMoveEndpoints (to get an AbsMoveable) or
 // the method ConfigMoveable (to get a ConfigMoveable).
 type MoveEndpoint struct {
 	// SourceRange is the location of the physical endpoint address
@@ -27,15 +27,15 @@ type MoveEndpoint struct {
 	// configuration expresson.
 	SourceRange tfdiags.SourceRange
 
-	// Internally we (ab)use AbsMovable as the representation of our
+	// Internally we (ab)use AbsMoveable as the representation of our
 	// relative address, even though everywhere else in Terraform
-	// AbsMovable always represents a fully-absolute address.
+	// AbsMoveable always represents a fully-absolute address.
 	// In practice, due to the implementation of ParseMoveEndpoint,
 	// this is always either a ModuleInstance or an AbsResourceInstance,
 	// and we only consider the possibility of interpreting it as
 	// a AbsModuleCall or an AbsResource in UnifyMoveEndpoints.
 	// This is intentionally unexported to encapsulate this unusual
-	// meaning of AbsMovable.
+	// meaning of AbsMoveable.
 	relSubject AbsMoveable
 }
 
@@ -44,7 +44,7 @@ func (e *MoveEndpoint) ObjectKind() MoveEndpointKind {
 }
 
 func (e *MoveEndpoint) String() string {
-	// Our internal pseudo-AbsMovable representing the relative
+	// Our internal pseudo-AbsMoveable representing the relative
 	// address (either ModuleInstance or AbsResourceInstance) is
 	// a good enough proxy for the relative move endpoint address
 	// serialization.

internal/addrs/move_endpoint_module.go

@@ -131,7 +131,7 @@ func (e *MoveEndpointInModule) Module() Module {
 	return e.module
 }
 
-// InModuleInstance returns an AbsMovable address which concatenates the
+// InModuleInstance returns an AbsMoveable address which concatenates the
 // given module instance address with the receiver's relative object selection
 // to produce one example of an instance that might be affected by this
 // move statement.

internal/addrs/move_endpoint_test.go

@@ -12,7 +12,7 @@ import (
 func TestParseMoveEndpoint(t *testing.T) {
 	tests := []struct {
 		Input   string
-		WantRel AbsMoveable // funny intermediate subset of AbsMovable
+		WantRel AbsMoveable // funny intermediate subset of AbsMoveable
 		WantErr string
 	}{
 		{

internal/addrs/moveable.go

@@ -5,7 +5,7 @@ package addrs
 // with any other similar cross-module state refactoring statements we might
 // allow.
 //
-// Note that AbsMovable represents an absolute address relative to the root
+// Note that AbsMoveable represents an absolute address relative to the root
 // of the configuration, which is different than the direct representation
 // of these in configuration where the author gives an address relative to
 // the current module where the address is defined. The type MoveEndpoint
@@ -16,7 +16,7 @@ type AbsMoveable interface {
 	String() string
 }
 
-// The following are all of the possible AbsMovable address types:
+// The following are all of the possible AbsMoveable address types:
 var (
 	_ AbsMoveable = AbsResource{}
 	_ AbsMoveable = AbsResourceInstance{}
@@ -24,6 +24,19 @@ var (
 	_ AbsMoveable = AbsModuleCall{}
 )
 
+// AbsMoveableResource is an AbsMoveable that is either a resource or a resource
+// instance.
+type AbsMoveableResource interface {
+	AbsMoveable
+	AffectedAbsResource() AbsResource
+}
+
+// The following are all of the possible AbsMoveableResource types:
+var (
+	_ AbsMoveableResource = AbsResource{}
+	_ AbsMoveableResource = AbsResourceInstance{}
+)
+
 // ConfigMoveable is similar to AbsMoveable but represents a static object in
 // the configuration, rather than an instance of that object created by
 // module expansion.

internal/addrs/resource.go

@@ -186,6 +186,11 @@ func (r AbsResource) String() string {
 	return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String())
 }
 
+// AffectedAbsResource returns the AbsResource.
+func (r AbsResource) AffectedAbsResource() AbsResource {
+	return r
+}
+
 func (r AbsResource) Equal(o AbsResource) bool {
 	return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource)
 }
@@ -267,6 +272,14 @@ func (r AbsResourceInstance) String() string {
 	return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String())
 }
 
+// AffectedAbsResource returns the AbsResource for the instance.
+func (r AbsResourceInstance) AffectedAbsResource() AbsResource {
+	return AbsResource{
+		Module:   r.Module,
+		Resource: r.Resource.Resource,
+	}
+}
+
 func (r AbsResourceInstance) Equal(o AbsResourceInstance) bool {
 	return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource)
 }

internal/configs/moved_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/hashicorp/terraform/internal/addrs"
 )
 
-func TestDecodeMovedBlock(t *testing.T) {
+func TestMovedBlock_decode(t *testing.T) {
 	blockRange := hcl.Range{
 		Filename: "mock.tf",
 		Start:    hcl.Pos{Line: 3, Column: 12, Byte: 27},
@@ -169,7 +169,7 @@ func TestDecodeMovedBlock(t *testing.T) {
 	}
 }
 
-func TestMovedBlocksInModule(t *testing.T) {
+func TestMovedBlock_inModule(t *testing.T) {
 	parser := NewParser(nil)
 	mod, diags := parser.LoadConfigDir("testdata/valid-modules/moved-blocks")
 	if diags.HasErrors() {

internal/refactoring/move_validate.go

@@ -110,7 +110,7 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts
 				shortNoun = "resource"
 			default:
 				// The above cases should cover all of the AbsMoveable types
-				panic("unsupported AbsMovable address type")
+				panic("unsupported AbsMoveable address type")
 			}
 
 			// It's invalid to have a move statement whose "from" address
@@ -178,6 +178,16 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts
 				}
 			}
 
+			// Resource types must match.
+			if resourceTypesDiffer(absFrom, absTo) {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Resource type mismatch",
+					Detail: fmt.Sprintf(
+						"This statement declares a move from %s to %s, which is a %s of a different type.", absFrom, absTo, noun,
+					),
+				})
+			}
 		}
 	}
 
@@ -234,7 +244,20 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool {
 		return in.HasResource(addr)
 	default:
 		// The above cases should cover all of the AbsMoveable types
-		panic("unsupported AbsMovable address type")
+		panic("unsupported AbsMoveable address type")
+	}
+}
+
+func resourceTypesDiffer(absFrom, absTo addrs.AbsMoveable) bool {
+	switch absFrom := absFrom.(type) {
+	case addrs.AbsMoveableResource:
+		// addrs.UnifyMoveEndpoints guarantees that both addresses are of the
+		// same kind, so at this point we can assume that absTo is also an
+		// addrs.AbsResourceInstance or addrs.AbsResource.
+		absTo := absTo.(addrs.AbsMoveableResource)
+		return absFrom.AffectedAbsResource().Resource.Type != absTo.AffectedAbsResource().Resource.Type
+	default:
+		return false
 	}
 }
 
@@ -309,7 +332,7 @@ func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiag
 		return tfdiags.SourceRangeFromHCL(rc.DeclRange), true
 	default:
 		// The above cases should cover all of the AbsMoveable types
-		panic("unsupported AbsMovable address type")
+		panic("unsupported AbsMoveable address type")
 	}
 }
 

internal/refactoring/move_validate_test.go

@@ -386,6 +386,24 @@ Each resource can have moved from only one source resource.`,
 			},
 			WantError: ``, // This is okay because the call itself is not considered to be inside the package it refers to
 		},
+		"resource type mismatch": {
+			Statements: []MoveStatement{
+				makeTestMoveStmt(t, ``,
+					`test.nonexist1`,
+					`other.single`,
+				),
+			},
+			WantError: `Resource type mismatch: This statement declares a move from test.nonexist1 to other.single, which is a resource of a different type.`,
+		},
+		"resource instance type mismatch": {
+			Statements: []MoveStatement{
+				makeTestMoveStmt(t, ``,
+					`test.nonexist1[0]`,
+					`other.single`,
+				),
+			},
+			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.`,
+		},
 	}
 
 	for name, test := range tests {

internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf

@@ -41,6 +41,9 @@ resource "test" "for_each" {
   }
 }
 
+resource "other" "single" {
+}
+
 module "fake_external" {
   # Our configuration fixture loader has a special case for a module call
   # named "fake_external" where it will mutate the source address after