Encoding: 'omitempty' will panic if the struct contains a uncomparable type

[ybaldus]

While encoding a struct will be checked for emptiness with the isEmpty function:

func isEmpty(rv reflect.Value) bool {
	switch rv.Kind() {
	case reflect.Array, reflect.Slice, reflect.Map, reflect.String:
		return rv.Len() == 0
	case reflect.Struct:
		return reflect.Zero(rv.Type()).Interface() == rv.Interface()
	case reflect.Bool:
		return !rv.Bool()
	}
	return false
}

The comparing of structs panics if the structs contains any uncomparable type.

Minimal example:

type a struct {
	B b `toml:"B,omitempty"`
}

type b struct {
	C []string `toml:"C,omitempty"`
}

func TestToml(t *testing.T) {
	b1 := b{C: []string{"foo"}}
	a1 := a{B: b1}
	var buf bytes.Buffer
	toml.NewEncoder(&buf).Encode(a1)
	fmt.Printf("%s", buf.String())
}

Output:

=== RUN   TestToml
--- FAIL: TestToml (0.00s)
panic: runtime error: comparing uncomparable type data.b [recovered]
	panic: runtime error: comparing uncomparable type data.b [recovered]
	panic: runtime error: comparing uncomparable type data.b

[arp242]

Thanks for the reproducible example; this should be fixed by #361: it will return an error now instead.

[ybaldus]

Thanks for the fast fix. Is it correct to return an error here? Atleast that would be a breaking change because with 1.1.0 it was possible to encode those structs. Now everyone has to check every struct for compareable types before useing an omitempty flag.

[arp242]

Wouldn't it be surprising if you add omitempty and it silently gets ignored? The way I imagined it, is that the omitempty should be removed from your struct, rather than doing a IsComparable check in your code.

Breaking compatibility of course isn't great, but it seems to me that adding omitempty here is essentially a bug in your code.

We could ignore it, or maybe add a Strict option or something.

I'll reopen this issue for now to discus.

[arp242]

Would probably be best to see how encoding/{json,xml} behave for this, and then copy that. I typically try to follow the stdlib encoding's lead unless there's a good reason not to, as it avoids confusion.

[ybaldus]

Maybe you understood me wrong but I wouldn't ignore it silently. I wanted to say that it should be checked field by field if the struct is uncomparable instead of returning an error. I'm pretty sure the stdlib does it that way. I'm not sure about the best/efficient way to check if a struct is empty.

https://cs.opensource.google/go/go/+/master:src/encoding/json/encode.go;l=731;drc=424814615491c604e6a0945f33e5a7b779dc2be5

I think the stdlib always compares on a field basis if my understanding of that code is correct.

[arp242]

I wanted to say that it should be checked field by field if the struct is uncomparable instead of returning an error. I'm pretty sure the stdlib does it that way.

The standard library doesn't do anything for structs; omitempty on those seems silently ignored (no error). omitempty support for structs was added in the last release of this library. This is the equivalent of isEmpty(): https://cs.opensource.google/go/go/+/master:src/encoding/json/encode.go;drc=462b78fe7027ef0d2e2b40c3cfd1f5a37d307310;l=340

So I guess that ignoring it is better.

Support for some of these types could be added by looping over all struct fields and recursively calling isEmpty() on them as long as the type isn't a struct. Something like the below seems to work in a quick test, but need to look at it more.

For now, I just changed it to silently ignore these fields.

diff --git encode.go encode.go
index 6b41533..6bfae83 100644
--- encode.go
+++ encode.go
@@ -653,13 +653,20 @@ func (enc *Encoder) isEmpty(rv reflect.Value) bool {
        switch rv.Kind() {
        case reflect.Array, reflect.Slice, reflect.Map, reflect.String:
                return rv.Len() == 0
-       case reflect.Struct:
-               if !rv.Type().Comparable() {
-                       encPanic(fmt.Errorf("type %q cannot be used with omitempty as it's uncomparable", rv.Type()))
-               }
-               return reflect.Zero(rv.Type()).Interface() == rv.Interface()
        case reflect.Bool:
                return !rv.Bool()
+       case reflect.Struct:
+               if rv.Type().Comparable() {
+                       return reflect.Zero(rv.Type()).Interface() == rv.Interface()
+               }
+               // Not comparable; need to loop over all fields and call isEmpty()
+               // recursively so that we check the length for slices etc.
+               for i := 0; i < rv.NumField(); i++ {
+                       if enc.isEmpty(rv.Field(i)) {
+                               return true
+                       }
+               }
+               return false
        }
        return false
 }

[ybaldus]

Yes of course you are right. They will print out an empty object. Can I contribute anything to help you?

[arp242]

Can I contribute anything to help you?

Sure! Can make a PR based on the above with a bunch of test cases (including deeply nested structs and the like).

[arp242]

Also see the discussion over here regarding the implementation: #361 (comment)

[ybaldus]

I will check that. This week is unfortunately a little bit busy, so I will probably do it next week.

[arp242]

Sure, don't feel obliged :-)

[arp242]

#362

[joukewitteveen]

I believe there is an additional issue where isEmpty is called on a fully dereferenced struct, but if the original field type of the struct was a pointer to a struct, then omitempty should be taken to mean "drop if nil, otherwise output".

In other words: isEmpty on any pointer type should return true precisely when the pointer is nil. This is currently not what happens because the field value is fully dereferenced.

[arp242]

Fixed via #366

[joukewitteveen]

#366 does not appear to address the fact that isEmpty is used on fully dereferenced structs, while pointer types should only be considered empty when they are nil. Shall I open a new issue for that?

[arp242]

Shall I open a new issue for that?

Yes please @joukewitteveen; sorry, I didn't really register your comment back in August as I was busy with other things, and when I fixed this yesterday I just didn't see your comment so I didn't look in to it. It seems mostly unrelated anyway (other than being about the omitempty struct tag); usually best to err on the side of creating new issues for things, especially smaller projects like this where people only work on every once in a while. Just so easy to miss stuff otherwise.