encoding/json: panic when unmarshalling to struct with embedded struct pointer

[philpearl]

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

1.12

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
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/phil/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/phil/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/phil/go/src/github.com/unravelin/core/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0j/nhsrh_152tg44s03bvq7mrh80000gn/T/go-build963035476=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/SZeGjpJFpDr

What did you expect to see?

Certainly no panic. Ideally the struct would be fully populated from the json

c := C{
    D: "dddd",
    A: &A{
        B: "gat",
    },
}

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0xffffffff addr=0x0 pc=0x107ad2]

goroutine 1 [running]:
main.(*A).UnmarshalJSON(0x0, 0x45a000, 0x1c, 0x1c, 0x0, 0x0)
/tmp/sandbox606414898/main.go:17 +0x12
encoding/json.(*decodeState).object(0x454060, 0x1242c0, 0x40a0e0, 0x16, 0x454078, 0x207b)
/usr/local/go/src/encoding/json/decode.go:611 +0x2920
encoding/json.(*decodeState).value(0x454060, 0x1242c0, 0x40a0e0, 0x16, 0x454078, 0x22)
/usr/local/go/src/encoding/json/decode.go:381 +0x60
encoding/json.(*decodeState).unmarshal(0x454060, 0x1242c0, 0x40a0e0, 0x454078, 0x0, 0x0)
/usr/local/go/src/encoding/json/decode.go:179 +0x240
encoding/json.Unmarshal(0x45a000, 0x1c, 0x1c, 0x1242c0, 0x40a0e0, 0x20af, 0x8, 0x1250a0)
/usr/local/go/src/encoding/json/decode.go:106 +0x180
main.main()
/tmp/sandbox606414898/main.go:23 +0xc0

Looks like the code sees that c is non-null and implements Unmarshaller, so calls UnmarshalJSON with c.A as the receiver. Unfortunately c.A is nil. Ideally the library would detect that c does not implement Unmarshaller itself and carry on with standard decoding.

The big problem here is that you can have working code, then implement a working unmarshaller on a struct and then find the code as a whole breaks. I found this after generating some easyjson unmarshallers for a subset of the structs in our codebase.

Note similarity to #22967

[dsnet]

The root cause of this is #30148. Closing as duplicate.

[philpearl]

Wow. I understand the difficulty here, but just closing (and presumably ignoring - forgive me if there's a process I don't understand here) seems to be ignoring a big issue. The whole idea of unmarshallers (and marshallers https://play.golang.org/p/KsYZvfY7kvk) seems to be a little broken with embedding.

The "spooky action at a distance" is very unsettling here. I generated some easyjson marshallers for core structs, then found much later on that this broke unrelated code that happened to embed these structs. This means I can never implement an unmarshaller on any public struct in case someone somewhere has embedded it and things will start crashing

Cheeky little /cc @rsc

[dsnet]

ignoring a big issue

Perhaps, but not one specific to encoding/json. Embedding in Go is long known to lead to brittle code. There is an outstanding proposal to remove it in Go 2. See #22013

[dsnet]

This means I can never implement an unmarshaller on any public struct in case someone somewhere has embedded it and things will start crashing

I would say the opposite is true. Users should never embed a type that they do not own because it is unreasonable to expect that the author of the type never adds new fields and methods. Of course, people do this anyways and are surprised when their code breaks. Hence, the brittleness of embedding.

FYI, the Go1 compatibility calls this out:

Under some circumstances, such as when the type is embedded in a struct along with another type, the addition of the new method may break the struct by creating a conflict with an existing method of the other embedded type. We cannot protect against this rare case and do not guarantee compatibility should it arise.

[philpearl]

Ok. This is not something I was previously aware of! My bad for thinking I know what I'm doing. I thought embedding was supposed to be one of the strengths of the language -'composition over inheritance' and all that. Will watch for it carefully in future

…

On Fri, 1 Mar 2019, 12:13 am Joe Tsai, ***@***.***> wrote: This means I can never implement an unmarshaller on any public struct in case someone somewhere has embedded it and things will start crashing I would say the opposite is true. Users should never embed a type that they do not own because it is unreasonable to expect that the author of the type never adds new fields and methods. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30489 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAb33GLDc9r5tpDSTSTU3TVwHpH3ACSxks5vSHCTgaJpZM4bX-WO> .