omitisempty support

[bradleypeabody]

This PR adds support for an omitisempty tag, which works like omitempty but emits additional checks for emptiness using the interface { IsEmpty() bool }.

This should have no effect on code generated without omitisempty. But when omitisempty is included on a field, the result is to perform the existing emptiness check on that field plus || checkIsEmptyf(x), which calls IsEmpty() via interface if possible. This also emits the definition of checkIsEmpty (3 lines) once at the top of each MarshalMsg or EncodeMsg method as appropriate.

I've also include some test coverage (could use more work, but I think what's here is pretty good proof this is a viable feature and doesn't break anything).

Some additional notes on the implementation:

• The biggest change is the IfZeroExpr method was refactor to take an argument which triggers the omitisempty behavior. This meant changing the Elem interface, all of its implementations and all calls to it. That's where most of the work is being done to decide how to perform empty checks in which cases.
• From there, the changes to encode.go and marshal.go were relatively simple. I had to add a mechanism to only output checkIsEmptyIntf once per function, but ended up being easy since encodeGen and marshalGen each provide a good place to manage this state.
• There is some inefficiency in cases where someone uses omitisempty on a type that cannot possibly support an IsEmpty method (a primitive type) - in some places I was able to optimize this away and have it not perform the impossible check but a lot of cases still do this call that will always return false. I figure this is not a big deal since omitisempty is entirely opt-in on the part of the user.

[klauspost]

LGTM.

The only thing that annoys me is that omitisempty is very hard to read - and easy to mistake for omitempty.

I don't know if iszero would make it clearer, and then use the IsZero() bool signature as seen on time.Time?

However, I can surely live with it. Just wondering if that makes sense to anyone else.

[bradleypeabody]

I'm not opposed to renaming the tag option to iszero, and the interface to IsZero() bool, makes good sense to me. Unless there is any objection, I will go ahead and make this change and get this pushed soon.

[klauspost]

@philhofer Do you see that as a valuable change?

[philhofer]

Yeah, I'm in agreement that iszero is less likely to be misread than omitisempty

[bradleypeabody]

Thanks for the feedback. I went to start making these changes, a couple questions came up:

1. I'm thinking it might make more sense for check for the IsZero() bool interface to be completely separate from the existing omitempty functionality. Some examples to illustrate:

type Something struct {
    A CustomType `msg:"a,omitempty"` // omitted if A == (CustomType{})
    B CustomType `msg:"b,omitzero"` // omitted if B.IsZero()
    C CustomType `msg:"c,omitempty,omitzero"` omitted if C == (CustomType{}) || C.IsZero()
}

This way they are just two totally separate features and people can mix and match as they want to.

2. The name "zero" referring to the IsZero interface vs "empty" which actually refers to comparing against the Go zero value - that part still bothers me a little. After thinking of various options for names, the only other one that seems to make any sense to me is something like omitcheck as the tag name, and MsgOmit() bool as the interface.(?) Let me know either of you like that better. For me as a user of this library, I think I would be less surprised to find that "omitcheck" involved a separate msgp-specific interface that I needed to implement, than the fact that "omitzero" didn't refer to the actual Go zero value of a type. The scenarios driving this situation all involved custom types, so making a msgp-specific method to indicate specifically if we want to output a msgpack field seems to make a lot of sense. (Where as IsZero() is convenient only if the semantics of whatever that method checks for also align with the reason for omitting a field in msgpack output - which may not always be the case. A realistic example is I might want a time type that also omits Unix Epoch value 0 (1 Jan 1970 UTC). Plus, interestingly enough. in the case of e.g. time.IsZero(), that works just fine with omitempty, since IsZero() is just an alias for t == (time.Time{}).)

[klauspost]

Agree. IMO if omitzero is specified the codegen should assume the struct has func (f Foo) IsZero() bool method. That would also get rid of var checkIsEmptyIntf = func(i interface{}) bool. It sucks that we have to convert it to an interface just for this in the "happy path".

omitempty already checks against empty structs, so we cannot modify its behavior and TBH I don't see the need for both omitempty,omitzero. You just specify the one you want.

Forcing IsZero will also make it clear that you should just use omitempty if you want a zero value compare.

Does that sound reasonable to you?

Please add tests for //msgp:tuple Foo to ensure that omitzero is ignored for those.

[bradleypeabody]

@klauspost Yup, agreed on all points and thanks. I'll need a few days or so but I'll hammer this out and get the changes pushed back here.

[bradleypeabody]

This was simpler than I thought. See #334, which replaces this PR.