Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set)

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## 0.12.0 (August 4, 2018)
+
+BREAKING CHANGE:
+ - Write empty (non-nil) struct pointers, unless (is list element and empty_elements isn't set) #206
+
 ## 0.11.1 (July 17, 2018)
 
 IMPROVEMENTS:

binary-decode.go

@@ -401,6 +401,7 @@ func (cdc *Codec) decodeReflectBinaryByteArray(bz []byte, info *TypeInfo, rv ref
 }
 
 // CONTRACT: rv.CanAddr() is true.
+// NOTE: Keep the code structure similar to decodeReflectBinarySlice.
 func (cdc *Codec) decodeReflectBinaryArray(bz []byte, info *TypeInfo, rv reflect.Value, fopts FieldOptions, bare bool) (n int, err error) {
 	if !rv.CanAddr() {
 		panic("rv not addressable")
@@ -461,6 +462,9 @@ func (cdc *Codec) decodeReflectBinaryArray(bz []byte, info *TypeInfo, rv reflect
 			return
 		}
 	} else {
+		// NOTE: ert is for the element value, while einfo.Type is dereferenced.
+		isErtStructPointer := ert.Kind() == reflect.Ptr && einfo.Type.Kind() == reflect.Struct
+
 		// Read elements in unpacked form.
 		for i := 0; i < length; i++ {
 			// Read field key (number and type).
@@ -480,8 +484,14 @@ func (cdc *Codec) decodeReflectBinaryArray(bz []byte, info *TypeInfo, rv reflect
 			}
 			// Decode the next ByteLength bytes into erv.
 			var erv = rv.Index(i)
-			// Special case if next ByteLength bytes are 0x00, set nil.
-			if len(bz) > 0 && bz[0] == 0x00 {
+			// Special case if:
+			//  * next ByteLength bytes are 0x00, and
+			//  * - erv is not a struct pointer, or
+			//    - field option doesn't have EmptyElements set
+			// (the condition below uses demorgan's law)
+			if (len(bz) > 0 && bz[0] == 0x00) &&
+				!(isErtStructPointer && fopts.EmptyElements) {
+
 				slide(&bz, &n, 1)
 				erv.Set(defaultValue(erv.Type()))
 				continue
@@ -547,6 +557,7 @@ func (cdc *Codec) decodeReflectBinaryByteSlice(bz []byte, info *TypeInfo, rv ref
 }
 
 // CONTRACT: rv.CanAddr() is true.
+// NOTE: Keep the code structure similar to decodeReflectBinaryArray.
 func (cdc *Codec) decodeReflectBinarySlice(bz []byte, info *TypeInfo, rv reflect.Value, fopts FieldOptions, bare bool) (n int, err error) {
 	if !rv.CanAddr() {
 		panic("rv not addressable")
@@ -611,6 +622,9 @@ func (cdc *Codec) decodeReflectBinarySlice(bz []byte, info *TypeInfo, rv reflect
 			srv = reflect.Append(srv, erv)
 		}
 	} else {
+		// NOTE: ert is for the element value, while einfo.Type is dereferenced.
+		isErtStructPointer := ert.Kind() == reflect.Ptr && einfo.Type.Kind() == reflect.Struct
+
 		// Read elements in unpacked form.
 		for {
 			if len(bz) == 0 {
@@ -636,8 +650,14 @@ func (cdc *Codec) decodeReflectBinarySlice(bz []byte, info *TypeInfo, rv reflect
 			}
 			// Decode the next ByteLength bytes into erv.
 			erv, _n := reflect.New(ert).Elem(), int(0)
-			// Special case if next ByteLength bytes are 0x00, set nil.
-			if len(bz) > 0 && bz[0] == 0x00 {
+			// Special case if:
+			//  * next ByteLength bytes are 0x00, and
+			//  * - erv is not a struct pointer, or
+			//    - field option doesn't have EmptyElements set
+			// (the condition below uses demorgan's law)
+			if (len(bz) > 0 && bz[0] == 0x00) &&
+				!(isErtStructPointer && fopts.EmptyElements) {
+
 				slide(&bz, &n, 1)
 				erv.Set(defaultValue(erv.Type()))
 				srv = reflect.Append(srv, erv)

binary-encode.go

@@ -297,6 +297,9 @@ func (cdc *Codec) encodeReflectBinaryList(w io.Writer, info *TypeInfo, rv reflec
 			}
 		}
 	} else {
+		// NOTE: ert is for the element value, while einfo.Type is dereferenced.
+		isErtStructPointer := ert.Kind() == reflect.Ptr && einfo.Type.Kind() == reflect.Struct
+
 		// Write elems in unpacked form.
 		for i := 0; i < rv.Len(); i++ {
 			// Write elements as repeated fields of the parent struct.
@@ -307,6 +310,17 @@ func (cdc *Codec) encodeReflectBinaryList(w io.Writer, info *TypeInfo, rv reflec
 			// Get dereferenced element value and info.
 			var erv, isDefault = isDefaultValue(rv.Index(i))
 			if isDefault {
+				// Special case if:
+				//  - erv is a struct pointer and
+				//  - field option has EmptyElements set
+				if isErtStructPointer && fopts.EmptyElements {
+					// NOTE: Not sure what to do here, but for future-proofing,
+					// we explicitly fail on nil pointers, just like
+					// Proto3's Golang client does.
+					// This also makes it easier to upgrade to Amino2
+					// which would enable the encoding of nil structs.
+					return errors.New("nil struct pointers not supported when empty_elements field tag is set")
+				}
 				// Nothing to encode, so the length is 0.
 				err = EncodeByte(buf, byte(0x00))
 				if err != nil {
@@ -384,14 +398,17 @@ func (cdc *Codec) encodeReflectBinaryStruct(w io.Writer, info *TypeInfo, rv refl
 				return
 			}
 			// Get dereferenced field value and info.
-			var frv, isDefault = isDefaultValue(rv.Field(field.Index))
+			var frv = rv.Field(field.Index)
+			var frvIsPtr = frv.Kind() == reflect.Ptr
+			var dfrv, isDefault = isDefaultValue(frv)
 			if isDefault && !fopts.WriteEmpty {
-				// Do not encode default value fields (only if `amino:"write_empty"` is set).
+				// Do not encode default value fields
+				// (except when `amino:"write_empty"` is set).
 				continue
 			}
 			if field.UnpackedList {
 				// Write repeated field entries for each list item.
-				err = cdc.encodeReflectBinaryList(buf, finfo, frv, field.FieldOptions, true)
+				err = cdc.encodeReflectBinaryList(buf, finfo, dfrv, field.FieldOptions, true)
 				if err != nil {
 					return
 				}
@@ -405,14 +422,15 @@ func (cdc *Codec) encodeReflectBinaryStruct(w io.Writer, info *TypeInfo, rv refl
 				lBeforeValue := buf.Len()
 
 				// Write field value from rv.
-				err = cdc.encodeReflectBinary(buf, finfo, frv, field.FieldOptions, false)
+				err = cdc.encodeReflectBinary(buf, finfo, dfrv, field.FieldOptions, false)
 				if err != nil {
 					return
 				}
 				lAfterValue := buf.Len()
 
-				if !fopts.WriteEmpty && lBeforeValue == lAfterValue-1 && buf.Bytes()[buf.Len()-1] == 0x00 {
-					// rollback typ3/fieldnum and last byte if empty:
+				if !frvIsPtr && !fopts.WriteEmpty && lBeforeValue == lAfterValue-1 && buf.Bytes()[buf.Len()-1] == 0x00 {
+					// rollback typ3/fieldnum and last byte if
+					// not a pointer and empty:
 					buf.Truncate(lBeforeKey)
 				}
 

binary_test.go

@@ -174,3 +174,72 @@ func TestStructSlice(t *testing.T) {
 	cdc.UnmarshalBinaryBare(bz, &f2)
 	assert.Equal(t, f, f2)
 }
+
+func TestStructPointerSlice1(t *testing.T) {
+	cdc := amino.NewCodec()
+
+	type Foo struct {
+		A string
+		B int
+		C []*Foo
+		D string // exposed
+	}
+
+	var f = Foo{
+		A: "k",
+		B: 2,
+		C: []*Foo{nil, nil, nil},
+		D: "j",
+	}
+	bz, err := cdc.MarshalBinary(f)
+	assert.NoError(t, err)
+
+	var f2 Foo
+	err = cdc.UnmarshalBinary(bz, &f2)
+	assert.Nil(t, err)
+
+	assert.Equal(t, f, f2)
+	assert.Nil(t, f2.C[0])
+
+	var f3 = Foo{
+		A: "k",
+		B: 2,
+		C: []*Foo{&Foo{}, &Foo{}, &Foo{}},
+		D: "j",
+	}
+	bz2, err := cdc.MarshalBinary(f3)
+	assert.NoError(t, err)
+	assert.Equal(t, bz, bz2, "empty slices should be decoded to nil unless empty_elements")
+}
+
+// Like TestStructPointerSlice2, but with EmptyElements.
+func TestStructPointerSlice2(t *testing.T) {
+	cdc := amino.NewCodec()
+
+	type Foo struct {
+		A string
+		B int
+		C []*Foo `amino:"empty_elements"`
+		D string // exposed
+	}
+
+	var f = Foo{
+		A: "k",
+		B: 2,
+		C: []*Foo{nil, nil, nil},
+		D: "j",
+	}
+	bz, err := cdc.MarshalBinary(f)
+	assert.Error(t, err, "nil elements of a slice/array not supported when empty_elements field tag set.")
+
+	f.C = []*Foo{&Foo{}, &Foo{}, &Foo{}}
+	bz, err = cdc.MarshalBinary(f)
+	assert.NoError(t, err)
+
+	var f2 Foo
+	err = cdc.UnmarshalBinary(bz, &f2)
+	assert.Nil(t, err)
+
+	assert.Equal(t, f, f2)
+	assert.NotNil(t, f2.C[0])
+}

codec.go

@@ -116,8 +116,10 @@ type FieldOptions struct {
 	BinFixed64    bool   // (Binary) Encode as fixed64
 	BinFixed32    bool   // (Binary) Encode as fixed32
 	BinFieldNum   uint32 // (Binary) max 1<<29-1
-	Unsafe        bool   // e.g. if this field is a float.
-	WriteEmpty    bool   // write empty structs, default is to skip them
+
+	Unsafe        bool // e.g. if this field is a float.
+	WriteEmpty    bool // write empty structs and lists (default false except for pointers)
+	EmptyElements bool // Slice and Array elements are never nil, decode 0x00 as empty struct.
 }
 
 //----------------------------------------
@@ -514,6 +516,9 @@ func (cdc *Codec) parseFieldOptions(field reflect.StructField) (skip bool, fopts
 		if aminoTag == "write_empty" {
 			fopts.WriteEmpty = true
 		}
+		if aminoTag == "empty_elements" {
+			fopts.EmptyElements = true
+		}
 	}
 
 	return

reflect.go

@@ -96,10 +96,12 @@ func derefPointersZero(rv reflect.Value) (drv reflect.Value, isPtr bool, isNilPt
 	return
 }
 
-// Returns isDefaultValue=true iff is ultimately nil or empty after (recursive)
-// dereferencing. If isDefaultValue=false, erv is set to the non-nil non-empty
-// non-default dereferenced value.
-// A zero/empty struct is not considered default for this function.
+// Returns isDefaultValue=true iff is ultimately nil or empty
+// after (recursive) dereferencing.
+// If isDefaultValue=false, erv is set to the non-nil non-default
+// dereferenced value.
+// A zero/empty struct is not considered default for this
+// function.
 func isDefaultValue(rv reflect.Value) (erv reflect.Value, isDefaultValue bool) {
 	rv, _, isNilPtr := derefPointers(rv)
 	if isNilPtr {

reflect_test.go

@@ -524,6 +524,21 @@ var fuzzFuncs = []interface{}{
 		// Also set to UTC.
 		*tyme = tyme.Truncate(0).UTC()
 	},
+	func(esz *[]*tests.EmptyStruct, c fuzz.Continue) {
+		n := c.Intn(4)
+		switch n {
+		case 0:
+			// Prefer nil over empty slice.
+			*esz = nil
+		default:
+			// Slice of empty struct pointers should be nil,
+			// since we don't set amino:"empty_elements".
+			*esz = make([]*tests.EmptyStruct, n)
+			for i := 0; i < n; i++ {
+				(*esz)[i] = nil
+			}
+		}
+	},
 }
 
 //----------------------------------------

tests/common.go

@@ -5,6 +5,9 @@ import "time"
 //----------------------------------------
 // Struct types
 
+type EmptyStruct struct {
+}
+
 type PrimitivesStruct struct {
 	Int8    int8
 	Int16   int16
@@ -22,6 +25,7 @@ type PrimitivesStruct struct {
 	String  string
 	Bytes   []byte
 	Time    time.Time
+	Empty   EmptyStruct
 }
 
 type ShortArraysStruct struct {
@@ -45,6 +49,7 @@ type ArraysStruct struct {
 	StringAr  [4]string
 	BytesAr   [4][]byte
 	TimeAr    [4]time.Time
+	EmptyAr   [4]EmptyStruct
 }
 
 type SlicesStruct struct {
@@ -64,6 +69,7 @@ type SlicesStruct struct {
 	StringSl  []string
 	BytesSl   [][]byte
 	TimeSl    []time.Time
+	EmptySl   []EmptyStruct
 }
 
 type SliceSlicesStruct struct {
@@ -83,6 +89,7 @@ type SliceSlicesStruct struct {
 	StringSlSl  [][]string
 	BytesSlSl   [][][]byte
 	TimeSlSl    [][]time.Time
+	EmptySlSl   [][]EmptyStruct
 }
 
 type PointersStruct struct {
@@ -102,6 +109,7 @@ type PointersStruct struct {
 	StringPt  *string
 	BytesPt   *[]byte
 	TimePt    *time.Time
+	EmptyPt   *EmptyStruct
 }
 
 type PointerSlicesStruct struct {
@@ -121,6 +129,7 @@ type PointerSlicesStruct struct {
 	StringPtSl  []*string
 	BytesPtSl   []*[]byte
 	TimePtSl    []*time.Time
+	EmptyPtSl   []*EmptyStruct
 }
 
 // NOTE: See registered fuzz funcs for *byte, **byte, and ***byte.
@@ -153,6 +162,7 @@ type EmbeddedSt3 struct {
 	*ArraysStruct
 	*SlicesStruct
 	*PointersStruct
+	*EmptyStruct
 }
 
 type EmbeddedSt4 struct {
@@ -180,6 +190,7 @@ type EmbeddedSt5 struct {
 }
 
 var StructTypes = []interface{}{
+	(*EmptyStruct)(nil),
 	(*PrimitivesStruct)(nil),
 	(*ShortArraysStruct)(nil),
 	(*ArraysStruct)(nil),