encoding/json: bad encoding of field with MarshalJSON method

[larhun]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

version 1.9

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

windows/amd64

What did you do?

package main

import (
    "encoding/json"
    "os"
)

type T bool

func (v *T) MarshalJSON() ([]byte, error) {
    return []byte{'1'}, nil
}

type S struct {
    X T
}

func main() {
    v := S{true}
    e := json.NewEncoder(os.Stderr)

    e.Encode(v)  // should print {"X":true}
    e.Encode(&v) // should print the same value
}

What did you expect to see?

{"X":true}
{"X":true}

What did you see instead?

{"X":true}
{"X":1}

Issue

The json.Marshal documentation states that:

Pointer values encode as the value pointed to. A nil pointer encodes as the null JSON value.

Thus, &v should be marshaled to the same JSON value as v:

{"X":true}

Moreover, it states that:

If an encountered value implements the Marshaler interface and is not a nil pointer, Marshal calls its MarshalJSON method to produce JSON.

Therefore, Marshal should not call the MarshalJSON method to produce JSON for the X field, because its T type does not implement the json.Marshaler interface. In fact, MarshalJSON has *T receiver and the Go documentation states:

The method set of an interface type is its interface. The method set of any other type T consists of all methods declared with receiver type T. ... The method set of a type determines the interfaces that the type implements ...

As a final remark:

1. from the source code, the most relevant difference between cases v and &v is that X field becomes addressable in case &v, changing the encoding generated by condAddrEncoder;

2. implementing MarshalJSON with T value receiver makes T a json.Marshaler interface, with X values properly encoded by MarshalJSON:

   {"X":1}
   {"X":1}

3. if the intended behavior is that Marshal should anyway call the MarshalJSON method on encoding T values, whenever *T implements the json.Marshaler interface, that should be clearly documented.

[dsnet]

This does seem like surprising behavior. It should either be documented or the encoder will need to jump through some hoops to copy the value so that it can be addressable.

[larhun]

This issue is more general and affects all custom encoders accepted by json and xml packages (maybe other?):

    encoding.TextMarshaler
    json.Marshaler
    xml.Marshaler
    xml.MarshalerAttr

More examples at:

• https://play.golang.org/p/4pkPQSGa7E (for json)
• https://play.golang.org/p/tU63LW0kIL (for (xml)

It occurs whenever a type T has a custom encoder with *T pointer receiver. In that case something weird and not documented happens on encoding a value x of type T: it does try to get the x address and call the custom encoder. However, if x is not addressable, it cannot succeed.

Is the address resolution really necessary? I do not think so. The custom encoder should only be called if properly implemented, as clearly stated by the Go documentation:

The method set of an interface type is its interface. The method set of any other type T consists of all methods declared with receiver type T. ... The method set of a type determines the interfaces that the type implements ...

The idiomatic and recommended implementation for a custom encoder should be with T value receiver. However, if *T pointer receiver is used, &x (addressable) should be encoded as x (not addressable): a varying behavior is confusing and not acceptable.

[larhun]

A minimal solution could be: clarify the behavior of each custom encoders by adding that T value receiver is the expected usage and *T pointer receiver may fail to be used.

A definitive solution could be: change the documentation and implementation of the encoding process by first checking if the value under encoding is a pointer, i.e. the first step should be:

Pointer values encode as the value pointed to.

and, therefore, call a custom encoder only if implemented with T value receiver. That way, a custom encoder with *T pointer receiver will never be called.

[rsc]

It seems clear that Encode(&v) should return {"X":1}. What's less clear is Encode(v). It can't return {"X":1} and today it returns {"X":true} but perhaps it should instead return an error (I can see there's a pointer method but I can't take the address of the value to get a pointer). Leaving that decision - is this OK or is should Encode return an error? - for Go 1.13.

[whyrusleeping]

I think that I would prefer returning an error in this case rather than exposing such weird behavior, definitely feels like classic 'undefined behavior'.

[josharian]

I just got bitten by this. Another option is to have vet check that MarshalJSON uses a value receiver.

[ash2k]

Ran into this today and made a quiz for this issue: https://play.golang.org/p/8WejDUNc907

Some of the people I asked guessed the correct answer but nobody was able to explain why it works like that (doesn't call MarshalJSON()). Some were close, but nobody seemed to know about addressability.

I think it should return an error, like Russ is suggesting above.

[CMDMichalKoval]

Hello, i found some info. In slice null pointer is OK. Marshaling ouside of slice, returns empty object.

More here: https://go.dev/play/p/m0smfLlycBg

[ghost]

Aren't there reasons that a pointer receiver is desired, and sometimes necessary? The go tutorial says that a non-pointer receiver will copy the entire structure before executing. For a big structure, that is undesirable, but for a structure that embeds a sync.Mutex, wouldn't that be impossible, due to:

// A Mutex must not be copied after first use.

Are we saying that trying to call MarshalJSON on a structure that uses a mutex for locking is not possible to get right?

[dsnet]

We can't the behavior in "encoding/json" as this would break existing usages. The "encoding/json/v2" draft design proposes to fix this issue by always treating values as addressable (shallow copying if necessary).

[dmitris]

could changing the receiver of MarshalJSON from a pointer to the value have performance implications, with the whole structure being copied instead of just a pointer?

[gopherbot]

Change https://go.dev/cl/606495 mentions this issue: encoding: add full support for marshalers with pointer receivers