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.

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.

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.

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.

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

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

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.

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

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?

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

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?