confusion with WithUnmarshalers

[elv-gilles]

I wanted to install multiple unmarshalers and wrote the wrong code below:

	err := json.UnmarshalDecode(dec, c,
		json.WithUnmarshalers(
			json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *Conf, opts json.Options) error {
				// do something
				return json.SkipFunc
			})),
		json.WithUnmarshalers(
			json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *SubConf, opts json.Options) error {
				// do something
				return json.SkipFunc
			})))

This code returns with no error, but the first unmarshalers is ignored.

Instead, after navigating in the source code of json-experiment, I discovered I should have written:

	err := json.UnmarshalDecode(dec, c,
		json.WithUnmarshalers(
			json.NewUnmarshalers(
				json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *Conf, opts json.Options) error {
					// do something
					return json.SkipFunc
				}),
				json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *SubConf, opts json.Options) error {
					// do something
					return json.SkipFunc
				}),
			)))

Looking at the code, I also see that options are merged through *Struct.Join.

I think it's currently easy to be tripped and (at least) one of the below - in order of increased preference - would be of interest:

1. improve the comment of UnmarshalDecode or Options to prominently say that only the last option of a given kind will be used (if that comment exists I missed it), and/or
2. have *Struct.Join return an error in case where an option would overwrite a previously set one, and/or
3. have *Struct.Join handle such case and do what NewUnmarshalers does (for marshalers/unmarshalers)

[dsnet]

For case 1, we currently document on JoinOptions:

Properties set in later options override the value of previously set properties.

but we can make this more explicit elsewhere in the documentation.

If this does get merged into the stdlib, one could imagine a go vet check for obviously incompatible sets of provided options.

The downside of case 2 is that we would need to return an error in more cases where we take in ...Option. For example, NewEncoder, NewDecoder, Encoder.Reset and Decoder.Reset would all need to return an error now.

The downside of case 3 is that it complicates what JoinOpions means for different option parameters. Always overriding is a relatively simple rule, while merging under some special cases can get confusing.

[elv-gilles]

Looks good, I guess (now - that I know - that seems obvious :-)

[MarkRosemaker]

I faced a similar issue.

I read the documentation @dsnet, but I understood it to mean that an unmarshal function for a struct will override a previous one for the same struct, not that an UnmarshalFuncV2 for one struct (e.g., SubConf) will override an UnmarshalFuncV2 for another (e.g., Conf).

Would it be feasible for the options to merge those definitions smartly? This would be particularly useful for my use case, where users can add marshalers/unmarshalers dynamically.

Incidentally, I had to implement the following workaround to manage dynamic addition of marshalers/unmarshalers:

type JSONConfig struct {
	mu           sync.Mutex
	marshalers   []*json.Marshalers
	unmarshalers []*json.Unmarshalers
	other        []json.Options
}

func (c *JSONConfig) AddMarshaler(m *json.Marshalers) {
	c.mu.Lock()
	defer c.mu.Unlock()

	c.marshalers = append(c.marshalers, m)
}

func (c *JSONConfig) AddUnmarshaler(u *json.Unmarshalers) {
	c.mu.Lock()
	defer c.mu.Unlock()

	c.unmarshalers = append(c.unmarshalers, u)
}

func (c *JSONConfig) AddOption(o json.Options) {
	c.mu.Lock()
	defer c.mu.Unlock()

	c.other = append(c.other, o)
}

func (c *JSONConfig) Options() json.Options {
	return json.JoinOptions(append(
		c.other,
		json.WithMarshalers(json.NewMarshalers(c.marshalers...)),
		json.WithUnmarshalers(json.NewUnmarshalers(c.unmarshalers...)),
	)...)
}


// in action:

var cfg = &JSONConfig{}

....

	err := json.Unmarshal(in, &v, cfg.Options())

Having such functionality built into the JSON library would greatly simplify managing marshalers/unmarshalers dynamically and avoid the need for custom configurations like mine.

Additionally, the plural method names like json.WithUnmarshalers seem a bit misleading since they take just one argument. Perhaps reconsidering the naming or allowing multiple arguments directly could help clarify their usage.