Include additional proposed change information for certain kinds of planning errors

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

@@ -177,7 +177,7 @@ func (n *NodeAbstractResourceInstance) readDiff(ctx EvalContext, providerSchema
 	return change, nil
 }
 
-func (n *NodeAbstractResourceInstance) checkPreventDestroy(change *plans.ResourceInstanceChange) error {
+func (n *NodeAbstractResourceInstance) checkPreventDestroy(change *plans.ResourceInstanceChange) tfdiags.Diagnostics {
 	if change == nil || n.Config == nil || n.Config.Managed == nil {
 		return nil
 	}
@@ -190,12 +190,12 @@ 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,
 		})
-		return diags.Err()
+		return diags
 	}
 
 	return nil

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

@@ -162,8 +162,6 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon
 		if diags.HasErrors() {
 			return diags
 		}
-
-		diags = diags.Append(n.checkPreventDestroy(change))
 	}
 	if diags.HasErrors() {
 		return diags
@@ -174,11 +172,23 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon
 	// 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
 	}
 
+	if !forget {
+		diags = diags.Append(n.checkPreventDestroy(change))
+		if diags.HasErrors() {
+			return diags
+		}
+	}
+
 	return diags.Append(n.writeResourceInstanceState(ctx, nil, workingState))
 }