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.

[ianlancetaylor]

I ran https://go.dev/cl/606495 through Google's tests. 238 tests failed, including the following open source repos. I haven't investigated further to see why the tests failed.

• github.com/goccy/go-json
• github.com/json-iterator/go
• github.com/openconfig/ygot
• github.com/openconfig/ondatra

[zenovich]

@ianlancetaylor Have you tried setting GODEBUG=jsoninconsistentmarshal=1?

[zenovich]

@ianlancetaylor Actually, the the broken tests I see so far are those checking the full compatibility (encoding to the exactly same bytes) with the current (broken) version of encoding/json.
A typical broken test looks like this:

func TestEncodeMarshalJSON(t *testing.T) {
        ...
	var buf, stdbuf bytes.Buffer
	enc := jsoniter.ConfigCompatibleWithStandardLibrary.NewEncoder(&buf)
	enc.Encode(foo)
	stdenc := json.NewEncoder(&stdbuf)
	stdenc.Encode(foo)
	should.Equal(stdbuf.Bytes(), buf.Bytes())
}

(from github.com/json-iterator/go)

[ianlancetaylor]

@dsnet and I have been saying that making this change will cause tests to fail. I just wanted to add some data.

I agree that it is possible to fix the tests.

As I think I said somewhere, a GODEBUG setting is problematic here. A large program can rely indirectly on multiple packages that use JSON. Those packages will not be updated at the same time to the proposed new semantics. That means that there will not be a single GODEBUG setting that lets the entire program behave as expected. A GODEBUG works well to enable or disable a specific behavior. It does not work well to change a behavior from one thing to another, at least not if the behavior can be relied on by various different packages.

[zenovich]

@ianlancetaylor, would it help if we set GODEBUG=jsoninconsistentmarshal=1 by default? This way, people interested in correct marshalling would be able to turn it on explicitly, so there would be no unexpected behaviour changes.

[ianlancetaylor]

I don't think I am explaining my point clearly.

There can be multiple packages that use JSON in a program. Those different packages can expect different choices for GODEBUG=jsoninconsistentmarshal. When different packages update at different time there is no clear single choice for a GODEBUG setting.

[zenovich]

@ianlancetaylor, Ah, that really makes sense. How could we do it correctly then?

[zenovich]

@ianlancetaylor, I believe it's impossible to fix a bug and keep the bug unfixed at the same time. Of course, tests of the libraries intentionally mocking the broken behaviour for the compatibility sake will be broken, but it doesn't sound like a reason for keeping the bug unfixed.
I can remove the GODEBUG settings if you will.

[dsnet]

I believe it's impossible to fix a bug and keep the bug unfixed at the same time.

Agreed and highly unfortunate. I made the argument earlier:

I agree that the the change is in the right direction what the package should have done, but I can tell you that previous attempts to fix this have been rolled back. The v2 "json" package is a place where we can fix this since there cannot be any usages depending on the current behavior.

My position is that we should just leave this behavior as is in v1 as a unfortunate quirk of library and fix it for v2. In other words, we leave v1 "unfixed" and "fix" the bug in v2.

[zenovich]

@dsnet, Doing this way can take many years.

The discussion about encoding/json/v2 started in October 2023 (#63397), but it's still at the proposal formulation stage (discussable draft) after one year of discussing. That's all good, but it doesn't fix the bugs for now.

The package trying to implement the (still not final and being discussed) proposal of v2 (https://github.com/go-json-experiment/json) is still called 'experimental', has 19 open issues, and still isn't compatible with the current encoding/json package (see go-json-experiment/json#16) although the work was started in October 2020 (see go-json-experiment/json@e1c1885), so four years ago.

Please get me right, I'm not against encoding/json/v2 itself, but it cannot take years to fix serious bugs. This bug was reported for the first time in 2013 (#6468), but, after almost eleven years of waiting, it's still not fixed. It still causes production issues for current users.

Also, I would like to hear opinions from people authored the packages this change breaks tests of (@goccy, @taowen, @wenovus, @robshakir, @greg-dennis).

[mvdan]

I made multiple fixes to encoding/json over the years, similar to yours, which were reverted due to backwards compatibility issues. Like @dsnet says, the way forward is v2. Yes, it is not quick nor easy, but it is the only way.

[zenovich]

At the same time, the bug is still the source of problems for end users, they are trying to find ways avoiding usage of the standard encoding library: go-chi/render#47

[dsnet]

Doing this way can take many years.

This is the reality of using a programming language that has arguably "crossed the chasm". Prior to crossing the chasm, we can be more aggressive about making breaking changes (even if they were good changes). After crossing the chasm, the later adopters of Go really want stability (even if that means preserving arguably buggy behavior). As a project gets older, stability inherently become more prioritized than progress. That isn't to say that Go can't make progress anymore, it's just that there are many more restrictions in how we can do so.

[dsnet]

Also, I think much of the discussion has been re-hashing the same arguments. Fundamentally, I think we have different perspectives on which take higher priority: fixing a bug or providing stable software. The maintainers of Go are on the hook for ensuring that Go remains stable. It's relatively easy for someone to make a bug fix, but it's much harder to handle the long-term maintenance cost of churn. I don't speak for all the Go maintainers, but my interpretation of historical decision-making is that stability is weighted higher than progress. Other languages may choose a different philosophy, but this is generally one of the hallmarks of Go.