Proposal: Add check via interface for IsEmpty() method, allows treating unknown types as empty

[bradleypeabody]

As a heavy user of the omitempty feature, and msgp in general, I've run into an issue where I have some types which implement msgp.Marshaler and other related interfaces manually. This is for custom types where I need to carefully tune the mapping between msgpack and the Go type, e.g. I have a type Float32 struct { Val float32; Ok bool } as a type which represents a float32 value which can be present or not, without having to use a pointer (makes a difference when you have to load millions or billions of them into memory).

However when types like these are referenced in another struct which does use the code generator, the generator is not aware of these non-code-generated types, and has no way to check for emptiness (the specific problem is it has no way to know if if someFloat32Val == (Float32{}) { is a valid Go statement without inspecting the fields on Float32, which are not within the scope of what the code currently analyzes).

A very simple way to get around all of this would be to make the code generator emit code like so for these types

// inside MarshalMsg and EncodeMsg

type isEmptier interface { IsEmpty() bool }

// instead of skipping the check for emptiness if the type of SomeField is unknown, we can implement our empty check like so:
if e, ok := z.SomeField.(.isEmptier); ok && e.IsEmpty() {

I believe this should have virtually no impact on existing implementations but allow people with this same case I'm describing above to easily implement a custom definition of "empty" if they need/choose to. It would probably be a very small PR as well, I just wanted to see if there was any feedback on the idea before I did the work on it.

[philhofer]

What if we checked for emptiness by comparing against the zero value of the type? For example:

var empty typeName
if z.someField == empty { /* is empty */ }

[bradleypeabody]

The unfortunate issue with this is we don't have a way of knowing if this will compile, since things like byte slices cannot be compared:

package main

type SomeType struct{ A []byte }
type SomeParentType struct{ SomeField SomeType }

func main() {

	var z SomeParentType
	var empty SomeType
	if z.SomeField == empty { /* is empty */
	}

}

Yields the error:

./prog.go:11:5: invalid operation: z.SomeField == empty (struct containing []byte cannot be compared)

In Playground: https://go.dev/play/p/XufWVZe1Fgn

In my case, I'm not sure if every custom type I have is comparable, thus the idea with the interface. If you feel really strongly about the issue I could probably make it work to refactor some things so everything is comparable, but for my case it's certainly less ideal and a fair bit more work. The interface gives a lot of flexibility, and I'm pretty sure the type check is a fast lookup, maybe it would even optimize down to something static - I can do some decompiling if there's a question on the performance.

[philhofer]

The only issue I can see with the interface is that someone somewhere may have already attached an IsEmpty method to a particular type that means something different. So we might want to make the new behavior opt-in somehow just to make sure existing code continues to work exactly as it does now.

[bradleypeabody]

I see what you mean and agree having this behavior be opt-in would be the least likely to break things for other users.

Using struct tags, two options come to mind:

A. SomeField SomeType `msg:"some_field,omitisempty"` ("omitisempty" would act like "omitempty" but with this additional behavior of checking the interface)
B. SomeField SomeType `msg:"some_field,omitempty,isemptyintf"` (where "isemptyintf" is essentially an option to the omitempty tag)

And in either case this would basically just trigger emitting struct comparisons as the interface check instead of ==, and in all other ways would keep the same omitempty behavior. Obviously primitive types like int won't have methods, but if someone decided to do type A int and then uses A as field type, it seems like omitisempty should trigger the isEmpty interface check since it's entirely possible to add an IsEmpty method to type A. I'll have to look more closely at that specific case, but I don't think it has a big impact on the overall design of this option regardless.

My preference would be to lean toward option A as it seems more concise, let me know if you feel differently. I was trying to think if there would be some other appropriate way to trigger this behavior such as a command line flag, but I'm thinking a struct tag is the most apropos.

If you're good with option A, I'll put together a PR.

[bradleypeabody]

Implemented omitisempty tag in PR #326

[klauspost]

Implemented in #334