cty: Fix panics with gob encoding of values containing numbers

We previously had a more localized workaround for this that worked only
for numbers directly, but it failed to deal with the same problem in
situations where the number is embedded inside some other compound type.

Now we'll apply the fix recursively over the whole raw decoded data
structure from gob, finding and fixing numbers inside all of the
structural and collection types too.

Although this fixup is mostly contained within the gob-handling functions,
it does bleed slightly into the set internals because otherwise we get
a chicken/egg problem where gob decoding of a set of numbers fails before
we get a chance to run the fixup.

The JSON and msgpack encodings of cty are the primary supported
serialization formats for the standard cty type system and I'd still
recommend using those instead of gob whenever possible, though as far as
I know gob encoding should now work as well as it can.

cty/gob.go

@@ -5,6 +5,8 @@ import (
 	"encoding/gob"
 	"fmt"
 	"math/big"
+
+	"github.com/zclconf/go-cty/cty/set"
 )
 
 // GobEncode is an implementation of the gob.GobEncoder interface, which
@@ -46,11 +48,12 @@ func (val *Value) GobDecode(buf []byte) error {
 		return fmt.Errorf("unsupported cty.Value encoding version %d; only 0 is supported", gv.Version)
 	}
 
-	// big.Float seems to, for some reason, lose its "pointerness" when we
-	// round-trip it, so we'll fix that here.
-	if bf, ok := gv.V.(big.Float); ok {
-		gv.V = &bf
-	}
+	// Because big.Float.GobEncode is implemented with a pointer reciever,
+	// gob encoding of an interface{} containing a *big.Float value does not
+	// round-trip correctly, emerging instead as a non-pointer big.Float.
+	// The rest of cty expects all number values to be represented by
+	// *big.Float, so we'll fix that up here.
+	gv.V = gobDecodeFixNumberPtr(gv.V, gv.Ty)
 
 	val.ty = gv.Ty
 	val.v = gv.V
