core: Emit only one warning for move collisions in destroy-plan mode

Our current implementation of destroy planning includes secretly running a
normal plan first, in order to get its effect of refreshing the state.

Previously our warning about colliding moves would betray that
implementation detail because we'd return it from both of our planning
operations here and thus show the message twice. That would also have
happened in theory for any other warnings emitted by both plan operations,
but it's the move collision warning that made it immediately visible.

We'll now only return warnings from the initial plan if we're also
returning errors from that plan, and thus the warnings of both plans can
never mix together into the same diags and thus we'll avoid duplicating
any warnings.

This does mean that we'd lose any warnings which might hypothetically
emerge only from the hidden normal plan and not from the subsequent
destroy plan, but we'll accept that as an okay tradeoff here because those
warnings are likely to not be super relevant to the destroy case anyway,
or else we'd emit them from the destroy-plan walk too.

internal/terraform/context_plan.go

@@ -262,8 +262,19 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State
 		normalOpts := *opts
 		normalOpts.Mode = plans.NormalMode
 		refreshPlan, refreshDiags := c.plan(config, prevRunState, rootVariables, &normalOpts)
-		diags = diags.Append(refreshDiags)
-		if diags.HasErrors() {
+		if refreshDiags.HasErrors() {
+			// NOTE: Normally we'd append diagnostics regardless of whether
+			// there are errors, just in case there are warnings we'd want to
+			// preserve, but we're intentionally _not_ doing that here because
+			// if the first plan succeeded then we'll be running another plan
+			// in DestroyMode below, and we don't want to double-up any
+			// warnings that both plan walks would generate.
+			// (This does mean we won't show any warnings that would've been
+			// unique to only this walk, but we're assuming here that if the
+			// warnings aren't also applicable to a destroy plan then we'd
+			// rather not show them here, because this non-destroy plan for
+			// refreshing is largely an implementation detail.)
+			diags = diags.Append(refreshDiags)
 			return nil, diags
 		}
 

internal/terraform/context_plan2_test.go

@@ -887,6 +887,124 @@ Terraform has planned to destroy these objects. If Terraform's proposed changes
 	})
 }
 
+func TestContext2Plan_movedResourceCollisionDestroy(t *testing.T) {
+	// This is like TestContext2Plan_movedResourceCollision but intended to
+	// ensure we still produce the expected warning (and produce it only once)
+	// when we're creating a destroy plan, rather than a normal plan.
+	// (This case is interesting at the time of writing because we happen to
+	// use a normal plan as a trick to refresh before creating a destroy plan.
+	// This test will probably become uninteresting if a future change to
+	// the destroy-time planning behavior handles refreshing in a different
+	// way, which avoids this pre-processing step of running a normal plan
+	// first.)
+
+	addrNoKey := mustResourceInstanceAddr("test_object.a")
+	addrZeroKey := mustResourceInstanceAddr("test_object.a[0]")
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+			resource "test_object" "a" {
+				# No "count" set, so test_object.a[0] will want
+				# to implicitly move to test_object.a, but will get
+				# blocked by the existing object at that address.
+			}
+		`,
+	})
+
+	state := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(addrNoKey, &states.ResourceInstanceObjectSrc{
+			AttrsJSON: []byte(`{}`),
+			Status:    states.ObjectReady,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+		s.SetResourceInstanceCurrent(addrZeroKey, &states.ResourceInstanceObjectSrc{
+			AttrsJSON: []byte(`{}`),
+			Status:    states.ObjectReady,
+		}, mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`))
+	})
+
+	p := simpleMockProvider()
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("test"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags := ctx.Plan(m, state, &PlanOpts{
+		Mode: plans.DestroyMode,
+	})
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
+	}
+
+	// We should have a warning, though! We'll lightly abuse the "for RPC"
+	// feature of diagnostics to get some more-readily-comparable diagnostic
+	// values.
+	gotDiags := diags.ForRPC()
+	wantDiags := tfdiags.Diagnostics{
+		tfdiags.Sourceless(
+			tfdiags.Warning,
+			"Unresolved resource instance address changes",
+			// NOTE: This message is _lightly_ confusing in the destroy case,
+			// because it says "Terraform has planned to destroy these objects"
+			// but this is a plan to destroy all objects, anyway. We expect the
+			// conflict situation to be pretty rare though, and even rarer in
+			// a "terraform destroy", so we'll just live with that for now
+			// unless we see evidence that lots of folks are being confused by
+			// it in practice.
+			`Terraform tried to adjust resource instance addresses in the prior state based on change information recorded in the configuration, but some adjustments did not succeed due to existing objects already at the intended addresses:
+  - test_object.a[0] could not move to test_object.a
+
+Terraform has planned to destroy these objects. If Terraform's proposed changes aren't appropriate, you must first resolve the conflicts using the "terraform state" subcommands and then create a new plan.`,
+		),
+	}.ForRPC()
+	if diff := cmp.Diff(wantDiags, gotDiags); diff != "" {
+		// If we get here with a diff that makes it seem like the above warning
+		// is being reported twice, the likely cause is not correctly handling
+		// the warnings from the hidden normal plan we run as part of preparing
+		// for a destroy plan, unless that strategy has changed in the meantime
+		// since we originally wrote this test.
+		t.Errorf("wrong diagnostics\n%s", diff)
+	}
+
+	t.Run(addrNoKey.String(), func(t *testing.T) {
+		instPlan := plan.Changes.ResourceInstance(addrNoKey)
+		if instPlan == nil {
+			t.Fatalf("no plan for %s at all", addrNoKey)
+		}
+
+		if got, want := instPlan.Addr, addrNoKey; !got.Equal(want) {
+			t.Errorf("wrong current address\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := instPlan.PrevRunAddr, addrNoKey; !got.Equal(want) {
+			t.Errorf("wrong previous run address\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := instPlan.Action, plans.Delete; got != want {
+			t.Errorf("wrong planned action\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := instPlan.ActionReason, plans.ResourceInstanceChangeNoReason; got != want {
+			t.Errorf("wrong action reason\ngot:  %s\nwant: %s", got, want)
+		}
+	})
+	t.Run(addrZeroKey.String(), func(t *testing.T) {
+		instPlan := plan.Changes.ResourceInstance(addrZeroKey)
+		if instPlan == nil {
+			t.Fatalf("no plan for %s at all", addrZeroKey)
+		}
+
+		if got, want := instPlan.Addr, addrZeroKey; !got.Equal(want) {
+			t.Errorf("wrong current address\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := instPlan.PrevRunAddr, addrZeroKey; !got.Equal(want) {
+			t.Errorf("wrong previous run address\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := instPlan.Action, plans.Delete; got != want {
+			t.Errorf("wrong planned action\ngot:  %s\nwant: %s", got, want)
+		}
+		if got, want := instPlan.ActionReason, plans.ResourceInstanceChangeNoReason; got != want {
+			t.Errorf("wrong action reason\ngot:  %s\nwant: %s", got, want)
+		}
+	})
+}
+
 func TestContext2Plan_movedResourceUntargeted(t *testing.T) {
 	addrA := mustResourceInstanceAddr("test_object.a")
 	addrB := mustResourceInstanceAddr("test_object.b")