core: Produce correct references for destroy nodes

Prior to the introduction of our "addrs" package, we represented destroy
nodes as a special kind of address string ending in ".destroy" or
".destroy-cbd".

Using references to resolve these dependencies is a strange idea to begin
with, since these are not user-visible addresses, but rather than refactor
that now we instead have these weird pseudo-address types ResourcePhase
and ResourceInstancePhase that correspond go those weird address suffixes,
thus restoring the prior behavior.

In future we should rework this so that destroy node edges are not handled
as references at all, and instead handled as part of
DestroyEdgeTransformer where there's better context for implementing this
logic and it can be maintained and tested in a single place.

addrs/resource_phase.go

@@ -30,6 +30,12 @@ func (r ResourceInstance) Phase(rpt ResourceInstancePhaseType) ResourceInstanceP
 	}
 }
 
+// ContainingResource returns an address for the same phase of the resource
+// that this instance belongs to.
+func (rp ResourceInstancePhase) ContainingResource() ResourcePhase {
+	return rp.ResourceInstance.Resource.Phase(rp.Phase)
+}
+
 func (rp ResourceInstancePhase) String() string {
 	// We use a different separator here than usual to ensure that we'll
 	// never conflict with any non-phased resource instance string. This
@@ -55,3 +61,45 @@ const (
 func (rpt ResourceInstancePhaseType) String() string {
 	return string(rpt)
 }
+
+// ResourcePhase is a special kind of reference used only internally
+// during graph building to represent resources that are in a
+// non-primary state.
+//
+// Graph nodes can declare themselves referenceable via a resource phase
+// or can declare that they reference a resource phase in order to accomodate
+// secondary graph nodes dealing with, for example, destroy actions.
+//
+// Since resources (as opposed to instances) aren't actually phased, this
+// address type is used only as an approximation during initial construction
+// of the resource-oriented plan graph, under the assumption that resource
+// instances with ResourceInstancePhase addresses will be created in dynamic
+// subgraphs during the graph walk.
+//
+// This special reference type cannot be accessed directly by end-users, and
+// should never be shown in the UI.
+type ResourcePhase struct {
+	referenceable
+	Resource Resource
+	Phase    ResourceInstancePhaseType
+}
+
+var _ Referenceable = ResourcePhase{}
+
+// Phase returns a special "phase address" for the receving instance. See the
+// documentation of ResourceInstancePhase for the limited situations where this
+// is intended to be used.
+func (r Resource) Phase(rpt ResourceInstancePhaseType) ResourcePhase {
+	return ResourcePhase{
+		Resource: r,
+		Phase:    rpt,
+	}
+}
+
+func (rp ResourcePhase) String() string {
+	// We use a different separator here than usual to ensure that we'll
+	// never conflict with any non-phased resource instance string. This
+	// is intentionally something that would fail parsing with ParseRef,
+	// because this special address type should never be exposed in the UI.
+	return fmt.Sprintf("%s#%s", rp.Resource, rp.Phase)
+}

terraform/node_resource_apply.go

@@ -52,12 +52,8 @@ func (n *NodeApplyableResourceInstance) References() []*addrs.Reference {
 				newRef.Remaining = nil // can't access attributes of something being destroyed
 				ret = append(ret, &newRef)
 			case addrs.Resource:
-				// We'll guess that this is actually a reference to a no-key
-				// instance here, and generate a reference under that assumption.
-				// If that's not true then this won't do any harm, since there
-				// won't actually be a node with this address.
 				newRef := *ref // shallow copy so we can mutate
-				newRef.Subject = tr.Instance(addrs.NoKey).Phase(addrs.ResourceInstancePhaseDestroy)
+				newRef.Subject = tr.Phase(addrs.ResourceInstancePhaseDestroy)
 				newRef.Remaining = nil // can't access attributes of something being destroyed
 				ret = append(ret, &newRef)
 			}

terraform/node_resource_destroy.go

@@ -55,20 +55,27 @@ func (n *NodeDestroyResourceInstance) ModifyCreateBeforeDestroy(v bool) error {
 }
 
 // GraphNodeReferenceable, overriding NodeAbstractResource
-func (n *NodeDestroyResourceInstance) ReferenceableName() []addrs.Referenceable {
-	relAddr := n.ResourceInstanceAddr().Resource
-	switch {
-	case n.CreateBeforeDestroy():
-		return []addrs.Referenceable{
-			relAddr.ContainingResource(),
-			relAddr.Phase(addrs.ResourceInstancePhaseDestroyCBD),
-		}
-	default:
-		return []addrs.Referenceable{
-			relAddr.ContainingResource(),
-			relAddr.Phase(addrs.ResourceInstancePhaseDestroy),
+func (n *NodeDestroyResourceInstance) ReferenceableAddrs() []addrs.Referenceable {
+	normalAddrs := n.NodeAbstractResourceInstance.ReferenceableAddrs()
+	destroyAddrs := make([]addrs.Referenceable, len(normalAddrs))
+
+	phaseType := addrs.ResourceInstancePhaseDestroy
+	if n.CreateBeforeDestroy() {
+		phaseType = addrs.ResourceInstancePhaseDestroyCBD
+	}
+
+	for i, normalAddr := range normalAddrs {
+		switch ta := normalAddr.(type) {
+		case addrs.Resource:
+			destroyAddrs[i] = ta.Phase(phaseType)
+		case addrs.ResourceInstance:
+			destroyAddrs[i] = ta.Phase(phaseType)
+		default:
+			destroyAddrs[i] = normalAddr
 		}
 	}
+
+	return destroyAddrs
 }
 
 // GraphNodeReferencer, overriding NodeAbstractResource

terraform/transform_reference.go

@@ -210,10 +210,13 @@ func (m *ReferenceMap) References(v dag.Vertex) ([]dag.Vertex, []addrs.Reference
 			// might be in a resource-oriented graph rather than an
 			// instance-oriented graph, and so we'll see if we have the
 			// resource itself instead.
-			if ri, ok := subject.(addrs.ResourceInstance); ok {
-				subject = ri.Resource
-				key = m.referenceMapKey(v, subject)
+			switch ri := subject.(type) {
+			case addrs.ResourceInstance:
+				subject = ri.ContainingResource()
+			case addrs.ResourceInstancePhase:
+				subject = ri.ContainingResource()
 			}
+			key = m.referenceMapKey(v, subject)
 		}
 
 		vertices := m.vertices[key]