WIP: produce a valid plist or error message if MarshalPlist returns nil

Refs #84

marshal.go

@@ -2,6 +2,7 @@ package plist
 
 import (
 	"encoding"
+	"fmt"
 	"reflect"
 	"time"
 )
@@ -57,10 +58,19 @@ func (p *Encoder) marshalStruct(typ reflect.Type, val reflect.Value) cfValue {
 		values: make([]cfValue, 0, len(tinfo.fields)),
 	}
 	for _, finfo := range tinfo.fields {
-		value := finfo.value(val)
+		value, omitIfEmpty := finfo.value(val)
 		if !value.IsValid() {
 			continue
 		}
+
+		nv := p.marshal(value)
+		if nv == nil {
+			if !omitIfEmpty {
+				panic(fmt.Errorf("plist: marshaled type `%v` produced no value, but omitifempty was not specified on `%v.%v`", value.Type(), typ, finfo.name))
+			}
+			continue
+		}
+
 		dict.keys = append(dict.keys, finfo.name)
 		dict.values = append(dict.values, p.marshal(value))
 	}

marshal_test.go

@@ -156,3 +156,132 @@ func TestInterfaceGeneralSliceMarshal(t *testing.T) {
 		t.Error("expect non-zero data")
 	}
 }
+
+type CustomMarshaler struct {
+	value interface{}
+}
+
+var _ Marshaler = (*CustomMarshaler)(nil)
+
+func (c *CustomMarshaler) MarshalPlist() (interface{}, error) {
+	// There are valid cases for testing *(nil).MarshalPlist, so don't blow up here.
+	if c == nil {
+		return nil, nil
+	}
+	return c.value, nil
+}
+
+func TestPlistMarshalerNil(t *testing.T) {
+	// Direct non-nil value encodes
+	subtest(t, "string", func(t *testing.T) {
+		c := &CustomMarshaler{value: "hello world"}
+		b, err := Marshal(c, XMLFormat)
+		if err != nil {
+			t.Error(err)
+		}
+		if len(b) == 0 {
+			t.Error("expect non-nil")
+		}
+
+		t.Log(string(b))
+		// <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+		// <plist version="1.0"><string>hello world</string></plist>
+	})
+
+	// Direct nil value correctly returns an error
+	subtest(t, "nil", func(t *testing.T) {
+		c := &CustomMarshaler{}
+		b, err := Marshal(c, XMLFormat)
+		if err == nil {
+			t.Error("expect error")
+		} else {
+			t.Log(err)
+		}
+		if len(b) != 0 {
+			t.Error("expect nil")
+		}
+	})
+
+	// Field nil value with omitempty correctly omitted
+	subtest(t, "ptr-omitempty", func(t *testing.T) {
+		type Structure struct {
+			C *CustomMarshaler `plist:"C,omitempty"`
+		}
+		s := &Structure{}
+		b, err := Marshal(s, XMLFormat)
+		if err != nil {
+			t.Error(err)
+		}
+		if len(b) == 0 {
+			t.Error("expect non-nil")
+		}
+		t.Log(string(b))
+
+		// <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+		// <plist version="1.0"><dict></dict></plist>
+	})
+
+	// Non-nil field returning marshaler nil value with omitempty should be omitted
+	subtest(t, "omitempty", func(t *testing.T) {
+		type Structure struct {
+			C CustomMarshaler `plist:"C,omitempty"`
+		}
+		s := &Structure{}
+		b, err := Marshal(s, XMLFormat)
+		if err != nil {
+			t.Error(err)
+		}
+		if len(b) == 0 {
+			t.Error("expect non-nil")
+		}
+		t.Log(string(b))
+
+		// Unmarshal to prove malformed encoding
+		var dst Structure
+		if _, err := Unmarshal(b, &dst); err != nil {
+			t.Error(err) // plist: error parsing XML property list: missing value in dictionary
+		}
+
+		// Get key without value and no error:
+		// <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+		// <plist version="1.0"><dict><key>C</key></dict></plist>
+
+		// Expect:
+		// <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+		// <plist version="1.0"><dict></dict></plist>
+	})
+
+	// Field nil value without omitempty correctly emits error
+	subtest(t, "ptr", func(t *testing.T) {
+		type Structure struct {
+			C *CustomMarshaler
+		}
+		s := &Structure{}
+		b, err := Marshal(s, XMLFormat)
+		if err == nil {
+			t.Error("expect error")
+		} else {
+			t.Log(err)
+		}
+		if len(b) != 0 {
+			t.Error("expect nil")
+		}
+	})
+
+	// Non-nil field returning marshaler nil value without omitempty should emit an error
+	subtest(t, "direct-nil-member", func(t *testing.T) {
+		type Structure struct {
+			C CustomMarshaler
+		}
+		s := &Structure{}
+		b, err := Marshal(s, XMLFormat)
+		if err == nil {
+			t.Error("expect error")
+		} else {
+			t.Log(err)
+		}
+		if len(b) != 0 {
+			t.Error("expect nil")
+		}
+	})
+}

typeinfo.go

@@ -201,10 +201,12 @@ func (finfo *fieldInfo) valueForWriting(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 {
+// value returns v's field value corresponding to finfo, as well as whether it
+// can be omitted if empty 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, bool) {
+	lastOmitIfEmpty := false
 	for i, x := range finfo.idx {
 		t := v.Type()
 		if t.Kind() == reflect.Ptr && t.Elem().Kind() == reflect.Struct {
@@ -213,9 +215,10 @@ func (finfo *fieldInfo) value(v reflect.Value) reflect.Value {
 
 		v = v.Field(x)
 
-		if (finfo.omitEmptyDepthMap&(1<<uint(i))) != 0 && isEmptyValue(v) {
-			return reflect.Value{}
+		lastOmitIfEmpty = (finfo.omitEmptyDepthMap & (1 << uint(i))) != 0
+		if lastOmitIfEmpty && isEmptyValue(v) {
+			return reflect.Value{}, lastOmitIfEmpty
 		}
 	}
-	return v
+	return v, lastOmitIfEmpty
 }