From 7b99861b1c0acf65ab994e06be13eb08ac6f353f Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Wed, 22 Sep 2021 16:57:01 -0700
Subject: [PATCH] refactoring: Don't implicitly move for resources with
 for_each

Our previous rule for implicitly moving from IntKey(0) to NoKey would
apply that move even when the current resource configuration uses
for_each, because we were only considering whether "count" were set.

Previously this was relatively harmless because the resource instance in
question would end up planned for deletion anyway: neither an IntKey nor
a NoKey are valid keys for for_each.

Now that we're going to be announcing these moves explicitly in the UI,
it would be confusing to see Terraform report that IntKey moved to NoKey
in a situation where the config changed from count to for_each, so to
address that we'll only generate the implied statement if neither
repetition argument is set.
---
 internal/refactoring/move_statement.go             |  2 +-
 internal/refactoring/move_statement_test.go        | 14 ++++++++++++--
 .../move-statement-implied.tf                      |  8 ++++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/internal/refactoring/move_statement.go b/internal/refactoring/move_statement.go
index baaf9d519..a363602c3 100644
--- a/internal/refactoring/move_statement.go
+++ b/internal/refactoring/move_statement.go
@@ -124,7 +124,7 @@ func impliedMoveStatements(cfg *configs.Config, prevRunState *states.State, expl
 					fromKey = addrs.NoKey
 					toKey = addrs.IntKey(0)
 				}
-			default:
+			case rCfg.Count == nil && rCfg.ForEach == nil: // no repetition at all
 				if riState := rState.Instances[addrs.IntKey(0)]; riState != nil {
 					fromKey = addrs.IntKey(0)
 					toKey = addrs.NoKey
diff --git a/internal/refactoring/move_statement_test.go b/internal/refactoring/move_statement_test.go
index 93164f94c..9724b1fe1 100644
--- a/internal/refactoring/move_statement_test.go
+++ b/internal/refactoring/move_statement_test.go
@@ -58,6 +58,16 @@ func TestImpliedMoveStatements(t *testing.T) {
 			instObjState(),
 			providerAddr,
 		)
+		s.SetResourceInstanceCurrent(
+			resourceAddr("now_for_each_formerly_count").Instance(addrs.IntKey(0)),
+			instObjState(),
+			providerAddr,
+		)
+		s.SetResourceInstanceCurrent(
+			resourceAddr("now_for_each_formerly_no_count").Instance(addrs.NoKey),
+			instObjState(),
+			providerAddr,
+		)
 
 		// This "ambiguous" resource is representing a rare but possible
 		// situation where we end up having a mixture of different index
@@ -113,8 +123,8 @@ func TestImpliedMoveStatements(t *testing.T) {
 			Implied: true,
 			DeclRange: tfdiags.SourceRange{
 				Filename: "testdata/move-statement-implied/move-statement-implied.tf",
-				Start:    tfdiags.SourcePos{Line: 42, Column: 1, Byte: 709},
-				End:      tfdiags.SourcePos{Line: 42, Column: 27, Byte: 735},
+				Start:    tfdiags.SourcePos{Line: 50, Column: 1, Byte: 858},
+				End:      tfdiags.SourcePos{Line: 50, Column: 27, Byte: 884},
 			},
 		},
 	}
diff --git a/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf b/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf
index 1de3238bd..142ffe702 100644
--- a/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf
+++ b/internal/refactoring/testdata/move-statement-implied/move-statement-implied.tf
@@ -39,6 +39,14 @@ moved {
   to   = foo.now_count_explicit[1]
 }
 
+resource "foo" "now_for_each_formerly_count" {
+  for_each = { a = 1 }
+}
+
+resource "foo" "now_for_each_formerly_no_count" {
+  for_each = { a = 1 }
+}
+
 resource "foo" "ambiguous" {
   # this one doesn't have count in the config, but the test should
   # set it up to have both no-key and zero-key instances in the