core: Be more explicit in how we handle create_before_destroy

Previously our handling of create_before_destroy -- and of deposed objects
in particular -- was rather "implicit" and spread over various different
subsystems. We'd quietly just destroy every deposed object during a
destroy operation, without any user-visible plan to do so.

Here we make things more explicit by tracking each deposed object
individually by its pseudorandomly-allocated key. There are two different
mechanisms at play here, building on the same concepts:

- During a replace operation with create_before_destroy, we *pre-allocate*
  a DeposedKey to use for the prior object in the "apply" node and then
  pass that exact id to the destroy node, ensuring that we only destroy
  the single object we planned to destroy. In the happy path here the
  user never actually sees the allocated deposed key because we use it and
  then immediately destroy it within the same operation. However, that
  destroy may fail, which brings us to the second mechanism:

- If any deposed objects are already present in state during _plan_, we
  insert a destroy change for them into the plan so that it's explicit to
  the user that we are going to destroy these additional objects, and then
  create an individual graph node for each one in DiffTransformer.

The main motivation here is to be more careful in how we handle these
destroys so that from a user's standpoint we never destroy something
without the user knowing about it ahead of time.

However, this new organization also hopefully makes the code itself a
little easier to follow because the connection between the create and
destroy steps of a Replace is reprseented in a single place (in
DiffTransformer) and deposed instances each have their own explicit graph
node rather than being secretly handled as part of the main instance-level
graph node.

states/module.go

