encoding/json: custom type marshaling doesn't work for map values

[friedrichsenm]

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

$ go version 1.16

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/home/matt/.cache/go-build"

GOENV="/home/matt/.config/go/env"

GOEXE=""

GOEXPERIMENT=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GOINSECURE=""

GOMODCACHE="/home/matt/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="linux"

GOPATH="/home/matt/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/home/matt/sdk/go1.19.1"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/home/matt/sdk/go1.19.1/pkg/tool/linux_amd64"

GOVCS=""

GOVERSION="go1.19.1"

GCCGO="gccgo"

GOAMD64="v1"

AR="ar"

CC="gcc"

CXX="g++"

CGO_ENABLED="1"

GOMOD="/home/matt/GitHub/relay_common/smm_api/go.mod"

GOWORK=""

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2596506419=/tmp/go-build -gno-record-gcc-switches"

What did you do?

If you define custom marshaling/unmarshaling functions for a type, json.Marshal doesn't use them when the custom type is used as value in a map.

https://go.dev/play/p/uggz6OAQo_J

What did you expect to see?

I expect the custom type marshaling functions to be respected even when the custom type is a value in a map

What did you see instead?

JSON reverts to a default marshaling/unmarshaling function

[panjf2000]

cc @mvdan

[dsnet]

This is a known problem. It is related to but not quite identical to #33993 and #22967.

Fundamentally, the issue is that the "encoding/json" package has an non-addressable value on hand, and so it can't call a pointer method. There are ways to resolve this in the internal implementation of "json", but doing so has historically proven to be a breaking change.

[zenovich]

This is a known problem. It is related to but not quite identical to #33993 and #22967.

Fundamentally, the issue is that the "encoding/json" package has an non-addressable value on hand, and so it can't call a pointer method. There are ways to resolve this in the internal implementation of "json", but doing so has historically proven to be a breaking change.

Have those historical breakages been documented somewhere? Where can I read about them?

[dsnet]

Unfortunately not, I used to manage the deployment of the latest Go release into Google's codebase and it was my job to understand why various tests would fail. Most times it was because the test depended on undefined behavior and I would go and fix the test. However, if enough tests failed by some arbitrary threshold, then the upstream change would likely be reverted because if X% of tests in Google break, then it is reasonable to also assume that a similar fraction of tests would break outside of Google. This is the fundamental nature of Hyrum's Law.

The data is probably kept in a random spreadsheet, but I have left Google years ago, so it may have been lost to time.

[zenovich]

It's a really sad story. At the same time, it looks like Go 1 and the Future of Go Programs: Expectations explicitly allow breaking code that relies on unspecified behavior.

[cherrymui]

#68919 has some related discussions.

[gopherbot]

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

[zenovich]

@ianlancetaylor,
As a follow-up to our recent conversation in #68919, I would like to ask you to take a look at the change I've made to fix the issue: https://go.dev/cl/606495.
This change fixes the issue for both encoding/json and encoding/xml and introduces two separate GODEBUG settings allowing to revert to the old behaviour for each package. Happily, it was easy to keep both behaviours without the need of maintaining two separate code branches. At the same time, I added tests fully covering both (old and new) behaviours for both packages as well as the documentation.

[zenovich]

@dsnet Can you look at https://go.dev/cl/606495 please? As the new consistent marshalling can be turned off with a GODEBUG setting, now it should be no risk to break programs depending on the undocumented behaviour.