encoding/json: document behaviour of json.Marshal() with non-pointer elements with MarshalJSON() defined for pointers

[zenovich]

Go version

go1.22.6, go1.23.0

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/dzenovich/Library/Caches/go-build'
GOENV='/Users/dzenovich/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/dzenovich/.gvm/pkgsets/go1.22.6/global/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/dzenovich/.gvm/pkgsets/go1.22.6/global'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/dzenovich/.gvm/gos/go1.22.6'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/dzenovich/.gvm/gos/go1.22.6/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.22.6'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/49/4jhg4kzs3pn3kfn5c4c1hkdw0000gn/T/go-build2096370642=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

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

What did you see happen?

slice of structs: [{"V":"marshalled"}] map with structs: {"v":{"V":{}}}

What did you expect to see?

slice of structs: [{"V":"marshalled"}] map with structs: {"v":{"V":"marshalled"}}

[gabyhelp]

Related Issues and Documentation

• encoding/json: Json marshaler seems to not marshal correctly if there is an embedded struct with custom marshaller #46799 (closed)
• encoding/json: Error in Marshalling Slice of Struct type -> affected/package: json #61710 (closed)
• encoding/json: Marshaling embedded structures which supports custom json.Marshaler interface #29336 (closed)
• affected/package: encoding/json #54609 (closed)
• encoding/json: UnmarshalJSON not working consistently with struct embedding #39175 (closed)
• encoding/json: Marshal silently ignores custom implementations on pass by value #30351 (closed)
• encoding/json: Marshal skips checking Marshaler implementation for nil pointer #48430 (closed)
• encoding/json: adding a MarshalJSON method confuses JSON marshalling #67154 (closed)
• encoding/json: panic when unmarshalling to struct with embedded struct pointer #30489 (closed)
• encoding/json: custom type marshaling doesn't work for map values #55890

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

https://stackoverflow.com/questions/32495402/why-does-go-forbid-taking-the-address-of-map-member-yet-allows-slice-el

Unlike many projects, the Go project does not use GitHub Issues for general discussion or asking questions. GitHub Issues are used for tracking bugs and proposals only.

For questions please refer to https://github.com/golang/go/wiki/Questions

[zenovich]

I didn't ask about taking the address of map members. This is a bug report. It's about inconsistent behaviour of json.Marshal().

[zenovich]

Also, I'm sure this behaviour can be fixed regardless of possibility to take an address of map members.

[timothy-king]

Re-opening. Related to #22967.

Here is a reduced version https://go.dev/play/p/_akdvjspgou:

type T struct{}

func (*T) MarshalJSON() ([]byte, error) {
    return []byte(`"marshalled"`), nil
}

func main() {
    s, _ := json.Marshal([]T{{}})
    m, _ := json.Marshal(map[string]interface{}{
        "T":  T{},     // not a json.Marshaler
        "*T": &T{},  // json.Marshaler
    })

    fmt.Printf("[]T: %s\n", s)
    fmt.Printf("map[string]any: %s\n", m)
}

This prints:

[]T: ["marshalled"]
map[string]any: {"*T":"marshalled","T":{}}

This boils down to json.Marshal treating the T elements in []T as a *T. A T is not a json.Marshaler while a *T is a json.Marshaler. Relevant line.

