core: Include some rejected changes in the plan

Some time ago we ruled that Terraform Core would make a best effort to
return a partial plan describing the subset of actions that were
successfully proposed before encountering an error, which Terraform CLI
and Terraform Cloud then rely on to present some additional context to
support the associated error messages.

This change aims to improve that effort by making a distinction between
the failure of the planning operation itself vs. something else in the
configuration ruling that the successfully-created plan is unacceptable
for some separate reason.

In that case, it's helpful to still include that proposed change in the
plan, because the planning step itself succeeded and these problems are in
a sense "between" the planning operations, blocking any downstream work
from starting.

In particular, this change arranges for a failed prevent_destroy check or
a failed postcondition check to still include the planning result that
they were checked against, which then allows the UI the option of
displaying that planned action alongside the error describing why it was
unacceptable.

This doesn't include any Terraform CLI UI changes, but the UI layer is
already built to show partial plans returned alongside errors and so as
of this change the additional planned changes are already included. It'll
be up to future maintainers of Terraform CLI to decide whether and how
to refine that output, but the existing behavior is sufficient for now.

internal/terraform/context_plan2_test.go

@@ -2898,7 +2898,7 @@ resource "test_resource" "a" {
 			resp.LegacyTypeSystem = true
 			return resp
 		}
-		_, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
+		plan, diags := ctx.Plan(m, states.NewState(), &PlanOpts{
 			Mode: plans.NormalMode,
 			SetVariables: InputValues{
 				"boop": &InputValue{
@@ -2916,6 +2916,27 @@ resource "test_resource" "a" {
 		if !p.PlanResourceChangeCalled {
 			t.Errorf("Provider's PlanResourceChange wasn't called; should've been")
 		}
+
+		if !plan.Errored {
+			t.Errorf("plan is not marked as errored")
+		}
+
+		// The plan should still include a proposed change for the resource
+		// instance whose postcondition failed, since its plan is valid in
+		// isolation even though the postcondition prevents any further
+		// planning downstream. This gives the UI the option of describing
+		// the planned change as additional context alongside the error
+		// message, although it's up to the UI to decide whether and how to
+		// do that.
+		changes := plan.Changes
+		changeSrc := changes.ResourceInstance(mustResourceInstanceAddr("test_resource.a"))
+		if changeSrc != nil {
+			if got, want := changeSrc.Action, plans.Create; got != want {
+				t.Errorf("wrong proposed change action\ngot:  %s\nwant: %s", got, want)
+			}
+		} else {
+			t.Errorf("no planned change for test_resource.a")
+		}
 	})
 
 	t.Run("postcondition fail refresh-only", func(t *testing.T) {

internal/terraform/context_plan_test.go

@@ -1390,6 +1390,30 @@ func TestContext2Plan_preventDestroy_bad(t *testing.T) {
 		}
 		t.Fatalf("expected err would contain %q\nerr: %s", expectedErr, err)
 	}
+
+	// The plan should still include the proposal to replace the object
+	// that cannot be destroyed, since the change is valid in isolation,
+	// and this then allows Terraform CLI and Terraform Cloud to still
+	// show the problematic change that caused the error as additional
+	// context.
+	changes := plan.Changes
+	changeSrc := changes.ResourceInstance(mustResourceInstanceAddr("aws_instance.foo"))
+	if changeSrc != nil {
+		if got, want := changeSrc.Action, plans.DeleteThenCreate; got != want {
+			t.Errorf("wrong proposed change action\ngot:  %s\nwant: %s", got, want)
+		}
+		gotReqRep := changeSrc.RequiredReplace
+		if !gotReqRep.Has(cty.GetAttrPath("require_new")) {
+			t.Errorf("plan does not indicate that the require_new change forced replacement")
+		}
+	} else {
+		t.Errorf("no planned change for aws_instance.foo")
+	}
+	// The plan must also be marked as errored, so that Terraform will reject
+	// any attempts to apply the plan with the forbidden replace action.
+	if got, want := plan.Errored, true; got != want {
+		t.Errorf("plan is not marked as errored\ngot:  %#v\nwant: %#v", got, want)
+	}
 }
 
 func TestContext2Plan_preventDestroy_good(t *testing.T) {

internal/terraform/node_resource_abstract_instance.go

@@ -190,7 +190,7 @@ func (n *NodeAbstractResourceInstance) checkPreventDestroy(change *plans.Resourc
 			Severity: hcl.DiagError,
 			Summary:  "Instance cannot be destroyed",
 			Detail: fmt.Sprintf(
-				"Resource %s has lifecycle.prevent_destroy set, but the plan calls for this resource to be destroyed. To avoid this error and continue with the plan, either disable lifecycle.prevent_destroy or reduce the scope of the plan using the -target flag.",
+				"Resource %s has lifecycle.prevent_destroy set, but the plan calls for this resource to be destroyed. To avoid this error and continue with the plan, either disable lifecycle.prevent_destroy or reduce the scope of the plan using the -target option.",
 				n.Addr.String(),
 			),
 			Subject: &n.Config.DeclRange,

internal/terraform/node_resource_plan_destroy.go

@@ -96,12 +96,18 @@ func (n *NodePlanDestroyableResourceInstance) managedResourceExecute(ctx EvalCon
 		return diags
 	}
 
+	// We intentionally write the change before the subsequent checks, because
+	// all of the checks below this point are for problems caused by the
+	// context surrounding the change, rather than the change itself, and
+	// so it's helpful to still include the valid-in-isolation change as
+	// part of the plan as additional context in our error output.
+	diags = diags.Append(n.writeChange(ctx, change, ""))
+
 	diags = diags.Append(n.checkPreventDestroy(change))
 	if diags.HasErrors() {
 		return diags
 	}
 
-	diags = diags.Append(n.writeChange(ctx, change, ""))
 	return diags
 }
 

internal/terraform/node_resource_plan_instance.go

@@ -286,11 +286,12 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext)
 			change.ActionReason = plans.ResourceInstanceReplaceByTriggers
 		}
 
-		diags = diags.Append(n.checkPreventDestroy(change))
-		if diags.HasErrors() {
-			return diags
-		}
-
+		// We intentionally write the change before the subsequent checks, because
+		// all of the checks below this point are for problems caused by the
+		// context surrounding the change, rather than the change itself, and
+		// so it's helpful to still include the valid-in-isolation change as
+		// part of the plan as additional context in our error output.
+		//
 		// FIXME: it is currently important that we write resource changes to
 		// the plan (n.writeChange) before we write the corresponding state
 		// (n.writeResourceInstanceState).
@@ -306,12 +307,19 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext)
 		// update these two data structures incorrectly through any objects
 		// reachable via the terraform.EvalContext API.
 		diags = diags.Append(n.writeChange(ctx, change, ""))
-
+		if diags.HasErrors() {
+			return diags
+		}
 		diags = diags.Append(n.writeResourceInstanceState(ctx, instancePlanState, workingState))
 		if diags.HasErrors() {
 			return diags
 		}
 
+		diags = diags.Append(n.checkPreventDestroy(change))
+		if diags.HasErrors() {
+			return diags
+		}
+
 		// If this plan resulted in a NoOp, then apply won't have a chance to make
 		// any changes to the stored dependencies. Since this is a NoOp we know
 		// that the stored dependencies will have no effect during apply, and we can

internal/terraform/node_resource_plan_orphan.go

@@ -139,21 +139,26 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon
 		return diags
 	}
 
-	diags = diags.Append(n.checkPreventDestroy(change))
-	if diags.HasErrors() {
-		return diags
-	}
-
 	// We might be able to offer an approximate reason for why we are
 	// planning to delete this object. (This is best-effort; we might
 	// sometimes not have a reason.)
 	change.ActionReason = n.deleteActionReason(ctx)
 
+	// We intentionally write the change before the subsequent checks, because
+	// all of the checks below this point are for problems caused by the
+	// context surrounding the change, rather than the change itself, and
+	// so it's helpful to still include the valid-in-isolation change as
+	// part of the plan as additional context in our error output.
 	diags = diags.Append(n.writeChange(ctx, change, ""))
 	if diags.HasErrors() {
 		return diags
 	}
 
+	diags = diags.Append(n.checkPreventDestroy(change))
+	if diags.HasErrors() {
+		return diags
+	}
+
 	return diags.Append(n.writeResourceInstanceState(ctx, nil, workingState))
 }