pruneUnusedNodesTransformer

Create a single transformer to remove all unused nodes from the apply
graph. This is similar to the combination of the resource pruning done
in the destroy edge transformer, and the unused values transformer. In
addition to resources, variables, locals, and outputs, we now need to
remove unused module expansion nodes as well. Since these can all be
interdependent, we need to process them as whole in a single
transformation.

terraform/graph_builder_apply.go

@@ -163,9 +163,10 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		// Create a destroy node for outputs to remove them from the state.
 		&DestroyOutputTransformer{Destroy: b.Destroy},
 
-		// Prune unreferenced values, which may have interpolations that can't
+		// We need to remove configuration nodes that are not used at all, as
-		// be resolved.
+		// they may not be able to evaluate, especially during destroy.
-		&pruneUnusedExpanderTransformer{},
+		// These include variables, locals, and instance expanders.
+		&pruneUnusedNodesTransformer{},
 
 		// Add the node to fix the state count boundaries
 		&CountBoundaryTransformer{

terraform/node_module_expand.go

@@ -1,7 +1,6 @@
 package terraform
 
 import (
-	"fmt"
 	"log"
 
 	"github.com/hashicorp/terraform/addrs"
@@ -10,12 +9,6 @@ import (
 	"github.com/hashicorp/terraform/lang"
 )
 
-// graphNodeModuleCloser is an interface implemented by nodes that finalize the
-// evaluation of modules.
-type graphNodeModuleCloser interface {
-	CloseModule() addrs.Module
-}
-
 type ConcreteModuleNodeFunc func(n *nodeExpandModule) dag.Vertex
 
 // nodeExpandModule represents a module call in the configuration that
@@ -126,14 +119,9 @@ func (n *nodeExpandModule) EvalTree() EvalNode {
 // empty resources and modules from the state.
 type nodeCloseModule struct {
 	Addr addrs.Module
-
-	// orphaned indicates that this module has no expansion, because it no
-	// longer exists in the configuration
-	orphaned bool
 }
 
 var (
-	_ graphNodeModuleCloser     = (*nodeCloseModule)(nil)
 	_ GraphNodeReferenceable    = (*nodeCloseModule)(nil)
 	_ GraphNodeReferenceOutside = (*nodeCloseModule)(nil)
 )
@@ -160,10 +148,6 @@ func (n *nodeCloseModule) Name() string {
 	return n.Addr.String() + " (close)"
 }
 
-func (n *nodeCloseModule) CloseModule() addrs.Module {
-	return n.Addr
-}
-
 // RemovableIfNotTargeted implementation
 func (n *nodeCloseModule) RemoveIfNotTargeted() bool {
 	// We need to add this so that this node will be removed if
@@ -178,7 +162,6 @@ func (n *nodeCloseModule) EvalTree() EvalNode {
 				Ops: []walkOperation{walkApply, walkDestroy},
 				Node: &evalCloseModule{
 					Addr: n.Addr,
-					orphaned: n.orphaned,
 				},
 			},
 		},
@@ -187,7 +170,6 @@ func (n *nodeCloseModule) EvalTree() EvalNode {
 
 type evalCloseModule struct {
 	Addr addrs.Module
-	orphaned bool
 }
 
 func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) {
@@ -196,20 +178,6 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) {
 	state := ctx.State().Lock()
 	defer ctx.State().Unlock()
 
-	expander := ctx.InstanceExpander()
-	var currentModuleInstances []addrs.ModuleInstance
-	// we can't expand if we're just removing
-	if !n.orphaned {
-		func() {
-			// FIXME: we need to "turn off" closers if their expander has been removed
-			defer func() {
-				recover()
-				n.orphaned = true
-			}()
-			currentModuleInstances = expander.ExpandModule(n.Addr)
-		}()
-	}
-
 	for modKey, mod := range state.Modules {
 		if !n.Addr.Equal(mod.Addr.Module()) {
 			continue
@@ -222,27 +190,8 @@ func (n *evalCloseModule) Eval(ctx EvalContext) (interface{}, error) {
 			}
 		}
 
-		found := false
+		// empty child modules are always removed
-		if n.orphaned {
+		if len(mod.Resources) == 0 && !mod.Addr.IsRoot() {
-			// we're removing the entire module, so all instances must go
-			found = true
-		} else {
-			// if this instance is not in the current expansion, remove it from
-			// the state
-			for _, current := range currentModuleInstances {
-				if current.Equal(mod.Addr) {
-					found = true
-					break
-				}
-			}
-		}
-
-		if !found {
-			if len(mod.Resources) > 0 {
-				// FIXME: add more info to this error
-				return nil, fmt.Errorf("module %q still contains resources in state", mod.Addr)
-			}
-
 			delete(state.Modules, modKey)
 		}
 	}

terraform/transform_destroy_edge.go

@@ -167,74 +167,87 @@ func (t *DestroyEdgeTransformer) Transform(g *Graph) error {
 	return nil
 }
 
-// Nodes that register instances in the instances.Expander are not needed
+// Remove any nodes that aren't needed when destroying modules.
-// during apply if there are no instances that will lookup the expansion. This
+// Variables, outputs, locals, and expanders may not be able to evaluate
-// is the case when a module tree is removed or during a full destroy, and we
+// correctly, so we can remove these if nothing depends on them. The module
-// may not be able to evaluate the expansion expression.
+// closers also need to disable their use of expansion if the module itself is
-type pruneUnusedExpanderTransformer struct {
+// no longer present.
+type pruneUnusedNodesTransformer struct {
 }
 
-func (t *pruneUnusedExpanderTransformer) Transform(g *Graph) error {
+func (t *pruneUnusedNodesTransformer) Transform(g *Graph) error {
-	// We need a reverse depth first walk of modules, but it needs to be
+	// We need a reverse depth first walk of modules, processing them in order
-	// recursive so that we can process the lead modules first.
+	// from the leaf modules to the root. This allows us to remove unneeded
+	// dependencies from child modules, freeing up nodes in the parent module
+	// to also be removed.
 
-	// collect all nodes into their containing module
+	// First collect the nodes into their respective modules based on
-	type mod struct {
+	// configuration path.
-		addr  addrs.Module
+	moduleMap := make(map[string]pruneUnusedNodesMod)
-		nodes []dag.Vertex
-	}
-
-	// first collect the nodes into their respective modules
-	moduleMap := make(map[string]*mod)
 	for _, v := range g.Vertices() {
 		var path addrs.Module
 		switch v := v.(type) {
 		case instanceExpander:
 			path = v.expandsInstances()
 
-		case graphNodeModuleCloser:
-			// module closers are connected like module calls, and report
-			// their parent module address
-			path = v.CloseModule()
-
 		case GraphNodeModulePath:
 			path = v.ModulePath()
+		default:
+			continue
+		}
+		m := moduleMap[path.String()]
+		m.addr = path
+		m.nodes = append(m.nodes, v)
+
+		// We need to keep track of the closers, to make sure they don't look
+		// for an expansion if there's nothing being expanded.
+		if c, ok := v.(*nodeCloseModule); ok {
+			m.closer = c
 		}
-		m, ok := moduleMap[path.String()]
-		if !ok {
-			m = &mod{}
 		moduleMap[path.String()] = m
 	}
 
-		m.addr = path
-		m.nodes = append(m.nodes, v)
-	}
-
 	// now we need to restructure the modules so we can sort them
-	var modules []*mod
+	var modules []pruneUnusedNodesMod
 
 	for _, mod := range moduleMap {
 		modules = append(modules, mod)
 	}
 
-	// Sort them by path length, longest first, so that we process the deepest
+	// Sort them by path length, longest first, so that start with the deepest
-	// modules first.  The order of modules at the same tree level doesn't
+	// modules.  The order of modules at the same tree level doesn't matter, we
-	// matter, we just need to ensure that child modules are processed before
+	// just need to ensure that child modules are processed before parent
-	// parent modules.
+	// modules.
 	sort.Slice(modules, func(i, j int) bool {
 		return len(modules[i].addr) > len(modules[j].addr)
 	})
 
-	for _, module := range modules {
+	for _, mod := range modules {
-		t.removeUnused(module.nodes, g)
+		mod.removeUnused(g)
 	}
 
 	return nil
 }
 
-func (t *pruneUnusedExpanderTransformer) removeUnused(nodes []dag.Vertex, g *Graph) {
+// pruneUnusedNodesMod is a container to hold the nodes that belong to a
+// particular configuration module for the pruneUnusedNodesTransformer
+type pruneUnusedNodesMod struct {
+	addr   addrs.Module
+	nodes  []dag.Vertex
+	closer *nodeCloseModule
+}
+
+// Remove any unused locals, variables, outputs and expanders.  Since module
+// closers can also lookup expansion info to detect orphaned instances, disable
+// them if their associated expander is removed.
+func (m *pruneUnusedNodesMod) removeUnused(g *Graph) {
+	// We modify the nodes slice during processing here.
+	// Make a copy so no one is surprised by this changing in the future.
+	nodes := make([]dag.Vertex, len(m.nodes))
+	copy(nodes, m.nodes)
+
 	// since we have no defined structure within the module, just cycle through
-	// the nodes until there are no more removals
+	// the nodes in each module until there are no more removals
 	removed := true
 	for {
 		if !removed {
@@ -242,51 +255,51 @@ func (t *pruneUnusedExpanderTransformer) removeUnused(nodes []dag.Vertex, g *Gra
 		}
 		removed = false
 
-		last := len(nodes) - 1
-
-	NEXT:
 		for i := 0; i < len(nodes); i++ {
+			// run this in a closure, so we can return early rather than
+			// dealing with complex looping and labels
+			func() {
 				n := nodes[i]
 				switch n.(type) {
 				case graphNodeTemporaryValue:
+					// temporary value, which consist of variables, locals, and
+					// outputs, must be kept if anything refers to them.
 					if n, ok := n.(GraphNodeModulePath); ok {
-					// root outputs always have a dependency on remote state
+						// root outputs always have an implicit dependency on
+						// remote state.
 						if n.ModulePath().IsRoot() {
-						continue NEXT
+							return
 						}
 					}
 					for _, vv := range g.UpEdges(n) {
+						// keep any value which is connected through a
+						// reference
 						if _, ok := vv.(GraphNodeReferencer); ok {
-						continue NEXT
+							return
 						}
 					}
 
 				case instanceExpander:
+					// Any nodes that expand instances are kept when their
+					// instances may need to be evaluated.
 					for _, vv := range g.UpEdges(n) {
 						if _, ok := vv.(requiresInstanceExpansion); ok {
-						continue NEXT
+							return
 						}
 					}
 
 				default:
-				continue NEXT
+					return
 				}
 
+				g.Remove(n)
 				removed = true
 
-			//// connect through edges
-			//for _, d := range g.DownEdges(n) {
-			//    for _, u := range g.UpEdges(n) {
-			//        g.Connect(dag.BasicEdge(u, d))
-			//    }
-			//}
-
-			g.Remove(n)
-
 				// remove the node from our iteration as well
+				last := len(nodes) - 1
 				nodes[i], nodes[last] = nodes[last], nodes[i]
 				nodes = nodes[:last]
-			last--
+			}()
 		}
 	}
 }

terraform/transform_module_expansion.go

@@ -43,7 +43,7 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error {
 	// the graph already, and need to be connected to their parent closers.
 	for _, v := range g.Vertices() {
 		// skip closers so they don't attach to themselves
-		if _, ok := v.(graphNodeModuleCloser); ok {
+		if _, ok := v.(*nodeCloseModule); ok {
 			continue
 		}
 		// any node that executes within the scope of a module should be a

terraform/transform_removed_modules.go

@@ -36,7 +36,6 @@ func (t *RemovedModuleTransformer) Transform(g *Graph) error {
 	for _, modAddr := range removed {
 		closer := &nodeCloseModule{
 			Addr: modAddr,
-			orphaned: true,
 		}
 		g.Add(closer)
 	}