always evaluate locals, even during destroy

Destroy-time provisioners require us to re-evaluate during destroy.

Rather than destroying local values, which doesn't do much since they
aren't persisted to state, we always evaluate them regardless of the
type of apply. Since the destroy-time local node is no longer a
"destroy" operation, the order of evaluation need to be reversed. Take
the existing DestroyValueReferenceTransformer and change it to reverse
the outgoing edges, rather than in incoming edges. This makes it so that
any dependencies of a local or output node are destroyed after
evaluation.

Having locals evaluated during destroy failed one other test, but that
was the odd case where we need `id` to exist as an attribute as well as
a field.

terraform/context_apply_test.go

@@ -7594,6 +7594,58 @@ aws_instance.bar:
 	`)
 }
 
+func TestContext2Apply_destroyProvisionerWithLocals(t *testing.T) {
+	m := testModule(t, "apply-provisioner-destroy-locals")
+	p := testProvider("aws")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = testDiffFn
+
+	pr := testProvisioner()
+	pr.ApplyFn = func(_ *InstanceState, rc *ResourceConfig) error {
+		cmd, ok := rc.Get("command")
+		if !ok || cmd != "local" {
+			fmt.Printf("%#v\n", rc.Config)
+			return fmt.Errorf("provisioner got %v:%s", ok, cmd)
+		}
+		return nil
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		ProviderResolver: ResourceProviderResolverFixed(
+			map[string]ResourceProviderFactory{
+				"aws": testProviderFuncFixed(p),
+			},
+		),
+		Provisioners: map[string]ResourceProvisionerFactory{
+			"shell": testProvisionerFuncFixed(pr),
+		},
+		State: &State{
+			Modules: []*ModuleState{
+				&ModuleState{
+					Path: rootModulePath,
+					Resources: map[string]*ResourceState{
+						"aws_instance.foo": resourceState("aws_instance", "1234"),
+					},
+				},
+			},
+		},
+		Destroy: true,
+		// the test works without targeting, but this also tests that the local
+		// node isn't inadvertently pruned because of the wrong evaluation
+		// order.
+		Targets: []string{"aws_instance.foo"},
+	})
+
+	if _, err := ctx.Plan(); err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := ctx.Apply(); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestContext2Apply_targetedDestroyCountDeps(t *testing.T) {
 	m := testModule(t, "apply-destroy-targeted-count")
 	p := testProvider("aws")
@@ -9000,6 +9052,10 @@ func TestContext2Apply_destroyWithLocals(t *testing.T) {
 						Type: "aws_instance",
 						Primary: &InstanceState{
 							ID: "foo",
+							// FIXME: id should only exist in one place
+							Attributes: map[string]string{
+								"id": "foo",
+							},
 						},
 					},
 				},

terraform/graph_builder_apply.go

@@ -119,7 +119,7 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
 		// Connect references so ordering is correct
 		&ReferenceTransformer{},
 
-		// Reverse the edges to outputs and locals, so that
+		// Reverse the edges from outputs and locals, so that
 		// interpolations don't fail during destroy.
 		GraphTransformIf(
 			func() bool { return b.Destroy },

terraform/node_local.go

@@ -59,38 +59,8 @@ func (n *NodeLocal) References() []string {
 
 // GraphNodeEvalable
 func (n *NodeLocal) EvalTree() EvalNode {
-	return &EvalSequence{
-		Nodes: []EvalNode{
-			&EvalOpFilter{
-				Ops: []walkOperation{
-					walkInput,
-					walkValidate,
-					walkRefresh,
-					walkPlan,
-					walkApply,
-				},
-				Node: &EvalSequence{
-					Nodes: []EvalNode{
-						&EvalLocal{
-							Name:  n.Config.Name,
-							Value: n.Config.RawConfig,
-						},
-					},
-				},
-			},
-			&EvalOpFilter{
-				Ops: []walkOperation{
-					walkPlanDestroy,
-					walkDestroy,
-				},
-				Node: &EvalSequence{
-					Nodes: []EvalNode{
-						&EvalDeleteLocal{
-							Name: n.Config.Name,
-						},
-					},
-				},
-			},
-		},
+	return &EvalLocal{
+		Name:  n.Config.Name,
+		Value: n.Config.RawConfig,
 	}
 }

terraform/test-fixtures/apply-provisioner-destroy-locals/main.tf

@@ -0,0 +1,14 @@
+locals {
+  value = "local"
+}
+
+resource "aws_instance" "foo" {
+    provisioner "shell" {
+        command  = "${local.value}"
+        when = "create"
+    }
+    provisioner "shell" {
+        command  = "${local.value}"
+        when = "destroy"
+    }
+}

terraform/transform_reference.go

@@ -77,15 +77,14 @@ func (t *ReferenceTransformer) Transform(g *Graph) error {
 }
 
 // DestroyReferenceTransformer is a GraphTransformer that reverses the edges
-// for nodes that depend on an Output or Local value. Output and local nodes are
-// removed during destroy, so anything which depends on them must be evaluated
-// first. These can't be interpolated during destroy, so the stored value must
-// be used anyway hence they don't need to be re-evaluated.
+// for locals and outputs that depend on other nodes which will be
+// removed during destroy. If a destroy node is evaluated before the local or
+// output value, it will be removed from the state, and the later interpolation
+// will fail.
 type DestroyValueReferenceTransformer struct{}
 
 func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error {
 	vs := g.Vertices()
-
 	for _, v := range vs {
 		switch v.(type) {
 		case *NodeApplyableOutput, *NodeLocal:
@@ -94,13 +93,13 @@ func (t *DestroyValueReferenceTransformer) Transform(g *Graph) error {
 			continue
 		}
 
-		// reverse any incoming edges so that the value is removed last
-		for _, e := range g.EdgesTo(v) {
-			source := e.Source()
-			log.Printf("[TRACE] output dep: %s", dag.VertexName(source))
+		// reverse any outgoing edges so that the value is evaluated first.
+		for _, e := range g.EdgesFrom(v) {
+			target := e.Target()
+			log.Printf("[TRACE] output dep: %s", dag.VertexName(target))
 
 			g.RemoveEdge(e)
-			g.Connect(&DestroyEdge{S: v, T: source})
+			g.Connect(&DestroyEdge{S: target, T: v})
 		}
 	}