proposal: encoding/json/v2: drop Options argument from user-declared methods and functions

[dsnet]

Proposal Details

This is a sub-issue of the "encoding/json/v2" proposal (#71497).

Here, we discuss whether to:

• Drop the json.Options argument from json.MarshalerTo, json.UnmarshalerFrom, json.MarshalToFunc, and json.UnmarshalFromFunc.
• Add an Options accessor method to jsontext.Encoder and jsontext.Decoder.

Specifically, this issue would result in the following changes to the API:

  package json

  type MarshalerTo interface {
- 	MarshalJSONTo(*jsontext.Encoder, Options) error
+ 	MarshalJSONTo(*jsontext.Encoder) error
  }

  type UnmarshalerFrom interface {
- 	UnmarshalJSONFrom(*jsontext.Decoder, Options) error
+ 	UnmarshalJSONFrom(*jsontext.Decoder) error
  }

- func MarshalToFunc[T any](fn func(*jsontext.Encoder, T, Options) error) *Marshalers
+ func MarshalToFunc[T any](fn func(*jsontext.Encoder, T) error) *Marshalers

- func UnmarshalFromFunc[T any](fn func(*jsontext.Decoder, T, Options) error) *Unmarshalers
+ func UnmarshalFromFunc[T any](fn func(*jsontext.Decoder, T) error) *Unmarshalers

  package jsontext

+ func (d *Decoder) Options() Options
+ func (e *Encoder) Options() Options

Functionally, we are dropping options as a direct argument and instead relying on the provided jsontext.Encoder or jsontext.Decoder being capable of storing and retrieving options, where these options are able to represent both semantic options (i.e., options declared in the "json" package) and syntactic options (i.e., options declared in the "jsontext" package). This embraces the possibility that Encoder and Decoder (which are functionally only concerned with the syntactic processing of JSON) are capable of storing semantic options (even if they have no relevance for syntactic processing).

As a side-effect of this change, previously the passing of semantic options to jsontext.NewEncoder, jsontext.Encoder.Reset, jsontext.NewDecoder, or jsontext.Decoder.Reset would have no effect and be ignored. Now, such options are stored within the Encoder or Decoder and while they continue to have no effect on the synctactic processing of JSON, they do take effect if the Encoder or Decoder is then passed to json.MarshalEncode or json.UnmarshalDecode.

Analysis of this change:

• ✔️ This simplifies the implementation of author-defined marshalers and unmarshalers methods.

• ✔️ This makes it impossible to accidentally drop the options in recursive plumbing.

  Currently, it is possible to write the following buggy code:

  func (v T) MarshalJSONTo(enc *jsontext.Encoder, opts json.Options) error {
  	return json.MarshalEncode(enc, v.v) // opts accidentally dropped
  }

  However, by dropping the Options argument, passing the enc alone is sufficiently to properly plumb down the options.

• ✔️ This assists in migration of v1 code. For example:

  dec := jsonv1.NewDecoder(in)
  dec.DisallowUnknownFields()
  for {
  	... := dec.Decode(&v)
  }

  can be more directly re-written as:

  dec := jsontext.NewDecoder(in, jsonv2.RejectUnknownMembers(true))
  for {
  	... := jsonv2.UnmarshalDecode(dec, &v)
  }

  where the options do not need to be repeatedly passed in to every UnmarshalDecode call.

• ❌ This makes it easier for type authors to forget about the existence of options. However, usually authors know to look into the options if there an option that is semantically relevant to the operation at hand.

• ❌ Consulting the options is now an extra level of indirection.

  - json.GetOption(opts, ...)
  + json.GetOption(dec.Options(), ...)

• ⚠️ Type purity has been somewhat diluted.

  In an older protype of jsontext, options were specified using option structs:

  func NewEncoder(w io.Writer, opts EncoderOptions) *Encoder

  By virtue of being a concrete EncoderOptions type, this constructor could only ever contain syntactic options. In fact, it would have been difficult to combine jsontext.EncoderOptions and json.MarshalOptions since that would have resulted in cyclic dependency.

  By switching to variadic options represented by a Go interface, we made it possible to pass semantic options to the Encoder and the implementation is able to store it since it does not need to introspect upon the contents of the semantic options. From the perspective of the Encoder it is just storing some opaque options and returning them back to the user. The loss of type purity is arguably an artifact of the decision to pursue variadic options and the proposed changed is a logical extension (or stronger embrace) of that approach.

Overall, the benefits seem to outweigh the detriments and I believe we should pursue this change.

In this sub-issue, please refrain from off-topic discussions such as:

• Whether to use option structs or variadic options. This sub-issue assumes that we are using variadic options.
• Whether to merge jsontext into jsonv2. This sub-issue assumes that the packages are split apart.

Thank you @willfaught for suggesting this change.

[mateusz834]

However, by dropping the Options argument, passing the enc alone is sufficiently to properly plumb down the options.

What if someone wants to explicitly ignore some option? This way there is no way to do so

[dsnet]

What if someone wants to explicitly ignore some option? This way there is no way to do so

It seems to me that this would be equivalent to:

func (v T) MarshalJSONTo(enc *jsontext.Encoder) error {
	return json.MarshalEncode(enc, v.v, json.DefaultOptionsV2())
}

I struggle to imagine a valid use-case, but at least it's possible.

[gabyhelp]

Related Issues

• proposal: encoding/json/v2: new API for encoding/json #71497

Related Discussions

• encoding/json/v2

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[mateusz834]

What if someone wants to explicitly ignore some option? This way there is no way to do so

That also makes me think that there is no Clone() for Options, if you only want to change a specific option.

I struggle to imagine a valid use-case, but at least it's possible.

Probably some edge-cases, like a need for FormatNilMapAsNull(true), FormatNilSliceAsNull(true), OmitZeroStructFields(false), StringifyNumbers(true), change marshallers.

[dsnet]

That also makes me think that there is no Clone() for Options, if you only want to change a specific option.

Suppose there were a Clone method, how would you envision it being used?

like a need for FormatNilMapAsNull(true) ...

Overriding select options is fairly reasonable, but to completely replace all options seems a bit odd.

[mateusz834]

Suppose there were a Clone method, how would you envision it being used?

Oh right, i thought that that would be the only way to override options, but actually it would not work. To override, people should: JoinOptions(opts, FormatNilMapAsNull(true)).

but to completely replace all options seems a bit odd.

Yep.

[doggedOwl]

Probably some edge-cases, like a need for FormatNilMapAsNull(true), FormatNilSliceAsNull(true), OmitZeroStructFields(false), StringifyNumbers(true), change marshallers.

wrote about this also in parent issue and I think the custom Methods should not be in control on whether to honor an option or not.
In particular if i have a Parent object which contains a child type with a custom Marshaller it woud be very surprising to have one of the fields (that of the child) behave differently from the rest of them.

[rogpeppe]

As one specific data point, I looked at places I've been using go-json-experiment and found at least one place that erroneously did not pass down the Options value when it should have, so definitely +1 for having a less error-prone API in this respect.