@@ -123,3 +126,74 @@ type gobType struct {
 
 type gobCapsuleTypeImpl struct {
 }
+
+// goDecodeFixNumberPtr fixes an unfortunate quirk of round-tripping cty.Number
+// values through gob: the big.Float.GobEncode method is implemented on a
+// pointer receiver, and so it loses the "pointer-ness" of the value on
+// encode, causing the values to emerge the other end as big.Float rather than
+// *big.Float as we expect elsewhere in cty.
+//
+// The implementation of gobDecodeFixNumberPtr mutates the given raw value
+// during its work, and may either return the same value mutated or a new
+// value. Callers must no longer use whatever value they pass as "raw" after
+// this function is called.
+func gobDecodeFixNumberPtr(raw interface{}, ty Type) interface{} {
+	// Unfortunately we need to work recursively here because number values
+	// might be embedded in structural or collection type values.
+
+	switch {
+	case ty.Equals(Number):
+		if bf, ok := raw.(big.Float); ok {
+			return &bf // wrap in pointer
+		}
+	case ty.IsMapType() && ty.ElementType().Equals(Number):
+		if m, ok := raw.(map[string]interface{}); ok {
+			for k, v := range m {
+				m[k] = gobDecodeFixNumberPtr(v, ty.ElementType())
+			}
+		}
+	case ty.IsListType() && ty.ElementType().Equals(Number):
+		if s, ok := raw.([]interface{}); ok {
+			for i, v := range s {
+				s[i] = gobDecodeFixNumberPtr(v, ty.ElementType())
+			}
+		}
+	case ty.IsSetType() && ty.ElementType().Equals(Number):
+		if s, ok := raw.(set.Set); ok {
+			newS := set.NewSet(s.Rules())
+			for it := s.Iterator(); it.Next(); {
+				newV := gobDecodeFixNumberPtr(it.Value(), ty.ElementType())
+				newS.Add(newV)
+			}
+			return newS
+		}
+	case ty.IsObjectType():
+		if m, ok := raw.(map[string]interface{}); ok {
+			for k, v := range m {
+				aty := ty.AttributeType(k)
+				m[k] = gobDecodeFixNumberPtr(v, aty)
+			}
+		}
+	case ty.IsTupleType():
+		if s, ok := raw.([]interface{}); ok {
+			for i, v := range s {
+				ety := ty.TupleElementType(i)
+				s[i] = gobDecodeFixNumberPtr(v, ety)
+			}
+		}
+	}
+
+	return raw
+}
+
+// gobDecodeFixNumberPtrVal is a helper wrapper around gobDecodeFixNumberPtr
+// that works with already-constructed values. This is primarily for testing,
+// to fix up intentionally-invalid number values for the parts of the test
+// code that need them to be valid, such as calling GoString on them.
+func gobDecodeFixNumberPtrVal(v Value) Value {
+	raw := gobDecodeFixNumberPtr(v.v, v.ty)
+	return Value{
+		v:  raw,
+		ty: v.ty,
+	}
+}

cty/gob_test.go

@@ -18,6 +18,21 @@ func TestGobabilty(t *testing.T) {
 		SetVal([]Value{True}),
 		TupleVal([]Value{True}),
 		ObjectVal(map[string]Value{"true": True}),
+
+		// Numbers are particularly tricky because big.Float.GobEncode is
+		// implemented as a pointer method and thus big floats lose their
+		// "pointerness" during gob round-trip. For that reason, we're testing
+		// all of the containers with nested numbers inside to make sure that
+		// our fixup step is working correctly for all of them.
+		ListVal([]Value{NumberIntVal(1)}),
+		MapVal(map[string]Value{
+			"num": NumberIntVal(1),
+		}),
+		SetVal([]Value{NumberIntVal(1)}),
+		TupleVal([]Value{NumberIntVal(1)}),
+		ObjectVal(map[string]Value{
+			"num": NumberIntVal(1),
+		}),
 	}
 
 	for _, testValue := range tests {

cty/set_internals.go

@@ -147,6 +147,17 @@ func appendSetHashBytes(val Value, buf *bytes.Buffer) {
 
 	switch val.ty {
 	case Number:
+		// Due to an unfortunate quirk of gob encoding for big.Float, we end up
+		// with non-pointer values immediately after a gob round-trip, and
+		// we end up in here before we've had a chance to run
+		// gobDecodeFixNumberPtr on the inner values of a gob-encoded set,
+		// and so sadly we must make a special effort to handle that situation
+		// here just so that we can get far enough along to fix it up for
+		// everything else in this package.
+		if bf, ok := val.v.(big.Float); ok {
+			buf.WriteString(bf.String())
+			return
+		}
 		buf.WriteString(val.v.(*big.Float).String())
 		return
 	case Bool:

cty/set_internals_test.go

@@ -35,6 +35,20 @@ func TestSetHashBytes(t *testing.T) {
 			NumberVal(big.NewFloat(12)),
 			"12",
 		},
+		{
+			// This weird case is an intentionally-invalid number value that
+			// mimics the incorrect result of a gob round-trip of a cty.Number
+			// value. For more information, see the function
+			// gobDecodeFixNumberPtr. Unfortunately the set internals need to
+			// be tolerant of this situation because gob-decoding a set
+			// causes this situation to arise before we have had an opportunity
+			// to run gobDecodeFixNumberPtr yet.
+			Value{
+				ty: Number,
+				v:  *big.NewFloat(13),
+			},
+			"13",
+		},
 		{
 			StringVal(""),
 			`""`,
@@ -120,7 +134,7 @@ func TestSetHashBytes(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		t.Run(test.value.GoString(), func(t *testing.T) {
+		t.Run(gobDecodeFixNumberPtrVal(test.value).GoString(), func(t *testing.T) {
 			got := string(makeSetHashBytes(test.value))
 			if got != test.want {
 				t.Errorf("wrong result\ngot:  %s\nwant: %s", got, test.want)