marshal: track omitempty state for the entire index path for embedded…

… structs

Embedded struct fields are recorded in `fieldInfo` by taking the outer
structure's field indices and concatenating the field indices for every
field inside the embedded one.

Given the example structs,

```go
struct Outer {
        S string
        *Inner   `plist:",omitempty"`
}

struct Inner {
        S2 string
}
```

We record the indices...

```
Outer.S  at [0]
Outer.S2 at [1, 0] // Outer.Inner in index 1, Inner.S2 at 0
```

Because we elided the `typeInfo` for `Outer.Inner`, there was nowhere to
store its `omitempty`. We would not want to propagate `omitempty` down
to every child, because that would not represent reality.

This commit introduces tracking for each index in the index path whether
omitempty was set at that level.

Now we understand `Outer.S2` like this:

```
             + Omit if Outer.Inner is empty
             |
Outer.S2 at [1, 0]
```

I chose to use a 64-bit bitfield to record index indices, so if you nest
64 anonymous embedded structs plist will forget the innermost omitempty
flags.

To accomplish this with minimal changes, I split `fieldInfo.value` into
`value` and `valueForWriting`. `value` no longer populates nil pointers
and interfaces, and is safe for reading. `valueForWriting` doesn't care
about `omitempty`, and does populate nil pointers in the destination.

Unfortunately, plist will still populate empty structures for nested
anonymous `omitempty` pointers-to-structs during unmarshal.

Closes #74

common_data_for_test.go

@@ -981,6 +981,83 @@ var tests = []TestData{
 			GNUStepFormat: []byte(`{Intppp=<*I3>;}`),
 		},
 	},