I doubt we can change this behavior. A large amount of code almost certainly depends on this. We may be able to better describe the current behavior in the documentation. (If this is described in the package documentation, apologies for missing it https://pkg.go.dev/encoding/json#Marshaler .)

@dsnet

[zenovich]

Folks, it's easy to fix. Really, let's fix it instead of documenting.

[zenovich]

I can create a PR if you will.

[ianlancetaylor]

The problem is that changing behavior at this level will almost certainly break existing working code. If we want to make this change, we need to first convince ourselves that that won't happen.

[zenovich]

The current behaviour doesn't make sense, so it's unlikely for existing code to rely on it.

[ianlancetaylor]

https://www.hyrumslaw.com/

[seankhliao]

I think this is #55890, where it has been noted that changing it is considered a breaking change

[zenovich]

I think this is #55890, where it has been noted that changing it is considered a breaking change

There are no proof links of that though

[zenovich]

Probably we could consider this change as something that can break something potentially, but will make life of Golang developers much better in the future. Looking at all the bug reports related to the issue, I see that I'm not the only one who suffers from this issue. Just imagine, you can return rows returned by the DB as JSON, but you cannot return one DB row as JSON in format '{"data": row}' without creating a wrapping struct.

[gopherbot]

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

[dsnet]

There are no proof links of that though

I tried fixing this in the past. There is proof of this. We can't accept this change in the v1 "json" package unfortunately.

[zenovich]

I can only say that my change doesn't break any tests, so I consider it non-breaking.

[dsnet]

I don't think lack of tests is a good metric for "non-breaking". It just goes to show that our test coverage is lacking.

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.

[zenovich]

According to Russ Cox:

That raises an obvious question: when should we expect the Go 2 specification that breaks old Go 1 programs?

The answer is never. Go 2, in the sense of breaking with the past and no longer compiling old programs, is never going to happen. Go 2 in the sense of being the major revision of Go 1 we started toward in 2017 has already happened. (from https://go.dev/blog/compat)

I (and, I believe, many others too) would be happy to have consistent marshalling in Golang a bit earlier.

[dsnet]

That quote by Russ is regarding the Go language specification itself.

A "encoding/json/v2" package is entirely different package that would be distinct from the v1 "encoding/json" package. Anything importing and using v1 "encoding/json" would continue to function as is, while anything that imports "encoding/json/v2" would use newer semantics. A program is allowed to import both v1 and v2 simultaneously.

(as an implementation detail, both v1 and v2 will use the same implementation under-the-hood, and pass relevant flags to specify whether to use v1 or v2 semantics.)

[zenovich]

As an idea, can we turn on the new consistent marshalling behaviour via setting an ENV variable, for example? This way it will not break anything by accident.

[zenovich]

That quote by Russ is regarding the Go language specification itself.

A "encoding/json/v2" package is entirely different package that would be distinct from the v1 "encoding/json" package. Anything importing and using v1 "encoding/json" would continue to function as is, while anything that imports "encoding/json/v2" would use newer semantics. A program is allowed to import both v1 and v2 simultaneously.

(as an implementation detail, both v1 and v2 will use the same implementation under-the-hood, and pass relevant flags to specify whether to use v1 or v2 semantics.)

Sounds good, but marshalling is mostly called by third-party libraries which will still use v1 "encoding/json". No chances to upgrade quickly :(

[mateusz834]

As an idea, can we turn on the new consistent marshalling behaviour via setting an ENV variable, for example? This way it will not break anything by accident.

We already have GODEBUGs.

[zenovich]

Btw, it's worth noting that Go 1 and the Future of Go Programs: Expectations say about this case:

Unspecified behavior. The Go specification tries to be explicit about most properties of the language, but there are some aspects that are undefined. Programs that depend on such unspecified behavior may break in future releases.

[ianlancetaylor]

@zenovich I am sympathetic to your arguments. But every change is a matter of weighing costs and benefits. You are focusing on the benefits.

We have to seriously consider what happens if we do make this change. We know, from what @dsnet says, that some existing code will break.

The effect will be that people will upgrade to a new version of Go, and their program will stop working in a strange way. They will look at the release notes. It's unlikely that the failure mode will be clearly reflected in the release notes, but they may eventually figure out that it is due to a change in how encoding/json handles the MarshalJSON (or MarshalText) method. For most people, this will be buried in some dependency of their program that they have never looked at closely. They will have to figure out which dependency it is, report a bug, and wait for a fix.

Or, more likely, they won't even go down that path, and will just report a bug to us saying "I updated to a new Go release and my program stopped working."

No matter what happens, it's not a good experience.

So, changing this behavior carries a cost.

Of course, changing this behavior also carries a benefit, which you've described.

Now we have to weigh the cost and the benefit, and decide what to do.

Our current decision is that we will aim to have more clear behavior in encoding/json/v2. That will let us reap the benefit over time, with a much smaller cost.

It's true that other choices are possible. The way to argue in favor of those other choices is not to just focus on the benefits. It's to explain why the benefit is worth the cost.

[zenovich]

@ianlancetaylor I understand your point. What do you think about turning the consistent marshalling behavior on/off via the GODEBUG variable?

[ianlancetaylor]

True, it's clear that if we make this change we will need to support disabling it using GODEBUG. That mitigates the cost somewhat, in that once someone figures out what the problem is, they can set the GODEBUG rather than wait for their dependency to be fixed.

But of course a GODEBUG has a different, arguably lesser, cost: we have to maintain two different code branches and test both of them. Plus of course documentation.

This is also an unfortunate case where the same code behaves differently depending on the GODEBUG setting. The ideal GODEBUG is one where one version or the other simply fails. For example, ending support for some insecure crypto library unless a GODEBUG is set. In that case it is generally clear enough that GODEBUG is the right approach, and that setting GODEBUG (which is a global setting) won't introduce an unexpected failure. When GODEBUG causes a behavior change, we have to worry about the possibility that some parts of a large program expect one behavior, and some parts expect another behavior.

So we still have to weigh costs and benefits.

[cherrymui]

It seems this is very similar (or same) to #55890. Closing as a duplicate. The discussion can be continued there. Thanks.