addrs: Central implementation of sets and maps with UniqueKeyer address types

go.mod

@@ -83,13 +83,13 @@ require (
 	github.com/zclconf/go-cty-yaml v1.0.2
 	go.etcd.io/etcd v0.5.0-alpha.5.0.20210428180535-15715dcf1ace
 	golang.org/x/crypto v0.0.0-20220518034528-6f7dac969898
-	golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3
+	golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4
 	golang.org/x/net v0.0.0-20211216030914-fe4d6282115f
 	golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f
 	golang.org/x/sys v0.0.0-20220517195934-5e4e11fc645e
 	golang.org/x/term v0.0.0-20210615171337-6886f2dfbf5b
 	golang.org/x/text v0.3.7
-	golang.org/x/tools v0.1.11-0.20220316014157-77aa08bb151a
+	golang.org/x/tools v0.1.11
 	google.golang.org/api v0.44.0-impersonate-preview
 	google.golang.org/grpc v1.36.1
 	google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0
@@ -206,4 +206,4 @@ replace github.com/golang/mock v1.5.0 => github.com/golang/mock v1.4.4
 // replacement that includes a fix for CVE-2020-26160.
 replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt v3.2.1+incompatible
 
-go 1.17
+go 1.18

internal/addrs/map.go

@@ -0,0 +1,128 @@
+package addrs
+
+// Map represents a mapping whose keys are address types that implement
+// UniqueKeyer.
+//
+// Since not all address types are comparable in the Go language sense, this
+// type cannot work with the typical Go map access syntax, and so instead has
+// a method-based syntax. Use this type only for situations where the key
+// type isn't guaranteed to always be a valid key for a standard Go map.
+type Map[K UniqueKeyer, V any] struct {
+	// Elems is the internal data structure of the map.
+	//
+	// This is exported to allow for comparisons during tests and other similar
+	// careful read operations, but callers MUST NOT modify this map directly.
+	// Use only the methods of Map to modify the contents of this structure,
+	// to ensure that it remains correct and consistent.
+	Elems map[UniqueKey]MapElem[K, V]
+}
+
+type MapElem[K UniqueKeyer, V any] struct {
+	Key   K
+	Value V
+}
+
+func MakeMap[K UniqueKeyer, V any](initialElems ...MapElem[K, V]) Map[K, V] {
+	inner := make(map[UniqueKey]MapElem[K, V], len(initialElems))
+	ret := Map[K, V]{inner}
+	for _, elem := range initialElems {
+		ret.Put(elem.Key, elem.Value)
+	}
+	return ret
+}
+
+func MakeMapElem[K UniqueKeyer, V any](key K, value V) MapElem[K, V] {
+	return MapElem[K, V]{key, value}
+}
+
+// Put inserts a new element into the map, or replaces an existing element
+// which has an equivalent key.
+func (m Map[K, V]) Put(key K, value V) {
+	realKey := key.UniqueKey()
+	m.Elems[realKey] = MapElem[K, V]{key, value}
+}
+
+// PutElement is like Put but takes the key and value from the given MapElement
+// structure instead of as individual arguments.
+func (m Map[K, V]) PutElement(elem MapElem[K, V]) {
+	m.Put(elem.Key, elem.Value)
+}
+
+// Remove deletes the element with the given key from the map, or does nothing
+// if there is no such element.
+func (m Map[K, V]) Remove(key K) {
+	realKey := key.UniqueKey()
+	delete(m.Elems, realKey)
+}
+
+// Get returns the value of the element with the given key, or the zero value
+// of V if there is no such element.
+func (m Map[K, V]) Get(key K) V {
+	realKey := key.UniqueKey()
+	return m.Elems[realKey].Value
+}
+
+// GetOk is like Get but additionally returns a flag for whether there was an
+// element with the given key present in the map.
+func (m Map[K, V]) GetOk(key K) (V, bool) {
+	realKey := key.UniqueKey()
+	elem, ok := m.Elems[realKey]
+	return elem.Value, ok
+}
+
+// Has returns true if and only if there is an element in the map which has the
+// given key.
+func (m Map[K, V]) Has(key K) bool {
+	realKey := key.UniqueKey()
+	_, ok := m.Elems[realKey]
+	return ok
+}
+
+// Len returns the number of elements in the map.
+func (m Map[K, V]) Len() int {
+	return len(m.Elems)
+}
+
+// Elements returns a slice containing a snapshot of the current elements of
+// the map, in an unpredictable order.
+func (m Map[K, V]) Elements() []MapElem[K, V] {
+	if len(m.Elems) == 0 {
+		return nil
+	}
+	ret := make([]MapElem[K, V], 0, len(m.Elems))
+	for _, elem := range m.Elems {
+		ret = append(ret, elem)
+	}
+	return ret
+}
+
+// Keys returns a Set[K] containing a snapshot of the current keys of elements
+// of the map.
+func (m Map[K, V]) Keys() Set[K] {
+	if len(m.Elems) == 0 {
+		return nil
+	}
+	ret := make(Set[K], len(m.Elems))
+
+	// We mess with the internals of Set here, rather than going through its
+	// public interface, because that means we can avoid re-calling UniqueKey
+	// on all of the elements when we know that our own Put method would have
+	// already done the same thing.
+	for realKey, elem := range m.Elems {
+		ret[realKey] = elem.Key
+	}
+	return ret
+}
+
+// Values returns a slice containing a snapshot of the current values of
+// elements of the map, in an unpredictable order.
+func (m Map[K, V]) Values() []V {
+	if len(m.Elems) == 0 {
+		return nil
+	}
+	ret := make([]V, 0, len(m.Elems))
+	for _, elem := range m.Elems {
+		ret = append(ret, elem.Value)
+	}
+	return ret
+}

internal/addrs/map_test.go

@@ -0,0 +1,83 @@
+package addrs
+
+import (
+	"testing"
+)
+
+func TestMap(t *testing.T) {
+	variableName := InputVariable{Name: "name"}
+	localHello := LocalValue{Name: "hello"}
+	pathModule := PathAttr{Name: "module"}
+	moduleBeep := ModuleCall{Name: "beep"}
+	eachKey := ForEachAttr{Name: "key"} // intentionally not in the map
+
+	m := MakeMap(
+		MakeMapElem[Referenceable](variableName, "Aisling"),
+	)
+
+	m.Put(localHello, "hello")
+	m.Put(pathModule, "boop")
+	m.Put(moduleBeep, "unrealistic")
+
+	keySet := m.Keys()
+	if want := variableName; !m.Has(want) {
+		t.Errorf("map does not include %s", want)
+	}
+	if want := variableName; !keySet.Has(want) {
+		t.Errorf("key set does not include %s", want)
+	}
+	if want := localHello; !m.Has(want) {
+		t.Errorf("map does not include %s", want)
+	}
+	if want := localHello; !keySet.Has(want) {
+		t.Errorf("key set does not include %s", want)
+	}
+	if want := pathModule; !keySet.Has(want) {
+		t.Errorf("key set does not include %s", want)
+	}
+	if want := moduleBeep; !keySet.Has(want) {
+		t.Errorf("key set does not include %s", want)
+	}
+	if doNotWant := eachKey; m.Has(doNotWant) {
+		t.Errorf("map includes rogue element %s", doNotWant)
+	}
+	if doNotWant := eachKey; keySet.Has(doNotWant) {
+		t.Errorf("key set includes rogue element %s", doNotWant)
+	}
+
+	if got, want := m.Get(variableName), "Aisling"; got != want {
+		t.Errorf("unexpected value %q for %s; want %q", got, variableName, want)
+	}
+	if got, want := m.Get(localHello), "hello"; got != want {
+		t.Errorf("unexpected value %q for %s; want %q", got, localHello, want)
+	}
+	if got, want := m.Get(pathModule), "boop"; got != want {
+		t.Errorf("unexpected value %q for %s; want %q", got, pathModule, want)
+	}
+	if got, want := m.Get(moduleBeep), "unrealistic"; got != want {
+		t.Errorf("unexpected value %q for %s; want %q", got, moduleBeep, want)
+	}
+	if got, want := m.Get(eachKey), ""; got != want {
+		// eachKey isn't in the map, so Get returns the zero value of string
+		t.Errorf("unexpected value %q for %s; want %q", got, eachKey, want)
+	}
+
+	if v, ok := m.GetOk(variableName); v != "Aisling" || !ok {
+		t.Errorf("GetOk for %q returned incorrect result (%q, %#v)", variableName, v, ok)
+	}
+	if v, ok := m.GetOk(eachKey); v != "" || ok {
+		t.Errorf("GetOk for %q returned incorrect result (%q, %#v)", eachKey, v, ok)
+	}
+
+	m.Remove(moduleBeep)
+	if doNotWant := moduleBeep; m.Has(doNotWant) {
+		t.Errorf("map still includes %s after removing it", doNotWant)
+	}
+	if want := moduleBeep; !keySet.Has(want) {
+		t.Errorf("key set no longer includes %s after removing it from the map; key set is supposed to be a snapshot at the time of call", want)
+	}
+	keySet = m.Keys()
+	if doNotWant := moduleBeep; keySet.Has(doNotWant) {
+		t.Errorf("key set still includes %s after a second call after removing it from the map", doNotWant)
+	}
+}

internal/addrs/set.go

@@ -1,23 +1,37 @@
 package addrs
 
 // Set represents a set of addresses of types that implement UniqueKeyer.
-type Set map[UniqueKey]UniqueKeyer
+//
+// Modify the set only by the methods on this type. This type exposes its
+// internals for convenience during reading, such as iterating over set elements
+// by ranging over the map values, but making direct modifications could
+// potentially make the set data invalid or inconsistent, leading to undefined
+// behavior elsewhere.
+type Set[T UniqueKeyer] map[UniqueKey]T
 
-func (s Set) Has(addr UniqueKeyer) bool {
+// Has returns true if and only if the set includes the given address.
+func (s Set[T]) Has(addr T) bool {
 	_, exists := s[addr.UniqueKey()]
 	return exists
 }
 
-func (s Set) Add(addr UniqueKeyer) {
+// Add inserts the given address into the set, if not already present. If
+// an equivalent address is already in the set, this replaces that address
+// with the new value.
+func (s Set[T]) Add(addr T) {
 	s[addr.UniqueKey()] = addr
 }
 
-func (s Set) Remove(addr UniqueKeyer) {
+// Remove deletes the given address from the set, if present. If not present,
+// this is a no-op.
+func (s Set[T]) Remove(addr T) {
 	delete(s, addr.UniqueKey())
 }
 
-func (s Set) Union(other Set) Set {
-	ret := make(Set)
+// Union returns a new set which contains the union of all of the elements
+// of both the reciever and the given other set.
+func (s Set[T]) Union(other Set[T]) Set[T] {
+	ret := make(Set[T])
 	for k, addr := range s {
 		ret[k] = addr
 	}
@@ -27,8 +41,10 @@ func (s Set) Union(other Set) Set {
 	return ret
 }
 
-func (s Set) Intersection(other Set) Set {
-	ret := make(Set)
+// Intersection returns a new set which contains the intersection of all of the
+// elements of both the reciever and the given other set.
+func (s Set[T]) Intersection(other Set[T]) Set[T] {
+	ret := make(Set[T])
 	for k, addr := range s {
 		if _, exists := other[k]; exists {
 			ret[k] = addr

internal/addrs/unique_key.go

@@ -21,3 +21,7 @@ type UniqueKey interface {
 type UniqueKeyer interface {
 	UniqueKey() UniqueKey
 }
+
+func Equivalent[T UniqueKeyer](a, b T) bool {
+	return a.UniqueKey() == b.UniqueKey()
+}

internal/refactoring/move_execute.go

@@ -26,10 +26,7 @@ import (
 // ApplyMoves expects exclusive access to the given state while it's running.
 // Don't read or write any part of the state structure until ApplyMoves returns.
 func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults {
-	ret := MoveResults{
-		Changes: make(map[addrs.UniqueKey]MoveSuccess),
-		Blocked: make(map[addrs.UniqueKey]MoveBlocked),
-	}
+	ret := makeMoveResults()
 
 	if len(stmts) == 0 {
 		return ret
@@ -71,25 +68,23 @@ func ApplyMoves(stmts []MoveStatement, state *states.State) MoveResults {
 	log.Printf("[TRACE] refactoring.ApplyMoves: Processing 'moved' statements in the configuration\n%s", logging.Indent(g.String()))
 
 	recordOldAddr := func(oldAddr, newAddr addrs.AbsResourceInstance) {
-		oldAddrKey := oldAddr.UniqueKey()
-		newAddrKey := newAddr.UniqueKey()
-		if prevMove, exists := ret.Changes[oldAddrKey]; exists {
+		if prevMove, exists := ret.Changes.GetOk(oldAddr); exists {
 			// If the old address was _already_ the result of a move then
 			// we'll replace that entry so that our results summarize a chain
 			// of moves into a single entry.
-			delete(ret.Changes, oldAddrKey)
+			ret.Changes.Remove(oldAddr)
 			oldAddr = prevMove.From
 		}
-		ret.Changes[newAddrKey] = MoveSuccess{
+		ret.Changes.Put(newAddr, MoveSuccess{
 			From: oldAddr,
 			To:   newAddr,
-		}
+		})
 	}
 	recordBlockage := func(newAddr, wantedAddr addrs.AbsMoveable) {
-		ret.Blocked[newAddr.UniqueKey()] = MoveBlocked{
+		ret.Blocked.Put(newAddr, MoveBlocked{
 			Wanted: wantedAddr,
 			Actual: newAddr,
-		}
+		})
 	}
 
 	g.ReverseDepthFirstWalk(startNodes, func(v dag.Vertex, depth int) error {
@@ -292,7 +287,7 @@ type MoveResults struct {
 	//
 	// In the return value from ApplyMoves, all of the keys are guaranteed to
 	// be unique keys derived from addrs.AbsResourceInstance values.
-	Changes map[addrs.UniqueKey]MoveSuccess
+	Changes addrs.Map[addrs.AbsResourceInstance, MoveSuccess]
 
 	// Blocked is a map from the unique keys of the final new
 	// resource instances addresses to information about where they "wanted"
@@ -308,7 +303,14 @@ type MoveResults struct {
 	//
 	// In the return value from ApplyMoves, all of the keys are guaranteed to
 	// be unique keys derived from values of addrs.AbsMoveable types.
-	Blocked map[addrs.UniqueKey]MoveBlocked
+	Blocked addrs.Map[addrs.AbsMoveable, MoveBlocked]
+}
+
+func makeMoveResults() MoveResults {
+	return MoveResults{
+		Changes: addrs.MakeMap[addrs.AbsResourceInstance, MoveSuccess](),
+		Blocked: addrs.MakeMap[addrs.AbsMoveable, MoveBlocked](),
+	}
 }
 
 type MoveSuccess struct {
@@ -327,15 +329,14 @@ type MoveBlocked struct {
 // If AddrMoved returns true, you can pass the same address to method OldAddr
 // to find its original address prior to moving.
 func (rs MoveResults) AddrMoved(newAddr addrs.AbsResourceInstance) bool {
-	_, ok := rs.Changes[newAddr.UniqueKey()]
-	return ok
+	return rs.Changes.Has(newAddr)
 }
 
 // OldAddr returns the old address of the given resource instance address, or
 // just returns back the same address if the given instance wasn't affected by
 // any move statements.
 func (rs MoveResults) OldAddr(newAddr addrs.AbsResourceInstance) addrs.AbsResourceInstance {
-	change, ok := rs.Changes[newAddr.UniqueKey()]
+	change, ok := rs.Changes.GetOk(newAddr)
 	if !ok {
 		return newAddr
 	}