Marshaler returning nil creates malformed output

[grahammiln]

If a value implementing the Marshaler interface returns a nil replacement value, the resulting encoding is malformed. The value's key is encoded but the value is silently skipped:

<plist version="1.0">
	<dict>
		<key>C</key>
	</dict>
</plist>

The correct behaviour is to return an encoding error. The exception is if the non-nil Marshaler value has the omitempty structure tag and it returns a replacement of nil, then the entire key value pair should be omitted:

type S struct {
	C CustomMarshaler `plist:"C,omitempty"`
}

Below are some tests to help isolate the problem:

type CustomMarshaler struct {
	value interface{}
}

var _ plist.Marshaler = (*CustomMarshaler)(nil)

func (c *CustomMarshaler) MarshalPlist() (interface{}, error) {
	return c.value, nil
}

func TestPlistMarshalerNil(t *testing.T) {

	// Direct non-nil value encodes
	t.Run("string", func(t *testing.T) {
		c := &CustomMarshaler{value: "hello world"}
		b, err := plist.Marshal(c, plist.XMLFormat)
		if err != nil {
			t.Error(err)
		}
		if len(b) == 0 {
			t.Error("expect non-nil")
		}

		// <!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
	t.Run("nil", func(t *testing.T) {
		c := &CustomMarshaler{}
		b, err := plist.Marshal(c, plist.XMLFormat)
		if err == nil {
			t.Error("expect error")
		}
		if len(b) != 0 {
			t.Error("expect nil")
		}
	})

	// Field nil value with omitempty correctly omitted
	t.Run("ptr-omitempty", func(t *testing.T) {
		type Structure struct {
			C *CustomMarshaler `plist:"C,omitempty"`
		}
		s := &Structure{}
		b, err := plist.Marshal(s, plist.XMLFormat)
		if err != nil {
			t.Error(err)
		}
		if len(b) == 0 {
			t.Error("expect non-nil")
		}

		// <!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
	t.Run("omitempty", func(t *testing.T) {
		type Structure struct {
			C CustomMarshaler `plist:"C,omitempty"`
		}
		s := &Structure{}
		b, err := plist.Marshal(s, plist.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 := plist.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>
	})
}

[DHowett]

I took a crack at this in 6650182, but I am unsure whether it is correct. For now, I'm keeping it on a branch.

=== RUN   TestPlistMarshalerNil
=== RUN   TestPlistMarshalerNil/string
    marshal_test.go:186: <?xml version="1.0" encoding="UTF-8"?>
        <!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>
=== RUN   TestPlistMarshalerNil/nil
    marshal_test.go:198: plist: no root element to encode
=== RUN   TestPlistMarshalerNil/ptr-omitempty
    marshal_test.go:218: <?xml version="1.0" encoding="UTF-8"?>
        <!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>
=== RUN   TestPlistMarshalerNil/omitempty
    marshal_test.go:237: <?xml version="1.0" encoding="UTF-8"?>
        <!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>
=== RUN   TestPlistMarshalerNil/ptr
    marshal_test.go:264: plist: marshaled type `*plist.CustomMarshaler` produced no value, but omitifempty was not specified on `plist.Structure.C`
=== RUN   TestPlistMarshalerNil/direct-nil-member
    marshal_test.go:281: plist: marshaled type `plist.CustomMarshaler` produced no value, but omitifempty was not specified on `plist.Structure.C`

I added a couple test cases to ensure that we exercised the new case (not omitted, could not be marshaled by custom marshaler).

How do you feel about that error description? I may add an indication that it is developer error to return nil but no err...