states: Local values no longer live in state

Back when we added local values (a long time ago now!) we put their
results in state mainly just because it was the only suitable shared data
structure to keep them in. They are a bit ideosyncratic there because we
intentionally discard them when serializing state to a snapshot, and
that's just fine because they never need to be retained between runs
anyway.

We now have namedvals.State for all of our named value result storage
needs, so we can remove the local-value-related fields of states.Module
and use the relevant map inside the local value state instead.

As of this commit, states.State now tracks only the data that we
actually persist between runs in state snapshots, which will hopefully
avoid future bugs resulting from the former difference in fidelity
between a freshly-created in-memory state vs. one loaded from a
snapshot.

internal/command/jsonformat/state_test.go

@@ -252,7 +252,6 @@ func basicState(t *testing.T) *states.State {
 		t.Errorf("root module is nil; want valid object")
 	}
 
-	rootModule.SetLocalValue("foo", cty.StringVal("foo value"))
 	state.SetOutputValue(
 		addrs.OutputValue{Name: "bar"}.Absolute(addrs.RootModuleInstance),
 		cty.StringVal("bar value"), false,

internal/states/module.go

@@ -5,7 +5,6 @@ package states
 
 import (
 	"github.com/hashicorp/terraform/internal/addrs"
-	"github.com/zclconf/go-cty/cty"
 )
 
 // Module is a container for the states of objects within a particular module.
@@ -15,18 +14,13 @@ type Module struct {
 	// Resources contains the state for each resource. The keys in this map are
 	// an implementation detail and must not be used by outside callers.
 	Resources map[string]*Resource
-
-	// LocalValues contains the value for each named output value. The keys
-	// in this map are local value names.
-	LocalValues map[string]cty.Value
 }
 
 // NewModule constructs an empty module state for the given module address.
 func NewModule(addr addrs.ModuleInstance) *Module {
 	return &Module{
-		Addr:        addr,
-		Resources:   map[string]*Resource{},
-		LocalValues: map[string]cty.Value{},
+		Addr:      addr,
+		Resources: map[string]*Resource{},
 	}
 }
 
@@ -247,19 +241,6 @@ func (ms *Module) maybeRestoreResourceInstanceDeposed(addr addrs.ResourceInstanc
 	return true
 }
 
-// SetLocalValue writes a local value into the state, overwriting any
-// existing value of the same name.
-func (ms *Module) SetLocalValue(name string, value cty.Value) {
-	ms.LocalValues[name] = value
-}
-
-// RemoveLocalValue removes the local value of the given name from the state,
-// if it exists. This method is a no-op if there is no value of the given
-// name.
-func (ms *Module) RemoveLocalValue(name string) {
-	delete(ms.LocalValues, name)
-}
-
 // PruneResourceHusks is a specialized method that will remove any Resource
 // objects that do not contain any instances, even if they have an EachMode.
 //

internal/states/state.go

@@ -7,10 +7,9 @@ import (
 	"fmt"
 	"sort"
 
-	"github.com/zclconf/go-cty/cty"
-
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/getproviders"
+	"github.com/zclconf/go-cty/cty"
 )
 
 // State is the top-level type of a Terraform state.
@@ -323,16 +322,6 @@ func (s *State) RemoveOutputValue(addr addrs.AbsOutputValue) {
 	delete(s.RootOutputValues, addr.OutputValue.Name)
 }
 
-// LocalValue returns the value of the named local value with the given address,
-// or cty.NilVal if no such value is tracked in the state.
-func (s *State) LocalValue(addr addrs.AbsLocalValue) cty.Value {
-	ms := s.Module(addr.Module)
-	if ms == nil {
-		return cty.NilVal
-	}
-	return ms.LocalValues[addr.LocalValue.Name]
-}
-
 // ProviderAddrs returns a list of all of the provider configuration addresses
 // referenced throughout the receiving state.
 //

internal/states/state_deepcopy.go

@@ -59,16 +59,10 @@ func (ms *Module) DeepCopy() *Module {
 	for k, r := range ms.Resources {
 		resources[k] = r.DeepCopy()
 	}
-	localValues := make(map[string]cty.Value, len(ms.LocalValues))
-	for k, v := range ms.LocalValues {
-		// cty.Value is immutable, so we don't need to copy these.
-		localValues[k] = v
-	}
 
 	return &Module{
-		Addr:        ms.Addr, // technically mutable, but immutable by convention
-		Resources:   resources,
-		LocalValues: localValues,
+		Addr:      ms.Addr, // technically mutable, but immutable by convention
+		Resources: resources,
 	}
 }
 

internal/states/state_test.go

@@ -28,7 +28,6 @@ func TestState(t *testing.T) {
 		t.Errorf("root module is nil; want valid object")
 	}
 
-	rootModule.SetLocalValue("foo", cty.StringVal("foo value"))
 	state.SetOutputValue(
 		addrs.OutputValue{Name: "bar"}.Absolute(addrs.RootModuleInstance),
 		cty.StringVal("bar value"), false,
@@ -91,9 +90,6 @@ func TestState(t *testing.T) {
 		Modules: map[string]*Module{
 			"": {
 				Addr: addrs.RootModuleInstance,
-				LocalValues: map[string]cty.Value{
-					"foo": cty.StringVal("foo value"),
-				},
 				Resources: map[string]*Resource{
 					"test_thing.baz": {
 						Addr: addrs.Resource{
@@ -120,19 +116,16 @@ func TestState(t *testing.T) {
 				},
 			},
 			"module.child": {
-				Addr:        addrs.RootModuleInstance.Child("child", addrs.NoKey),
-				LocalValues: map[string]cty.Value{},
-				Resources:   map[string]*Resource{},
+				Addr:      addrs.RootModuleInstance.Child("child", addrs.NoKey),
+				Resources: map[string]*Resource{},
 			},
 			`module.multi["a"]`: {
-				Addr:        addrs.RootModuleInstance.Child("multi", addrs.StringKey("a")),
-				LocalValues: map[string]cty.Value{},
-				Resources:   map[string]*Resource{},
+				Addr:      addrs.RootModuleInstance.Child("multi", addrs.StringKey("a")),
+				Resources: map[string]*Resource{},
 			},
 			`module.multi["b"]`: {
-				Addr:        addrs.RootModuleInstance.Child("multi", addrs.StringKey("b")),
-				LocalValues: map[string]cty.Value{},
-				Resources:   map[string]*Resource{},
+				Addr:      addrs.RootModuleInstance.Child("multi", addrs.StringKey("b")),
+				Resources: map[string]*Resource{},
 			},
 		},
 	}
@@ -189,7 +182,6 @@ func TestStateDeepCopy(t *testing.T) {
 		t.Fatalf("root module is nil; want valid object")
 	}
 
-	rootModule.SetLocalValue("foo", cty.StringVal("foo value"))
 	state.SetOutputValue(
 		addrs.OutputValue{Name: "bar"}.Absolute(rootModule.Addr),
 		cty.StringVal("bar value"), false,

internal/states/sync.go

@@ -103,44 +103,6 @@ func (s *SyncState) RemoveOutputValue(addr addrs.AbsOutputValue) {
 	s.state.RemoveOutputValue(addr)
 }
 
-// LocalValue returns the current value associated with the given local value
-// address.
-func (s *SyncState) LocalValue(addr addrs.AbsLocalValue) cty.Value {
-	s.lock.RLock()
-	// cty.Value is immutable, so we don't need any extra copying here.
-	ret := s.state.LocalValue(addr)
-	s.lock.RUnlock()
-	return ret
-}
-
-// SetLocalValue writes a given output value into the state, overwriting
-// any existing value of the same name.
-//
-// If the module containing the local value is not yet tracked in state then it
-// will be added as a side-effect.
-func (s *SyncState) SetLocalValue(addr addrs.AbsLocalValue, value cty.Value) {
-	defer s.beginWrite()()
-
-	ms := s.state.EnsureModule(addr.Module)
-	ms.SetLocalValue(addr.LocalValue.Name, value)
-}
-
-// RemoveLocalValue removes the stored value for the local value with the
-// given address.
-//
-// If this results in its containing module being empty, the module will be
-// pruned from the state as a side-effect.
-func (s *SyncState) RemoveLocalValue(addr addrs.AbsLocalValue) {
-	defer s.beginWrite()()
-
-	ms := s.state.Module(addr.Module)
-	if ms == nil {
-		return
-	}
-	ms.RemoveLocalValue(addr.LocalValue.Name)
-	s.maybePruneModule(addr.Module)
-}
-
 // Resource returns a snapshot of the state of the resource with the given
 // address, or nil if no such resource is tracked.
 //

internal/terraform/context_apply2_test.go

@@ -2272,16 +2272,29 @@ locals {
 		t.Errorf("expected no errors, but got %s", diags)
 	}
 
-	state, diags := ctx.Apply(plan, m, nil)
-	if diags.HasErrors() {
-		t.Errorf("expected no errors, but got %s", diags)
+	g, _, diags := ctx.applyGraph(plan, m, &ApplyOpts{}, true)
+	assertNoDiagnostics(t, diags)
+
+	// The local value should've been pruned from the graph because nothing
+	// refers to it.
+	gotGraph := g.String()
+	wantGraph := `provider["registry.terraform.io/hashicorp/test"]
+provider["registry.terraform.io/hashicorp/test"] (close)
+  test_object.a
+root
+  provider["registry.terraform.io/hashicorp/test"] (close)
+test_object.a
+  test_object.a (expand)
+test_object.a (expand)
+  provider["registry.terraform.io/hashicorp/test"]
+`
+	if diff := cmp.Diff(wantGraph, gotGraph); diff != "" {
+		t.Errorf("wrong apply graph\n%s", diff)
 	}
 
-	// We didn't specify any external references, so the unreferenced local
-	// value should have been tidied up and never made it into the state.
-	module := state.RootModule()
-	if len(module.LocalValues) > 0 {
-		t.Errorf("expected no local values in the state but found %d", len(module.LocalValues))
+	_, diags = ctx.Apply(plan, m, nil)
+	if diags.HasErrors() {
+		t.Errorf("expected no errors, but got %s", diags)
 	}
 }
 
@@ -2315,17 +2328,36 @@ locals {
 		t.Errorf("expected no errors, but got %s", diags)
 	}
 
-	state, diags := ctx.Apply(plan, m, nil)
+	g, _, diags := ctx.applyGraph(plan, m, &ApplyOpts{}, true)
 	if diags.HasErrors() {
 		t.Errorf("expected no errors, but got %s", diags)
 	}
 
-	// We did specify the local value in the external references, so it should
-	// have been preserved even though it is not referenced by anything directly
-	// in the config.
-	module := state.RootModule()
-	if module.LocalValues["local_value"].AsString() != "foo" {
-		t.Errorf("expected local value to be \"foo\" but was \"%s\"", module.LocalValues["local_value"].AsString())
+	// The local value should remain in the graph because the external
+	// reference uses it.
+	gotGraph := g.String()
+	wantGraph := `<external ref to local.local_value>
+  local.local_value (expand)
+local.local_value (expand)
+  test_object.a
+provider["registry.terraform.io/hashicorp/test"]
+provider["registry.terraform.io/hashicorp/test"] (close)
+  test_object.a
+root
+  <external ref to local.local_value>
+  provider["registry.terraform.io/hashicorp/test"] (close)
+test_object.a
+  test_object.a (expand)
+test_object.a (expand)
+  provider["registry.terraform.io/hashicorp/test"]
+`
+	if diff := cmp.Diff(wantGraph, gotGraph); diff != "" {
+		t.Errorf("wrong graph\n%s", diff)
+	}
+
+	_, diags = ctx.Apply(plan, m, nil)
+	if diags.HasErrors() {
+		t.Errorf("expected no errors, but got %s", diags)
 	}
 }
 

internal/terraform/context_plan2_test.go

@@ -4234,6 +4234,10 @@ resource "test_object" "a" {
 locals {
   local_value = test_object.a.test_string
 }
+
+output "from_local_value" {
+  value = local.local_value
+}
 `,
 	})
 
@@ -4253,20 +4257,20 @@ locals {
 	module := state.RootModule()
 
 	// So, the original state shouldn't have been updated at all.
-	if len(module.LocalValues) > 0 {
-		t.Errorf("expected no local values in the state but found %d", len(module.LocalValues))
+	if len(state.RootOutputValues) > 0 {
+		t.Errorf("expected no root output values in the state but found %d", len(state.RootOutputValues))
 	}
 
 	if len(module.Resources) > 0 {
-		t.Errorf("expected no resources in the state but found %d", len(module.LocalValues))
+		t.Errorf("expected no resources in the state but found %d", len(module.Resources))
 	}
 
 	// But, this makes it hard for the testing framework to valid things about
 	// the returned plan. So, the plan contains the planned state:
 	module = plan.PlannedState.RootModule()
 
-	if module.LocalValues["local_value"].AsString() != "foo" {
-		t.Errorf("expected local value to be \"foo\" but was \"%s\"", module.LocalValues["local_value"].AsString())
+	if got, want := plan.PlannedState.RootOutputValues["from_local_value"].Value.AsString(), "foo"; got != want {
+		t.Errorf("expected local value to be %q but was %q", want, got)
 	}
 
 	if module.ResourceInstance(addr.Resource).Current.Status != states.ObjectPlanned {

internal/terraform/evaluate.go

@@ -303,12 +303,7 @@ func (d *evaluationStateData) GetLocalValue(addr addrs.LocalValue, rng tfdiags.S
 		return cty.DynamicVal, diags
 	}
 
-	val := d.Evaluator.State.LocalValue(addr.Absolute(d.ModulePath))
-	if val == cty.NilVal {
-		// Not evaluated yet?
-		val = cty.DynamicVal
-	}
-
+	val := d.Evaluator.NamedValues.GetLocalValue(addr.Absolute(d.ModulePath))
 	return val, diags
 }
 

internal/terraform/node_external_reference.go

@@ -3,7 +3,13 @@
 
 package terraform
 
-import "github.com/hashicorp/terraform/internal/addrs"
+import (
+	"fmt"
+	"sort"
+	"strings"
+
+	"github.com/hashicorp/terraform/internal/addrs"
+)
 
 // nodeExternalReference allows external callers (such as the testing framework)
 // to provide the list of references they are making into the graph. This
@@ -32,3 +38,13 @@ func (n *nodeExternalReference) ModulePath() addrs.Module {
 func (n *nodeExternalReference) References() []*addrs.Reference {
 	return n.ExternalReferences
 }
+
+// Name implements dag.NamedVertex
+func (n *nodeExternalReference) Name() string {
+	names := make([]string, len(n.ExternalReferences))
+	for i, ref := range n.ExternalReferences {
+		names[i] = ref.DisplayString()
+	}
+	sort.Strings(names)
+	return fmt.Sprintf("<external ref to %s>", strings.Join(names, ", "))
+}

internal/terraform/node_local.go

@@ -161,13 +161,7 @@ func (n *NodeLocal) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Di
 		return diags
 	}
 
-	state := ctx.State()
-	if state == nil {
-		diags = diags.Append(fmt.Errorf("cannot write local value to nil state"))
-		return diags
-	}
-
-	state.SetLocalValue(addr.Absolute(ctx.Path()), val)
+	ctx.NamedValues().SetLocalValue(addr.Absolute(ctx.Path()), val)
 
 	return diags
 }

internal/terraform/node_local_test.go

@@ -4,17 +4,15 @@
 package terraform
 
 import (
-	"reflect"
 	"testing"
 
-	"github.com/davecgh/go-spew/spew"
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
-	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/configs"
 	"github.com/hashicorp/terraform/internal/configs/hcl2shim"
+	"github.com/hashicorp/terraform/internal/namedvals"
 	"github.com/hashicorp/terraform/internal/states"
 )
 
@@ -48,14 +46,16 @@ func TestNodeLocalExecute(t *testing.T) {
 				t.Fatal(diags.Error())
 			}
 
+			localAddr := addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance)
 			n := &NodeLocal{
-				Addr: addrs.LocalValue{Name: "foo"}.Absolute(addrs.RootModuleInstance),
+				Addr: localAddr,
 				Config: &configs.Local{
 					Expr: expr,
 				},
 			}
 			ctx := &MockEvalContext{
-				StateState: states.NewState().SyncWrapper(),
+				StateState:       states.NewState().SyncWrapper(),
+				NamedValuesState: namedvals.NewState(),
 
 				EvaluateExprResult: hcl2shim.HCL2ValueFromConfigValue(test.Want),
 			}
@@ -69,18 +69,19 @@ func TestNodeLocalExecute(t *testing.T) {
 				}
 			}
 
-			ms := ctx.StateState.Module(addrs.RootModuleInstance)
-			gotLocals := ms.LocalValues
-			wantLocals := map[string]cty.Value{}
-			if test.Want != nil {
-				wantLocals["foo"] = hcl2shim.HCL2ValueFromConfigValue(test.Want)
-			}
-
-			if !reflect.DeepEqual(gotLocals, wantLocals) {
-				t.Errorf(
-					"wrong locals after Eval\ngot:  %swant: %s",
-					spew.Sdump(gotLocals), spew.Sdump(wantLocals),
-				)
+			if test.Err {
+				if ctx.NamedValues().HasLocalValue(localAddr) {
+					t.Errorf("have value for %s, but wanted none", localAddr)
+				}
+			} else {
+				if !ctx.NamedValues().HasLocalValue(localAddr) {
+					t.Fatalf("no value for %s", localAddr)
+				}
+				got := ctx.NamedValues().GetLocalValue(localAddr)
+				want := hcl2shim.HCL2ValueFromConfigValue(test.Want)
+				if !want.RawEquals(got) {
+					t.Errorf("wrong value for %s\ngot:  %#v\nwant: %#v", localAddr, got, want)
+				}
 			}
 		})
 	}