core: Keep old value on error even for delete

builtin/providers/test/provider.go

@@ -36,6 +36,7 @@ func Provider() terraform.ResourceProvider {
 			"test_resource_computed_set":     testResourceComputedSet(),
 			"test_resource_config_mode":      testResourceConfigMode(),
 			"test_resource_nested_id":        testResourceNestedId(),
+			"test_undeleteable":              testResourceUndeleteable(),
 		},
 		DataSourcesMap: map[string]*schema.Resource{
 			"test_data_source":    testDataSource(),

builtin/providers/test/resource_undeletable.go

@@ -0,0 +1,30 @@
+package test
+
+import (
+	"fmt"
+
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func testResourceUndeleteable() *schema.Resource {
+	return &schema.Resource{
+		Create: testResourceUndeleteableCreate,
+		Read:   testResourceUndeleteableRead,
+		Delete: testResourceUndeleteableDelete,
+
+		Schema: map[string]*schema.Schema{},
+	}
+}
+
+func testResourceUndeleteableCreate(d *schema.ResourceData, meta interface{}) error {
+	d.SetId("placeholder")
+	return testResourceUndeleteableRead(d, meta)
+}
+
+func testResourceUndeleteableRead(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}
+
+func testResourceUndeleteableDelete(d *schema.ResourceData, meta interface{}) error {
+	return fmt.Errorf("test_undeleteable always fails deletion (use terraform state rm if you really want to delete it)")
+}

terraform/context_apply_test.go

@@ -7097,6 +7097,80 @@ func TestContext2Apply_error(t *testing.T) {
 	}
 }
 
+func TestContext2Apply_errorDestroy(t *testing.T) {
+	m := testModule(t, "empty")
+	p := testProvider("test")
+
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_thing": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {Type: cty.String, Optional: true},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		// Should actually be called for this test, because Terraform Core
+		// constructs the plan for a destroy operation itself.
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse {
+		// The apply (in this case, a destroy) always fails, so we can verify
+		// that the object stays in the state after a destroy fails even though
+		// we aren't returning a new state object here.
+		return providers.ApplyResourceChangeResponse{
+			Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("failed")),
+		}
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		State: states.BuildState(func(ss *states.SyncState) {
+			ss.SetResourceInstanceCurrent(
+				addrs.Resource{
+					Mode: addrs.ManagedResourceMode,
+					Type: "test_thing",
+					Name: "foo",
+				}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+				&states.ResourceInstanceObjectSrc{
+					Status:    states.ObjectReady,
+					AttrsJSON: []byte(`{"id":"baz"}`),
+				},
+				addrs.ProviderConfig{
+					Type: "test",
+				}.Absolute(addrs.RootModuleInstance),
+			)
+		}),
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	if _, diags := ctx.Plan(); diags.HasErrors() {
+		t.Fatalf("plan errors: %s", diags.Err())
+	}
+
+	state, diags := ctx.Apply()
+	if diags == nil {
+		t.Fatal("should have error")
+	}
+
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(`
+test_thing.foo:
+  ID = baz
+  provider = provider.test
+`) // test_thing.foo is still here, even though provider returned no new state along with its error
+	if actual != expected {
+		t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
+	}
+}
+
 func TestContext2Apply_errorCreateInvalidNew(t *testing.T) {
 	m := testModule(t, "apply-error")
 

terraform/eval_apply.go

@@ -247,11 +247,11 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	// Sometimes providers return a null value when an operation fails for some
-	// reason, but for any action other than delete we'd rather keep the prior
-	// state so that the error can be corrected on a subsequent run. We must
-	// only do this for null new value though, or else we may discard partial
-	// updates the provider was able to complete.
-	if change.Action != plans.Delete && diags.HasErrors() && newVal.IsNull() {
+	// reason, but we'd rather keep the prior state so that the error can be
+	// corrected on a subsequent run. We must only do this for null new value
+	// though, or else we may discard partial updates the provider was able to
+	// complete.
+	if diags.HasErrors() && newVal.IsNull() {
 		// Otherwise, we'll continue but using the prior state as the new value,
 		// making this effectively a no-op. If the item really _has_ been
 		// deleted then our next refresh will detect that and fix it up.