+	{
+		Name: "Embedded fields within a nil omitempty member",
+		Value: struct {
+			*InnerStructWithSimpleField `plist:",omitempty"`
+		}{},
+		Documents: map[int][]byte{
+			GNUStepFormat: []byte(`{}`),
+		},
+		// TODO: right now we populate the inner struct even though we do not fill it
+		SkipDecode: map[int]bool{GNUStepFormat: true},
+	},
+	{
+		Name: "Embedded fields within a nil omitempty member, telescoping",
+		Value: struct {
+			O                      string `plist:",omitempty"`
+			*InnerStructEmbedNest1 `plist:",omitempty"`
+		}{O: "sentinel"},
+		Documents: map[int][]byte{
+			GNUStepFormat: []byte(`{O=sentinel;}`),
+		},
+		// TODO: right now we populate the inner struct even though we do not fill it
+		SkipDecode: map[int]bool{GNUStepFormat: true},
+	},
+	{
+		Name: "Embedded fields within a nil omitempty member, telescoping, 1",
+		Value: struct {
+			O                      string `plist:",omitempty"`
+			*InnerStructEmbedNest1 `plist:",omitempty"`
+		}{
+			O:                     "sentinel",
+			InnerStructEmbedNest1: &InnerStructEmbedNest1{One: "one"},
+		},
+		Documents: map[int][]byte{
+			GNUStepFormat: []byte(`{O=sentinel;One=one;}`),
+		},
+		// TODO: right now we populate the inner struct even though we do not fill it
+		SkipDecode: map[int]bool{GNUStepFormat: true},
+	},
+	{
+		Name: "Embedded fields within a nil omitempty member, telescoping, 2",
+		Value: struct {
+			O                      string `plist:",omitempty"`
+			*InnerStructEmbedNest1 `plist:",omitempty"`
+		}{
+			O: "sentinel",
+			InnerStructEmbedNest1: &InnerStructEmbedNest1{
+				One: "one",
+				InnerStructEmbedNest2: &InnerStructEmbedNest2{
+					Two: "two",
+				},
+			},
+		},
+		Documents: map[int][]byte{
+			GNUStepFormat: []byte(`{O=sentinel;One=one;Two=two;}`),
+		},
+		// TODO: right now we populate the inner struct even though we do not fill it
+		SkipDecode: map[int]bool{GNUStepFormat: true},
+	},
+	{
+		Name: "Embedded fields within a nil omitempty member, telescoping, non-nil-but-non-empty",
+		Value: struct {
+			O                      string `plist:",omitempty"`
+			*InnerStructEmbedNest1 `plist:",omitempty"`
+		}{
+			O: "sentinel",
+			InnerStructEmbedNest1: &InnerStructEmbedNest1{
+				InnerStructEmbedNest2: &InnerStructEmbedNest2{
+					InnerStructEmbedNest3: &InnerStructEmbedNest3{},
+				},
+			},
+		},
+		Documents: map[int][]byte{
+			GNUStepFormat: []byte(`{O=sentinel;One="";Three="";Two="";}`),
+		},
+		// TODO: right now we populate the inner struct even though we do not fill it
+		SkipDecode: map[int]bool{GNUStepFormat: true},
+	},
 }
 
 type StructWithDeeplyNestedPointer struct {
@@ -992,6 +1069,25 @@ var nestedPtrIntValp = &nestedPtrIntVal
 var nestedPtrIntValpp = &nestedPtrIntValp
 var nestedPtrIntValppp = &nestedPtrIntValpp
 
+type InnerStructWithSimpleField struct {
+	S string
+}
+
+type InnerStructEmbedNest1 struct {
+	One                    string
+	*InnerStructEmbedNest2 `plist:",omitempty"`
+}
+
+type InnerStructEmbedNest2 struct {
+	Two                    string
+	*InnerStructEmbedNest3 `plist:",omitempty"`
+}
+
+type InnerStructEmbedNest3 struct {
+	Three                       string
+	*InnerStructWithSimpleField `plist:",omitempty"`
+}
+
 type EverythingTestData struct {
 	Intarray []uint64  `plist:"intarray"`
 	Floats   []float64 `plist:"floats"`

marshal.go

@@ -6,24 +6,6 @@ import (
 	"time"
 )
 
-func isEmptyValue(v reflect.Value) bool {
-	switch v.Kind() {
-	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
-		return v.Len() == 0
-	case reflect.Bool:
-		return !v.Bool()
-	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-		return v.Int() == 0
-	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
-		return v.Uint() == 0
-	case reflect.Float32, reflect.Float64:
-		return v.Float() == 0
-	case reflect.Interface, reflect.Ptr:
-		return v.IsNil()
-	}
-	return false
-}
-
 var (
 	plistMarshalerType = reflect.TypeOf((*Marshaler)(nil)).Elem()
 	textMarshalerType  = reflect.TypeOf((*encoding.TextMarshaler)(nil)).Elem()
@@ -76,7 +58,7 @@ func (p *Encoder) marshalStruct(typ reflect.Type, val reflect.Value) cfValue {
 	}
 	for _, finfo := range tinfo.fields {
 		value := finfo.value(val)
-		if !value.IsValid() || finfo.omitEmpty && isEmptyValue(value) {
+		if !value.IsValid() {
 			continue
 		}
 		dict.keys = append(dict.keys, finfo.name)

typeinfo.go

@@ -6,16 +6,47 @@ import (
 	"sync"
 )
 
+func isEmptyValue(v reflect.Value) bool {
+	switch v.Kind() {
+	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
+		return v.Len() == 0
+	case reflect.Bool:
+		return !v.Bool()
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		return v.Int() == 0
+	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+		return v.Uint() == 0
+	case reflect.Float32, reflect.Float64:
+		return v.Float() == 0
+	case reflect.Interface, reflect.Ptr:
+		return v.IsNil()
+	}
+	return false
+}
+
 // typeInfo holds details for the plist representation of a type.
 type typeInfo struct {
 	fields []fieldInfo
 }
 
 // fieldInfo holds details for the plist representation of a single field.
 type fieldInfo struct {
-	idx       []int
-	name      string
-	omitEmpty bool
+	idx  []int
+	name string
+
+	// omitEmptyDepthMap stores, for each entry in idx, whether at that level the user had specified
+	// omitempty. This matters for anonymous embedded structs, where the index path to a given field
+	// may traverse different struct types
+	//
+	// For example, given struct S{ *I } and struct I{ V int }, the path to V is [0,0].
+	// If S.I were marked `omitempty`, we would need to record that omitempty was seen at the first index entry.
+	//    *I,         int
+	// [   0,           0 ]
+	// [omit, do not omit ]
+	//
+	// As an optimization, we store it as a bit field. This means anonymous embedded structs more than 64 entries
+	// may forget their omitempty states.
+	omitEmptyDepthMap uint64
 }
 
 var tinfoMap = make(map[reflect.Type]*typeInfo)
@@ -39,6 +70,11 @@ func getTypeInfo(typ reflect.Type) (*typeInfo, error) {
 				continue // Private field
 			}
 
+			finfo, err := structFieldInfo(typ, &f)
+			if err != nil {
+				return nil, err
+			}
+
 			// For embedded structs, embed its fields.
 			if f.Anonymous {
 				t := f.Type
@@ -50,21 +86,17 @@ func getTypeInfo(typ reflect.Type) (*typeInfo, error) {
 					if err != nil {
 						return nil, err
 					}
-					for _, finfo := range inner.fields {
-						finfo.idx = append([]int{i}, finfo.idx...)
-						if err := addFieldInfo(typ, tinfo, &finfo); err != nil {
+					for _, innerFinfo := range inner.fields {
+						innerFinfo.idx = append(finfo.idx, innerFinfo.idx...)
+						innerFinfo.omitEmptyDepthMap = finfo.omitEmptyDepthMap | (innerFinfo.omitEmptyDepthMap << uint(len(finfo.idx)))
+						if err := addFieldInfo(typ, tinfo, &innerFinfo); err != nil {
 							return nil, err
 						}
 					}
 					continue
 				}
 			}
 
-			finfo, err := structFieldInfo(typ, &f)
-			if err != nil {
-				return nil, err
-			}
-
 			// Add the field if it doesn't conflict with other fields.
 			if err := addFieldInfo(typ, tinfo, finfo); err != nil {
 				return nil, err
@@ -92,7 +124,7 @@ func structFieldInfo(typ reflect.Type, f *reflect.StructField) (*fieldInfo, erro
 		for _, flag := range tokens[1:] {
 			switch flag {
 			case "omitempty":
-				finfo.omitEmpty = true
+				finfo.omitEmptyDepthMap = 1 << uint(len(f.Index)-1)
 			}
 		}
 	}
@@ -150,10 +182,10 @@ func addFieldInfo(typ reflect.Type, tinfo *typeInfo, newf *fieldInfo) error {
 	return nil
 }
 
-// value returns v's field value corresponding to finfo.
+// valueForWriting returns v's field value corresponding to finfo.
 // It's equivalent to v.FieldByIndex(finfo.idx), but initializes
 // and dereferences pointers as necessary.
-func (finfo *fieldInfo) value(v reflect.Value) reflect.Value {
+func (finfo *fieldInfo) valueForWriting(v reflect.Value) reflect.Value {
 	for i, x := range finfo.idx {
 		if i > 0 {
 			t := v.Type()
@@ -168,3 +200,22 @@ func (finfo *fieldInfo) value(v reflect.Value) reflect.Value {
 	}
 	return v
 }
+
+// valueForWriting returns v's field value corresponding to finfo.
+// It's equivalent to v.FieldByIndex(finfo.idx), but bails out if one of the
+// indices indicated that it should be omitted if it's empty and it is empty.
+func (finfo *fieldInfo) value(v reflect.Value) reflect.Value {
+	for i, x := range finfo.idx {
+		t := v.Type()
+		if t.Kind() == reflect.Ptr && t.Elem().Kind() == reflect.Struct {
+			v = v.Elem()
+		}
+
+		v = v.Field(x)
+
+		if (finfo.omitEmptyDepthMap&(1<<uint(i))) != 0 && isEmptyValue(v) {
+			return reflect.Value{}
+		}
+	}
+	return v
+}

unmarshal.go

@@ -260,7 +260,7 @@ func (p *Decoder) unmarshalDictionary(dict *cfDictionary, val reflect.Value) {
 		}
 
 		for _, finfo := range tinfo.fields {
-			p.unmarshal(entries[finfo.name], finfo.value(val))
+			p.unmarshal(entries[finfo.name], finfo.valueForWriting(val))
 		}
 	case reflect.Map:
 		if val.IsNil() {