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 :-)