@@ -173,12 +173,12 @@ func (ms *Module) ForgetResourceInstanceDeposed(addr addrs.ResourceInstance, key
 
 // deposeResourceInstanceObject is the real implementation of
 // SyncState.DeposeResourceInstanceObject.
-func (ms *Module) deposeResourceInstanceObject(addr addrs.ResourceInstance) DeposedKey {
+func (ms *Module) deposeResourceInstanceObject(addr addrs.ResourceInstance, forceKey DeposedKey) DeposedKey {
 	is := ms.ResourceInstance(addr)
 	if is == nil {
 		return NotDeposed
 	}
-	return is.deposeCurrentObject()
+	return is.deposeCurrentObject(forceKey)
 }
 
 // SetOutputValue writes an output value into the state, overwriting any

states/resource.go

@@ -104,12 +104,19 @@ func (i *ResourceInstance) HasObjects() bool {
 // SyncState.DeposeResourceInstanceObject. The exported method uses a lock
 // to ensure that we can safely allocate an unused deposed key without
 // collision.
-func (i *ResourceInstance) deposeCurrentObject() DeposedKey {
+func (i *ResourceInstance) deposeCurrentObject(forceKey DeposedKey) DeposedKey {
 	if !i.HasCurrent() {
 		return NotDeposed
 	}
 
-	key := i.findUnusedDeposedKey()
+	key := forceKey
+	if key == NotDeposed {
+		key = i.findUnusedDeposedKey()
+	} else {
+		if _, exists := i.Deposed[key]; exists {
+			panic(fmt.Sprintf("forced key %s is already in use", forceKey))
+		}
+	}
 	i.Deposed[key] = i.Current
 	i.Current = nil
 	return key
@@ -134,6 +141,17 @@ func (i *ResourceInstance) GetGeneration(gen Generation) *ResourceInstanceObject
 	panic(fmt.Sprintf("get invalid Generation %#v", gen))
 }
 
+// FindUnusedDeposedKey generates a unique DeposedKey that is guaranteed not to
+// already be in use for this instance at the time of the call.
+//
+// Note that the validity of this result may change if new deposed keys are
+// allocated before it is used. To avoid this risk, instead use the
+// DeposeResourceInstanceObject method on the SyncState wrapper type, which
+// allocates a key and uses it atomically.
+func (i *ResourceInstance) FindUnusedDeposedKey() DeposedKey {
+	return i.findUnusedDeposedKey()
+}
+
 // findUnusedDeposedKey generates a unique DeposedKey that is guaranteed not to
 // already be in use for this instance.
 func (i *ResourceInstance) findUnusedDeposedKey() DeposedKey {

states/state.go

@@ -61,6 +61,9 @@ func (s *State) Empty() bool {
 // Module returns the state for the module with the given address, or nil if
 // the requested module is not tracked in the state.
 func (s *State) Module(addr addrs.ModuleInstance) *Module {
+	if s == nil {
+		panic("State.Module on nil *State")
+	}
 	return s.Modules[addr.String()]
 }
 
@@ -127,6 +130,9 @@ func (s *State) Resource(addr addrs.AbsResource) *Resource {
 // ResourceInstance returns the state for the resource instance with the given
 // address, or nil if no such resource is tracked in the state.
 func (s *State) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInstance {
+	if s == nil {
+		panic("State.ResourceInstance on nil *State")
+	}
 	ms := s.Module(addr.Module)
 	if ms == nil {
 		return nil

states/sync.go

@@ -323,7 +323,28 @@ func (s *SyncState) DeposeResourceInstanceObject(addr addrs.AbsResourceInstance)
 		return NotDeposed
 	}
 
-	return ms.deposeResourceInstanceObject(addr.Resource)
+	return ms.deposeResourceInstanceObject(addr.Resource, NotDeposed)
+}
+
+// DeposeResourceInstanceObjectForceKey is like DeposeResourceInstanceObject
+// but uses a pre-allocated key. It's the caller's responsibility to ensure
+// that there aren't any races to use a particular key; this method will panic
+// if the given key is already in use.
+func (s *SyncState) DeposeResourceInstanceObjectForceKey(addr addrs.AbsResourceInstance, forcedKey DeposedKey) {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	if forcedKey == NotDeposed {
+		// Usage error: should use DeposeResourceInstanceObject in this case
+		panic("DeposeResourceInstanceObjectForceKey called without forced key")
+	}
+
+	ms := s.state.Module(addr.Module)
+	if ms == nil {
+		return // Nothing to do, since there can't be any current object either.
+	}
+
+	ms.deposeResourceInstanceObject(addr.Resource, forcedKey)
 }
 
 // ForgetResourceInstanceDeposed removes the record of the deposed object with

terraform/context_apply_test.go

@@ -8375,32 +8375,35 @@ func TestContext2Apply_createBefore_depends(t *testing.T) {
 	})
 
 	if p, diags := ctx.Plan(); diags.HasErrors() {
-		t.Fatalf("diags: %s", diags.Err())
+		logDiagnostics(t, diags)
+		t.Fatal("plan failed")
 	} else {
-		t.Logf("plan: %s", legacyDiffComparisonString(p.Changes))
+		t.Logf("plan:\n%s", legacyDiffComparisonString(p.Changes))
 	}
 
 	h.Active = true
 	state, diags := ctx.Apply()
 	if diags.HasErrors() {
-		t.Fatalf("diags: %s", diags.Err())
+		logDiagnostics(t, diags)
+		t.Fatal("apply failed")
 	}
 
 	mod := state.RootModule()
 	if len(mod.Resources) < 2 {
-		t.Fatalf("bad: %#v", mod.Resources)
+		t.Logf("state after apply:\n%s", state.String())
+		t.Fatalf("only %d resources in root module; want at least 2", len(mod.Resources))
 	}
 
-	actual := strings.TrimSpace(state.String())
+	got := strings.TrimSpace(state.String())
-	expected := strings.TrimSpace(testTerraformApplyDependsCreateBeforeStr)
+	want := strings.TrimSpace(testTerraformApplyDependsCreateBeforeStr)
-	if actual != expected {
+	if got != want {
-		t.Fatalf("bad: \n%s\n\n%s", actual, expected)
+		t.Fatalf("wrong final state\ngot:\n%s\n\nwant:\n%s", got, want)
 	}
 
 	// Test that things were managed _in the right order_
 	order := h.States
 	diffs := h.Diffs
-	if order[0].GetAttr("id").AsString() != "" || diffs[0].Action == plans.Delete {
+	if !order[0].IsNull() || diffs[0].Action == plans.Delete {
 		t.Fatalf("should create new instance first: %#v", order)
 	}
 

terraform/eval_apply.go

@@ -224,6 +224,10 @@ func (n *EvalApplyPre) Eval(ctx EvalContext) (interface{}, error) {
 	change := *n.Change
 	absAddr := n.Addr.Absolute(ctx.Path())
 
+	if change == nil {
+		panic(fmt.Sprintf("EvalApplyPre for %s called with nil Change", absAddr))
+	}
+
 	if resourceHasUserVisibleApply(n.Addr) {
 		priorState := change.Before
 		plannedNewState := change.After

terraform/eval_state.go

@@ -310,6 +310,12 @@ func (n *EvalWriteStateDeposed) Eval(ctx EvalContext) (interface{}, error) {
 type EvalDeposeState struct {
 	Addr addrs.ResourceInstance
 
+	// ForceKey, if a value other than states.NotDeposed, will be used as the
+	// key for the newly-created deposed object that results from this action.
+	// If set to states.NotDeposed (the zero value), a new unique key will be
+	// allocated.
+	ForceKey states.DeposedKey
+
 	// OutputKey, if non-nil, will be written with the deposed object key that
 	// was generated for the object. This can then be passed to
 	// EvalUndeposeState.Key so it knows which deposed instance to forget.
@@ -321,7 +327,13 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) {
 	absAddr := n.Addr.Absolute(ctx.Path())
 	state := ctx.State()
 
-	key := state.DeposeResourceInstanceObject(absAddr)
+	var key states.DeposedKey
+	if n.ForceKey == states.NotDeposed {
+		key = state.DeposeResourceInstanceObject(absAddr)
+	} else {
+		key = n.ForceKey
+		state.DeposeResourceInstanceObjectForceKey(absAddr, key)
+	}
 	log.Printf("[TRACE] EvalDeposeState: prior object for %s now deposed with key %s", absAddr, key)
 
 	if n.OutputKey != nil {

terraform/graph_builder_apply.go

@@ -95,6 +95,8 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		// ConfigTransformer above.
 		&DiffTransformer{
 			Concrete: concreteResourceInstance,
+			Config:   b.Config,
+			State:    b.State,
 			Changes:  b.Changes,
 		},
 

terraform/graph_builder_apply_test.go

@@ -1,12 +1,13 @@
 package terraform
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
-	"github.com/hashicorp/terraform/plans"
-
 	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/plans"
+	"github.com/hashicorp/terraform/states"
 )
 
 func TestApplyGraphBuilder_impl(t *testing.T) {
@@ -105,20 +106,42 @@ func TestApplyGraphBuilder_depCbd(t *testing.T) {
 		t.Fatalf("wrong path %q", g.Path.String())
 	}
 
+	// We're going to go hunting for our deposed instance node here, so we
+	// can find out its key to use in the assertions below.
+	var dk states.DeposedKey
+	for _, v := range g.Vertices() {
+		tv, ok := v.(*NodeDestroyDeposedResourceInstanceObject)
+		if !ok {
+			continue
+		}
+		if dk != states.NotDeposed {
+			t.Fatalf("more than one deposed instance node in the graph; want only one")
+		}
+		dk = tv.DeposedKey
+	}
+	if dk == states.NotDeposed {
+		t.Fatalf("no deposed instance node in the graph; want one")
+	}
+
+	destroyName := fmt.Sprintf("test_object.A (deposed %s)", dk)
+
 	// Create A, Modify B, Destroy A
 
 	testGraphHappensBefore(
 		t, g,
 		"test_object.A",
-		"test_object.A (destroy)")
+		destroyName,
+	)
 	testGraphHappensBefore(
 		t, g,
 		"test_object.A",
-		"test_object.B")
+		"test_object.B",
+	)
 	testGraphHappensBefore(
 		t, g,
 		"test_object.B",
-		"test_object.A (destroy)")
+		destroyName,
+	)
 }
 
 // This tests the ordering of two resources that are both CBD that

terraform/graph_builder_destroy_plan.go

@@ -51,6 +51,12 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer {
 			NodeAbstractResourceInstance: a,
 		}
 	}
+	concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex {
+		return &NodePlanDeposedResourceInstanceObject{
+			NodeAbstractResourceInstance: a,
+			DeposedKey: key,
+		}
+	}
 
 	concreteProvider := func(a *NodeAbstractProvider) dag.Vertex {
 		return &NodeApplyableProvider{
@@ -61,7 +67,8 @@ func (b *DestroyPlanGraphBuilder) Steps() []GraphTransformer {
 	steps := []GraphTransformer{
 		// Creates nodes for the resource instances tracked in the state.
 		&StateTransformer{
-			Concrete: concreteResourceInstance,
+			ConcreteCurrent: concreteResourceInstance,
+			ConcreteDeposed: concreteResourceInstanceDeposed,
 			State:           b.State,
 		},
 

terraform/graph_builder_plan.go

@@ -69,6 +69,13 @@ func (b *PlanGraphBuilder) Build(path addrs.ModuleInstance) (*Graph, tfdiags.Dia
 func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 	b.once.Do(b.init)
 
+	concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex {
+		return &NodePlanDeposedResourceInstanceObject{
+			NodeAbstractResourceInstance: a,
+			DeposedKey: key,
+		}
+	}
+
 	steps := []GraphTransformer{
 		// Creates all the resources represented in the config
 		&ConfigTransformer{
@@ -89,6 +96,15 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 			Config:   b.Config,
 		},
 
+		// We also need nodes for any deposed instance objects present in the
+		// state, so we can plan to destroy them. (This intentionally
+		// skips creating nodes for _current_ objects, since ConfigTransformer
+		// created nodes that will do that during DynamicExpand.)
+		&StateTransformer{
+			ConcreteDeposed: concreteResourceInstanceDeposed,
+			State:           b.State,
+		},
+
 		// Create orphan output nodes
 		&OrphanOutputTransformer{
 			Config: b.Config,

terraform/graph_builder_refresh.go

@@ -78,6 +78,14 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer {
 		}
 	}
 
+	concreteResourceInstanceDeposed := func(a *NodeAbstractResourceInstance, key states.DeposedKey) dag.Vertex {
+		// The "Plan" node type also handles refreshing behavior.
+		return &NodePlanDeposedResourceInstanceObject{
+			NodeAbstractResourceInstance: a,
+			DeposedKey: key,
+		}
+	}
+
 	concreteDataResource := func(a *NodeAbstractResource) dag.Vertex {
 		return &NodeRefreshableDataResource{
 			NodeAbstractResource: a,
@@ -121,6 +129,15 @@ func (b *RefreshGraphBuilder) Steps() []GraphTransformer {
 			Config:   b.Config,
 		},
 
+		// We also need nodes for any deposed instance objects present in the
+		// state, so we can check if they still exist. (This intentionally
+		// skips creating nodes for _current_ objects, since ConfigTransformer
+		// created nodes that will do that during DynamicExpand.)
+		&StateTransformer{
+			ConcreteDeposed: concreteResourceInstanceDeposed,
+			State:           b.State,
+		},
+
 		// Attach the state
 		&AttachStateTransformer{State: b.State},
 

terraform/node_resource_apply_instance.go

@@ -23,6 +23,7 @@ type NodeApplyableResourceInstance struct {
 	*NodeAbstractResourceInstance
 
 	destroyNode GraphNodeDestroyerCBD
+	graphNodeDeposer // implementation of GraphNodeDeposer
 }
 
 var (
@@ -30,6 +31,7 @@ var (
 	_ GraphNodeResourceInstance = (*NodeApplyableResourceInstance)(nil)
 	_ GraphNodeCreator          = (*NodeApplyableResourceInstance)(nil)
 	_ GraphNodeReferencer       = (*NodeApplyableResourceInstance)(nil)
+	_ GraphNodeDeposer          = (*NodeApplyableResourceInstance)(nil)
 	_ GraphNodeEvalable         = (*NodeApplyableResourceInstance)(nil)
 )
 
@@ -248,6 +250,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				},
 				Then: &EvalDeposeState{
 					Addr:      addr.Resource,
+					ForceKey:  n.PreallocatedDeposedKey,
 					OutputKey: &deposedKey,
 				},
 			},

terraform/node_resource_destroy.go

@@ -37,6 +37,9 @@ var (
 )
 
 func (n *NodeDestroyResourceInstance) Name() string {
+	if n.DeposedKey != states.NotDeposed {
+		return fmt.Sprintf("%s (destroy deposed %s)", n.ResourceInstanceAddr(), n.DeposedKey)
+	}
 	return n.ResourceInstanceAddr().String() + " (destroy)"
 }
 
@@ -115,44 +118,6 @@ func (n *NodeDestroyResourceInstance) References() []*addrs.Reference {
 	return nil
 }
 
-// GraphNodeDynamicExpandable
-func (n *NodeDestroyResourceInstance) DynamicExpand(ctx EvalContext) (*Graph, error) {
-	if n.DeposedKey != states.NotDeposed {
-		return nil, fmt.Errorf("NodeDestroyResourceInstance not yet updated to deal with explicit DeposedKey")
-	}
-
-	// Our graph transformers require direct access to read the entire state
-	// structure, so we'll lock the whole state for the duration of this work.
-	state := ctx.State().Lock()
-	defer ctx.State().Unlock()
-
-	// Start creating the steps
-	steps := make([]GraphTransformer, 0, 5)
-
-	// We want deposed resources in the state to be destroyed
-	steps = append(steps, &DeposedTransformer{
-		State:            state,
-		InstanceAddr:     n.ResourceInstanceAddr(),
-		ResolvedProvider: n.ResolvedProvider,
-	})
-
-	// Target
-	steps = append(steps, &TargetsTransformer{
-		Targets: n.Targets,
-	})
-
-	// Always end with the root being added
-	steps = append(steps, &RootTransformer{})
-
-	// Build the graph
-	b := &BasicGraphBuilder{
-		Steps: steps,
-		Name:  "NodeResourceDestroy",
-	}
-	g, diags := b.Build(ctx.Path())
-	return g, diags.ErrWithWarnings()
-}
-
 // GraphNodeEvalable
 func (n *NodeDestroyResourceInstance) EvalTree() EvalNode {
 	addr := n.ResourceInstanceAddr()

terraform/node_resource_destroy_deposed.go

@@ -0,0 +1,297 @@
+package terraform
+
+import (
+	"fmt"
+
+	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/dag"
+	"github.com/hashicorp/terraform/plans"
+	"github.com/hashicorp/terraform/providers"
+	"github.com/hashicorp/terraform/states"
+)
+
+// ConcreteResourceInstanceDeposedNodeFunc is a callback type used to convert
+// an abstract resource instance to a concrete one of some type that has
+// an associated deposed object key.
+type ConcreteResourceInstanceDeposedNodeFunc func(*NodeAbstractResourceInstance, states.DeposedKey) dag.Vertex
+
+// NodePlanDeposedResourceInstanceObject represents deposed resource
+// instance objects during plan. These are distinct from the primary object
+// for each resource instance since the only valid operation to do with them
+// is to destroy them.
+//
+// This node type is also used during the refresh walk to ensure that the
+// record of a deposed object is up-to-date before we plan to destroy it.
+type NodePlanDeposedResourceInstanceObject struct {
+	*NodeAbstractResourceInstance
+	DeposedKey states.DeposedKey
+}
+
+var (
+	_ GraphNodeResource            = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeResourceInstance    = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeReferenceable       = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeReferencer          = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeEvalable            = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeProviderConsumer    = (*NodePlanDeposedResourceInstanceObject)(nil)
+	_ GraphNodeProvisionerConsumer = (*NodePlanDeposedResourceInstanceObject)(nil)
+)
+
+func (n *NodePlanDeposedResourceInstanceObject) Name() string {
+	return fmt.Sprintf("%s (deposed %s)", n.Addr.String(), n.DeposedKey)
+}
+
+// GraphNodeReferenceable implementation, overriding the one from NodeAbstractResourceInstance
+func (n *NodePlanDeposedResourceInstanceObject) ReferenceableAddrs() []addrs.Referenceable {
+	// Deposed objects don't participate in references.
+	return nil
+}
+
+// GraphNodeReferencer implementation, overriding the one from NodeAbstractResourceInstance
+func (n *NodePlanDeposedResourceInstanceObject) References() []*addrs.Reference {
+	// We don't evaluate configuration for deposed objects, so they effectively
+	// make no references.
+	return nil
+}
+
+// GraphNodeEvalable impl.
+func (n *NodePlanDeposedResourceInstanceObject) EvalTree() EvalNode {
+	addr := n.ResourceInstanceAddr()
+
+	var provider providers.Interface
+	var providerSchema *ProviderSchema
+	var state *states.ResourceInstanceObject
+
+	seq := &EvalSequence{Nodes: make([]EvalNode, 0, 5)}
+
+	// During the refresh walk we will ensure that our record of the deposed
+	// object is up-to-date. If it was already deleted outside of Terraform
+	// then this will remove it from state and thus avoid us planning a
+	// destroy for it during the subsequent plan walk.
+	seq.Nodes = append(seq.Nodes, &EvalOpFilter{
+		Ops: []walkOperation{walkRefresh},
+		Node: &EvalSequence{
+			Nodes: []EvalNode{
+				&EvalGetProvider{
+					Addr:   n.ResolvedProvider,
+					Output: &provider,
+					Schema: &providerSchema,
+				},
+				&EvalReadStateDeposed{
+					Addr:           addr.Resource,
+					ProviderSchema: &providerSchema,
+					Key:            n.DeposedKey,
+					Output:         &state,
+				},
+				&EvalRefresh{
+					Addr:           addr.Resource,
+					ProviderAddr:   n.ResolvedProvider,
+					Provider:       &provider,
+					ProviderSchema: &providerSchema,
+					State:          &state,
+					Output:         &state,
+				},
+				&EvalWriteStateDeposed{
+					Addr:           addr.Resource,
+					Key:            n.DeposedKey,
+					ProviderAddr:   n.ResolvedProvider,
+					ProviderSchema: &providerSchema,
+					State:          &state,
+				},
+			},
+		},
+	})
+
+	// During the plan walk we always produce a planned destroy change, because
+	// destroying is the only supported action for deposed objects.
+	var change *plans.ResourceInstanceChange
+	seq.Nodes = append(seq.Nodes, &EvalOpFilter{
+		Ops: []walkOperation{walkPlan, walkPlanDestroy},
+		Node: &EvalSequence{
+			Nodes: []EvalNode{
+				&EvalGetProvider{
+					Addr:   n.ResolvedProvider,
+					Output: &provider,
+					Schema: &providerSchema,
+				},
+				&EvalReadStateDeposed{
+					Addr:           addr.Resource,
+					Output:         &state,
+					Key:            n.DeposedKey,
+					Provider:       &provider,
+					ProviderSchema: &providerSchema,
+				},
+				&EvalDiffDestroy{
+					Addr:       addr.Resource,
+					DeposedKey: n.DeposedKey,
+					State:      &state,
+					Output:     &change,
+				},
+				&EvalWriteDiff{
+					Addr:           addr.Resource,
+					DeposedKey:     n.DeposedKey,
+					ProviderSchema: &providerSchema,
+					Change:         &change,
+				},
+				// Since deposed objects cannot be referenced by expressions
+				// elsewhere, we don't need to also record the planned new
+				// state in this case.
+			},
+		},
+	})
+
+	return seq
+}
+
+
+// NodeDestroyDeposedResourceInstanceObject represents deposed resource
+// instance objects during apply. Nodes of this type are inserted by
+// DiffTransformer when the planned changeset contains "delete" changes for
+// deposed instance objects, and its only supported operation is to destroy
+// and then forget the associated object.
+type NodeDestroyDeposedResourceInstanceObject struct {
+	*NodeAbstractResourceInstance
+	DeposedKey states.DeposedKey
+}
+
+var (
+	_ GraphNodeResource            = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeResourceInstance    = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeDestroyer           = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeDestroyerCBD        = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeReferenceable       = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeReferencer          = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeEvalable            = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeProviderConsumer    = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+	_ GraphNodeProvisionerConsumer = (*NodeDestroyDeposedResourceInstanceObject)(nil)
+)
+
+func (n *NodeDestroyDeposedResourceInstanceObject) Name() string {
+	return fmt.Sprintf("%s (destroy deposed %s)", n.Addr.String(), n.DeposedKey)
+}
+
+// GraphNodeReferenceable implementation, overriding the one from NodeAbstractResourceInstance
+func (n *NodeDestroyDeposedResourceInstanceObject) ReferenceableAddrs() []addrs.Referenceable {
+	// Deposed objects don't participate in references.
+	return nil
+}
+
+// GraphNodeReferencer implementation, overriding the one from NodeAbstractResourceInstance
+func (n *NodeDestroyDeposedResourceInstanceObject) References() []*addrs.Reference {
+	// We don't evaluate configuration for deposed objects, so they effectively
+	// make no references.
+	return nil
+}
+
+// GraphNodeDestroyer
+func (n *NodeDestroyDeposedResourceInstanceObject) DestroyAddr() *addrs.AbsResourceInstance {
+	addr := n.ResourceInstanceAddr()
+	return &addr
+}
+
+// GraphNodeDestroyerCBD
+func (n *NodeDestroyDeposedResourceInstanceObject) CreateBeforeDestroy() bool {
+	// A deposed instance is always CreateBeforeDestroy by definition, since
+	// we use deposed only to handle create-before-destroy.
+	return true
+}
+
+// GraphNodeDestroyerCBD
+func (n *NodeDestroyDeposedResourceInstanceObject) ModifyCreateBeforeDestroy(v bool) error {
+	if !v {
+		// Should never happen: deposed instances are _always_ create_before_destroy.
+		return fmt.Errorf("can't deactivate create_before_destroy for a deposed instance")
+	}
+	return nil
+}
+
+// GraphNodeEvalable impl.
+func (n *NodeDestroyDeposedResourceInstanceObject) EvalTree() EvalNode {
+	addr := n.ResourceInstanceAddr()
+
+	var provider providers.Interface
+	var providerSchema *ProviderSchema
+	var state *states.ResourceInstanceObject
+	var change *plans.ResourceInstanceChange
+	var err error
+
+	return &EvalSequence{
+		Nodes: []EvalNode{
+			&EvalGetProvider{
+				Addr:   n.ResolvedProvider,
+				Output: &provider,
+				Schema: &providerSchema,
+			},
+			&EvalReadStateDeposed{
+				Addr:           addr.Resource,
+				Output:         &state,
+				Key:            n.DeposedKey,
+				Provider:       &provider,
+				ProviderSchema: &providerSchema,
+			},
+			&EvalDiffDestroy{
+				Addr:   addr.Resource,
+				State:  &state,
+				Output: &change,
+			},
+			// Call pre-apply hook
+			&EvalApplyPre{
+				Addr:   addr.Resource,
+				State:  &state,
+				Change: &change,
+			},
+			&EvalApply{
+				Addr:           addr.Resource,
+				Config:         nil, // No configuration because we are destroying
+				State:          &state,
+				Change:         &change,
+				Provider:       &provider,
+				ProviderAddr:   n.ResolvedProvider,
+				ProviderSchema: &providerSchema,
+				Output:         &state,
+				Error:          &err,
+			},
+			// Always write the resource back to the state deposed... if it
+			// was successfully destroyed it will be pruned. If it was not, it will
+			// be caught on the next run.
+			&EvalWriteStateDeposed{
+				Addr:           addr.Resource,
+				Key:            n.DeposedKey,
+				ProviderAddr:   n.ResolvedProvider,
+				ProviderSchema: &providerSchema,
+				State:          &state,
+			},
+			&EvalApplyPost{
+				Addr:  addr.Resource,
+				State: &state,
+				Error: &err,
+			},
+			&EvalReturnError{
+				Error: &err,
+			},
+			&EvalUpdateStateHook{},
+		},
+	}
+}
+
+// GraphNodeDeposer is an optional interface implemented by graph nodes that
+// might create a single new deposed object for a specific associated resource
+// instance, allowing a caller to optionally pre-allocate a DeposedKey for
+// it.
+type GraphNodeDeposer interface {
+	// SetPreallocatedDeposedKey will be called during graph construction
+	// if a particular node must use a pre-allocated deposed key if/when it
+	// "deposes" the current object of its associated resource instance.
+	SetPreallocatedDeposedKey(key states.DeposedKey)
+}
+
+// graphNodeDeposer is an embeddable implementation of GraphNodeDeposer.
+// Embed it in a node type to get automatic support for it, and then access
+// the field PreallocatedDeposedKey to access any pre-allocated key.
+type graphNodeDeposer struct {
+	PreallocatedDeposedKey states.DeposedKey
+}
+
+func (n *graphNodeDeposer) SetPreallocatedDeposedKey(key states.DeposedKey) {
+	n.PreallocatedDeposedKey = key
+}

terraform/node_resource_destroy_test.go

@@ -1,68 +0,0 @@
-package terraform
-
-import (
-	"strings"
-	"testing"
-
-	"github.com/hashicorp/terraform/addrs"
-)
-
-func TestNodeDestroyResourceDynamicExpand_deposedCount(t *testing.T) {
-	state := mustShimLegacyState(&State{
-		Modules: []*ModuleState{
-			&ModuleState{
-				Path: rootModulePath,
-				Resources: map[string]*ResourceState{
-					"aws_instance.bar.0": &ResourceState{
-						Type: "aws_instance",
-						Deposed: []*InstanceState{
-							&InstanceState{
-								ID: "foo",
-							},
-						},
-						Provider: "provider.aws",
-					},
-					"aws_instance.bar.1": &ResourceState{
-						Type: "aws_instance",
-						Deposed: []*InstanceState{
-							&InstanceState{
-								ID: "bar",
-							},
-						},
-						Provider: "provider.aws",
-					},
-				},
-			},
-		},
-	})
-
-	m := testModule(t, "apply-cbd-count")
-	n := &NodeDestroyResourceInstance{
-		NodeAbstractResourceInstance: &NodeAbstractResourceInstance{
-			NodeAbstractResource: NodeAbstractResource{
-				Addr: addrs.RootModuleInstance.Resource(
-					addrs.ManagedResourceMode, "aws_instance", "bar",
-				),
-				Config: m.Module.ManagedResources["aws_instance.bar"],
-			},
-			InstanceKey:   addrs.IntKey(0),
-			ResourceState: state.Modules[""].Resources["aws_instance.bar[0]"],
-		},
-	}
-
-	g, err := n.DynamicExpand(&MockEvalContext{
-		PathPath:   addrs.RootModuleInstance,
-		StateState: state.SyncWrapper(),
-	})
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-
-	got := strings.TrimSpace(g.String())
-	want := strings.TrimSpace(`
-aws_instance.bar[0] (deposed 00000001)
-`)
-	if got != want {
-		t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", got, want)
-	}
-}

terraform/test-fixtures/apply-depends-create-before/main.tf

@@ -6,5 +6,5 @@ resource "aws_instance" "web" {
 }
 
 resource "aws_instance" "lb" {
-    instance = "${aws_instance.web.id}"
+  instance = aws_instance.web.id
 }

terraform/transform_deposed.go

@@ -1,190 +0,0 @@
-package terraform
-
-import (
-	"fmt"
-
-	"github.com/hashicorp/terraform/addrs"
-	"github.com/hashicorp/terraform/plans"
-	"github.com/hashicorp/terraform/providers"
-	"github.com/hashicorp/terraform/states"
-)
-
-// DeposedTransformer is a GraphTransformer that adds nodes to the graph for
-// the deposed objects associated with a given resource instance.
-type DeposedTransformer struct {
-	// State is the global state, from which we'll retrieve the state for
-	// the instance given in InstanceAddr.
-	State *states.State
-
-	// InstanceAddr is the address of the instance whose deposed objects will
-	// have graph nodes created.
-	InstanceAddr addrs.AbsResourceInstance
-
-	// The provider used by the resourced which were deposed
-	ResolvedProvider addrs.AbsProviderConfig
-}
-
-func (t *DeposedTransformer) Transform(g *Graph) error {
-	rs := t.State.Resource(t.InstanceAddr.ContainingResource())
-	if rs == nil {
-		// If the resource has no state then there can't be deposed objects.
-		return nil
-	}
-	is := rs.Instances[t.InstanceAddr.Resource.Key]
-	if is == nil {
-		// If the instance has no state then there can't be deposed objects.
-		return nil
-	}
-
-	providerAddr := rs.ProviderConfig
-
-	for k := range is.Deposed {
-		g.Add(&graphNodeDeposedResource{
-			Addr:             t.InstanceAddr,
-			DeposedKey:       k,
-			RecordedProvider: providerAddr,
-			ResolvedProvider: t.ResolvedProvider,
-		})
-	}
-
-	return nil
-}
-
-// graphNodeDeposedResource is the graph vertex representing a deposed resource.
-type graphNodeDeposedResource struct {
-	Addr             addrs.AbsResourceInstance
-	DeposedKey       states.DeposedKey
-	RecordedProvider addrs.AbsProviderConfig
-	ResolvedProvider addrs.AbsProviderConfig
-}
-
-var (
-	_ GraphNodeProviderConsumer = (*graphNodeDeposedResource)(nil)
-	_ GraphNodeEvalable         = (*graphNodeDeposedResource)(nil)
-)
-
-func (n *graphNodeDeposedResource) Name() string {
-	return fmt.Sprintf("%s (deposed %s)", n.Addr.String(), n.DeposedKey)
-}
-
-func (n *graphNodeDeposedResource) ProvidedBy() (addrs.AbsProviderConfig, bool) {
-	return n.RecordedProvider, true
-}
-
-func (n *graphNodeDeposedResource) SetProvider(addr addrs.AbsProviderConfig) {
-	// Because our ProvidedBy returns exact=true, this is actually rather
-	// pointless and should always just be the address we asked for.
-	n.RecordedProvider = addr
-}
-
-// GraphNodeEvalable impl.
-func (n *graphNodeDeposedResource) EvalTree() EvalNode {
-	addr := n.Addr
-
-	var provider providers.Interface
-	var providerSchema *ProviderSchema
-	var state *states.ResourceInstanceObject
-
-	seq := &EvalSequence{Nodes: make([]EvalNode, 0, 5)}
-
-	// Refresh the resource
-	seq.Nodes = append(seq.Nodes, &EvalOpFilter{
-		Ops: []walkOperation{walkRefresh},
-		Node: &EvalSequence{
-			Nodes: []EvalNode{
-				&EvalGetProvider{
-					Addr:   n.ResolvedProvider,
-					Output: &provider,
-					Schema: &providerSchema,
-				},
-				&EvalReadStateDeposed{
-					Addr:           addr.Resource,
-					ProviderSchema: &providerSchema,
-					Key:            n.DeposedKey,
-					Output:         &state,
-				},
-				&EvalRefresh{
-					Addr:           addr.Resource,
-					ProviderAddr:   n.ResolvedProvider,
-					Provider:       &provider,
-					ProviderSchema: &providerSchema,
-					State:          &state,
-					Output:         &state,
-				},
-				&EvalWriteStateDeposed{
-					Addr:           addr.Resource,
-					Key:            n.DeposedKey,
-					ProviderAddr:   n.ResolvedProvider,
-					ProviderSchema: &providerSchema,
-					State:          &state,
-				},
-			},
-		},
-	})
-
-	// Apply
-	var change *plans.ResourceInstanceChange
-	var err error
-	seq.Nodes = append(seq.Nodes, &EvalOpFilter{
-		Ops: []walkOperation{walkApply, walkDestroy},
-		Node: &EvalSequence{
-			Nodes: []EvalNode{
-				&EvalGetProvider{
-					Addr:   n.ResolvedProvider,
-					Output: &provider,
-					Schema: &providerSchema,
-				},
-				&EvalReadStateDeposed{
-					Addr:           addr.Resource,
-					Output:         &state,
-					Key:            n.DeposedKey,
-					Provider:       &provider,
-					ProviderSchema: &providerSchema,
-				},
-				&EvalDiffDestroy{
-					Addr:   addr.Resource,
-					State:  &state,
-					Output: &change,
-				},
-				// Call pre-apply hook
-				&EvalApplyPre{
-					Addr:   addr.Resource,
-					State:  &state,
-					Change: &change,
-				},
-				&EvalApply{
-					Addr:           addr.Resource,
-					Config:         nil, // No configuration because we are destroying
-					State:          &state,
-					Change:         &change,
-					Provider:       &provider,
-					ProviderAddr:   n.ResolvedProvider,
-					ProviderSchema: &providerSchema,
-					Output:         &state,
-					Error:          &err,
-				},
-				// Always write the resource back to the state deposed... if it
-				// was successfully destroyed it will be pruned. If it was not, it will
-				// be caught on the next run.
-				&EvalWriteStateDeposed{
-					Addr:           addr.Resource,
-					Key:            n.DeposedKey,
-					ProviderAddr:   n.ResolvedProvider,
-					ProviderSchema: &providerSchema,
-					State:          &state,
-				},
-				&EvalApplyPost{
-					Addr:  addr.Resource,
-					State: &state,
-					Error: &err,
-				},
-				&EvalReturnError{
-					Error: &err,
-				},
-				&EvalUpdateStateHook{},
-			},
-		},
-	})
-
-	return seq
-}

terraform/transform_diff.go

@@ -4,15 +4,19 @@ import (
 	"fmt"
 	"log"
 
+	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/dag"
 	"github.com/hashicorp/terraform/plans"
 	"github.com/hashicorp/terraform/states"
+	"github.com/hashicorp/terraform/tfdiags"
 )
 
 // DiffTransformer is a GraphTransformer that adds graph nodes representing
 // each of the resource changes described in the given Changes object.
 type DiffTransformer struct {
 	Concrete ConcreteResourceInstanceNodeFunc
+	Config   *configs.Config
+	State    *states.State
 	Changes  *plans.Changes
 }
 
@@ -25,6 +29,11 @@ func (t *DiffTransformer) Transform(g *Graph) error {
 	// Go through all the modules in the diff.
 	log.Printf("[TRACE] DiffTransformer starting")
 
+	var diags tfdiags.Diagnostics
+	config := t.Config
+	state := t.State
+	changes := t.Changes
+
 	// DiffTransformer creates resource _instance_ nodes. If there are any
 	// whole-resource nodes already in the graph, we must ensure that they
 	// get evaluated before any of the corresponding instances by creating
@@ -47,14 +56,22 @@ func (t *DiffTransformer) Transform(g *Graph) error {
 		resourceNodes[addr] = append(resourceNodes[addr], rn)
 	}
 
-	for _, rc := range t.Changes.Resources {
+	for _, rc := range changes.Resources {
 		addr := rc.Addr
 		dk := rc.DeposedKey
+		var rCfg *configs.Resource
+
+		log.Printf("[TRACE] DiffTransformer: found %s change for %s %s", rc.Action, addr, dk)
+
+		modCfg := config.DescendentForInstance(addr.Module)
+		if modCfg != nil {
+			rCfg = modCfg.Module.ResourceByAddr(addr.Resource.Resource)
+		}
 
 		// Depending on the action we'll need some different combinations of
 		// nodes, because destroying uses a special node type separate from
 		// other actions.
-		var update, delete bool
+		var update, delete, createBeforeDestroy bool
 		switch rc.Action {
 		case plans.NoOp:
 			continue
@@ -67,6 +84,56 @@ func (t *DiffTransformer) Transform(g *Graph) error {
 			update = true
 		}
 
+		if dk != states.NotDeposed && update {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				"Invalid planned change for deposed object",
+				fmt.Sprintf("The plan contains a non-delete change for %s deposed object %s. The only valid action for a deposed object is to destroy it, so this is a bug in Terraform.", addr, dk),
+			))
+			continue
+		}
+
+		if rCfg != nil && rCfg.Managed != nil && rCfg.Managed.CreateBeforeDestroy {
+			createBeforeDestroy = true
+		}
+
+		// If we're going to do a create_before_destroy Replace operation then
+		// we need to allocate a DeposedKey to use to retain the
+		// not-yet-destroyed prior object, so that the delete node can destroy
+		// _that_ rather than the newly-created node, which will be current
+		// by the time the delete node is visited.
+		if update && delete && createBeforeDestroy {
+			// In this case, variable dk will be the _pre-assigned_ DeposedKey
+			// that must be used if the update graph node deposes the current
+			// instance, which will then align with the same key we pass
+			// into the destroy node to ensure we destroy exactly the deposed
+			// object we expect.
+			if state != nil {
+				ris := state.ResourceInstance(addr)
+				if ris == nil {
+					// Should never happen, since we don't plan to replace an
+					// instance that doesn't exist yet.
+					diags = diags.Append(tfdiags.Sourceless(
+						tfdiags.Error,
+						"Invalid planned change",
+						fmt.Sprintf("The plan contains a replace change for %s, which doesn't exist yet. This is a bug in Terraform.", addr),
+					))
+					continue
+				}
+
+				// Allocating a deposed key separately from using it can be racy
+				// in general, but we assume here that nothing except the apply
+				// node we instantiate below will actually make new deposed objects
+				// in practice, and so the set of already-used keys will not change
+				// between now and then.
+				dk = ris.FindUnusedDeposedKey()
+			} else {
+				// If we have no state at all yet then we can use _any_
+				// DeposedKey.
+				dk = states.NewDeposedKey()
+			}
+		}
+
 		if update {
 			// All actions except destroying the node type chosen by t.Concrete
 			abstract := NewNodeAbstractResourceInstance(addr)
@@ -75,14 +142,19 @@ func (t *DiffTransformer) Transform(g *Graph) error {
 				node = f(abstract)
 			}
 
-			if dk != states.NotDeposed {
+			if createBeforeDestroy {
-				// The only valid action for deposed objects is to destroy them.
+				// We'll attach our pre-allocated DeposedKey to the node if
-				// Entering this branch suggests a bug in the plan phase that
+				// it supports that. NodeApplyableResourceInstance is the
-				// proposed this change.
+				// specific concrete node type we are looking for here really,
-				return fmt.Errorf("invalid %s action for deposed object on %s: only Delete is allowed", rc.Action, addr)
+				// since that's the only node type that might depose objects.
+				if dn, ok := node.(GraphNodeDeposer); ok {
+					dn.SetPreallocatedDeposedKey(dk)
+				}
+				log.Printf("[TRACE] DiffTransformer: %s will be represented by %s, deposing prior object to %s", addr, dag.VertexName(node), dk)
+			} else {
+				log.Printf("[TRACE] DiffTransformer: %s will be represented by %s", addr, dag.VertexName(node))
 			}
 
-			log.Printf("[TRACE] DiffTransformer: %s will be represented by %s", addr, dag.VertexName(node))
 			g.Add(node)
 			rsrcAddr := addr.ContainingResource().String()
 			for _, rsrcNode := range resourceNodes[rsrcAddr] {
@@ -91,12 +163,22 @@ func (t *DiffTransformer) Transform(g *Graph) error {
 		}
 
 		if delete {
-			// Destroying always uses this destroy-specific node type.
+			// Destroying always uses a destroy-specific node type, though
+			// which one depends on whether we're destroying a current object
+			// or a deposed object.
+			var node GraphNodeResourceInstance
 			abstract := NewNodeAbstractResourceInstance(addr)
-			node := &NodeDestroyResourceInstance{
+			if dk == states.NotDeposed {
+				node = &NodeDestroyResourceInstance{
 					NodeAbstractResourceInstance: abstract,
 					DeposedKey:                   dk,
 				}
+			} else {
+				node = &NodeDestroyDeposedResourceInstanceObject{
+					NodeAbstractResourceInstance: abstract,
+					DeposedKey:                   dk,
+				}
+			}
 			if dk == states.NotDeposed {
 				log.Printf("[TRACE] DiffTransformer: %s will be represented for destruction by %s", addr, dag.VertexName(node))
 			} else {
@@ -117,5 +199,5 @@ func (t *DiffTransformer) Transform(g *Graph) error {
 
 	log.Printf("[TRACE] DiffTransformer complete")
 
-	return nil
+	return diags.Err()
 }

terraform/transform_provisioner_test.go

@@ -93,7 +93,7 @@ func TestMissingProvisionerTransformer_module(t *testing.T) {
 		})
 
 		tf := &StateTransformer{
-			Concrete: concreteResource,
+			ConcreteCurrent: concreteResource,
 			State:           state,
 		}
 		if err := tf.Transform(&g); err != nil {

terraform/transform_state.go

@@ -3,7 +3,6 @@ package terraform
 import (
 	"log"
 
-	"github.com/hashicorp/terraform/dag"
 	"github.com/hashicorp/terraform/states"
 )
 
@@ -13,7 +12,15 @@ import (
 // This transform is used for example by the DestroyPlanGraphBuilder to ensure
 // that only resources that are in the state are represented in the graph.
 type StateTransformer struct {
-	Concrete ConcreteResourceInstanceNodeFunc
+	// ConcreteCurrent and ConcreteDeposed are used to specialize the abstract
+	// resource instance nodes that this transformer will create.
+	//
+	// If either of these is nil, the objects of that type will be skipped and
+	// not added to the graph at all. It doesn't make sense to use this
+	// transformer without setting at least one of these, since that would
+	// skip everything and thus be a no-op.
+	ConcreteCurrent ConcreteResourceInstanceNodeFunc
+	ConcreteDeposed ConcreteResourceInstanceDeposedNodeFunc
 
 	State *states.State
 }
@@ -24,24 +31,41 @@ func (t *StateTransformer) Transform(g *Graph) error {
 		return nil
 	}
 
-	log.Printf("[TRACE] StateTransformer: starting")
+	switch {
+	case t.ConcreteCurrent != nil && t.ConcreteDeposed != nil:
+		log.Printf("[TRACE] StateTransformer: creating nodes for both current and deposed instance objects")
+	case t.ConcreteCurrent != nil:
+		log.Printf("[TRACE] StateTransformer: creating nodes for current instance objects only")
+	case t.ConcreteDeposed != nil:
+		log.Printf("[TRACE] StateTransformer: creating nodes for deposed instance objects only")
+	default:
+		log.Printf("[TRACE] StateTransformer: pointless no-op call, creating no nodes at all")
+	}
+
 	for _, ms := range t.State.Modules {
 		moduleAddr := ms.Addr
 
 		for _, rs := range ms.Resources {
 			resourceAddr := rs.Addr.Absolute(moduleAddr)
 
-			for key := range rs.Instances {
+			for key, is := range rs.Instances {
 				addr := resourceAddr.Instance(key)
 
+				if obj := is.Current; obj != nil && t.ConcreteCurrent != nil {
 					abstract := NewNodeAbstractResourceInstance(addr)
-				var node dag.Vertex = abstract
+					node := t.ConcreteCurrent(abstract)
-				if f := t.Concrete; f != nil {
+					g.Add(node)
-					node = f(abstract)
+					log.Printf("[TRACE] StateTransformer: added %T for %s current object", node, addr)
 				}
 
+				if t.ConcreteDeposed != nil {
+					for dk := range is.Deposed {
+						abstract := NewNodeAbstractResourceInstance(addr)
+						node := t.ConcreteDeposed(abstract, dk)
 						g.Add(node)
-				log.Printf("[TRACE] StateTransformer: added %T for %s", node, addr)
+						log.Printf("[TRACE] StateTransformer: added %T for %s deposed object %s", node, addr, dk)
+					}
+				}
 			}
 		}
 	}