Empty values serialising to the zero value is semantically confusing

[Southclaws]

Great library! So much easier than pointers all over the place. We use this pretty heavily in our codebase and it's been mostly fine however this behaviour is causing some confusing outcomes when it comes to JSON serialisation and consumption by TypeScript.

One nice simple yet annoying side of Go is the use of zero values to indicate emptiness. This is fine for some values but structs are problematic.

The issue I've just noticed is optional.Optional[time.Time] this serialises to:

{
  "date": "0001-01-01T00:00:00Z"
}

Whereas the expected result from the perspective of a JSON consumer (in any language) is one of:

{
  "date": null
}

Or, simply:

{}

Where result.date would yield undefined and be typed as:

type Payload = {
  date: string | undefined;
}
// or with a Zod schema:
z.object({
  date: z.string().optional()
})

(Which we generate from Go structs using Supervillain)

---

Given that use-case, I propose that MarshalJSON be implemented in a v2 (breaking change) as:

func (o Optional[T]) MarshalJSON() (data []byte, err error) {
	if v, ok := o.Get(); ok {
		return json.Marshal(v)
	}
	return []byte("null")
}

(There's no way to omit a field via MarshalJSON yet, see: https://github.com/golang/go/issues/50480)

[leighmcculloch]

Thanks for reporting this. This is a bug I think. The intent is for this library to handle the empty state well, and the marshal step should be treating the final value as if it's a pointer: empty is no value/null for all types. Let me see how we can fix this in a safe predictable way for all types.

[leighmcculloch]

I want to make sure I understand the problem clearly before I jump into the PR and into how this should be fixed.

I'm using this example to explore the problem: https://go.dev/play/p/E4TxEKZhwhE

1. When a struct field is tagged with json:"time": a. optional.Of(time.Now()) => {"time":"2009-11-10T23:00:00Z"} b. optional.Of(time.Time{}) => {"time":"0001-01-01T00:00:00Z"} c. optional.Empty[time.Time]() => {"time":"0001-01-01T00:00:00Z"}

2. When a struct field is tagged with json:"time,omitempty": a. optional.Of(time.Now()) => {"time":"2009-11-10T23:00:00Z"} b. optional.Of(time.Time{}) => {"time":"0001-01-01T00:00:00Z"} c. optional.Empty[time.Time]()) => {}

1a and 2a are relatively self explanatory. When a time value is set we want that to be serialized.

1b and 2b are less obvious. A time value is still set within the optional. If we think about the option as either "none" or "some value" the optional is in the "some value" state. The optional package cannot know for every type if its "some value" should also be treated as "none" because some types default / zero value is meaningful.

1c and 2c I think highlight the bug. The optional is empty and so in 2c it correctly renders to an omitted field. The optional is empty for 1c also but it still renders the time, which doesn't make much sense. In this situation I would expect null to be rendered, as you pointed out, because if a developer wanted a zero value to be rendered they wouldn't be using the optional at all.

Could you confirm if 1c is the case you're seeing as a bug?

[Southclaws]

Hey Leigh, sorry for the late response!

Yeah, case 1-c is what I'm seeing as the bug here. omitempty is useful for avoiding this and we've done that in a couple of places before I used my fork (we make use of code generation tools a lot and sometimes it's not easy to add omitempty to every field)

For b cases, I actually wrote an IsZero() bool interface which applies to time and decimals and a few other types. Not relevant for this specific issue, but quite useful!

2-c is correct by omitting which is controlled by the JSON library itself so 1-c is the only problem here, an empty value serialising as a zero value instead of null. (it would be preferable to omit entirely, but as mentioned it's not possible to do this from the default JSON library)

Thanks! I've opened a PR with the change, though my coworker also pushed some changes to that fork so I could move the fix for this issue onto a separate branch if necessary.

[leighmcculloch]

Thank for confirming. I've been pondering the behavior, and why I implemented it this way, and all the options that exist for how 1c could behave. I agree, null is more appropriate in the 1c case, but I wanted to write out my thinking in full.

There are three ways the 1c case, when the optional is empty, could behave:

1. Emit output zero value

   { "date": "0001-01-01T00:00:00Z" }
   

   This behavior is the same as not using the optional at all. We could argue, and I think my original intent was, that the developer hasn't indicated the value should be omitted, so therefore it should be rendered as its default value. However, an empty optional that renders as a default value cannot be round tripped back to an empty value, it will round trip back as a some-value with the default value set. This inability to round trip is pretty inconvenient, so I do think it is a big.

2. Omit

   {}
   

   This behavior would be identical to the 2c case, when the omitempty tag option is provided. It wouldn't make much sense for 1c to behave in the same way, since a developer could simply use the omitempty tag to achieve the same behavior. No native type omits when the tag option isn't provided and so it would be surprising for this optional to do so.

3. Emit output null

   { "date": null }
   

   This behavior would be the same as using a pointer type, but part of the point of the optional type is to provide optional behavior without the use of pointers. So this option is likely the most appropriate behavior that the 1c case should take. We could argue that null, or nil, is only relevant in the context of a value that can also carry the value nil and if a variable has the type optional.Optional[time.Time] then it can't be nil. However, in JSON the only way to represent a value as empty for all types is null, so it feels like an appropriate use of null.