Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding/json: unmarshaler doesn't find UnmarshalJSON on embedded anonymous struct #46516

Open
fancl20 opened this issue Jun 2, 2021 · 6 comments

Comments

@fancl20
Copy link

@fancl20 fancl20 commented Jun 2, 2021

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

$ go version
go version go1.15.9 linux/amd64

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="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.15/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build393930481=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

Outer1 unmarshalled and Outer2 failed.

type Outer1 struct {
	Inner inner
}
type inner struct{ Mixin }

type Outer2 struct {
	Inner struct{ Mixin }
}

type Mixin string

func (r * Mixin) UnmarshalJSON(data []byte) error {
	return json.Unmarshal(data, (*string)(r))
}

What did you expect to see?

Outer2 unmarshalled as Outer1

What did you see instead?

panic: json: cannot unmarshal string into Go struct field Outer2.Inner of type struct { main.Mixin }
@mvdan
Copy link
Member

@mvdan mvdan commented Jun 2, 2021

At first glance this seems like a bug, thanks. There have been many bugs in the past involving struct embedding, but after a quick search I don't think this is a duplicate. Want to try working on a fix with a test?

cc @dsnet

@mvdan mvdan changed the title json unmarshaler doesn't find UnmarshalJSON on embedded anonymous struct encoding/json: unmarshaler doesn't find UnmarshalJSON on embedded anonymous struct Jun 2, 2021
@googollee
Copy link

@googollee googollee commented Jun 2, 2021

It's because json.indirect() only tries to get a field address with a named type. See https://cs.opensource.google/go/go/+/refs/tags/go1.16.4:src/encoding/json/decode.go;l=443.

The type of Outer2.Inner is a struct without a name, so json.indirect() doesn't try its pointer and of course it's not compatible with json.UnmarshalJSONinterface.

I think at the entrance of json.indirect(), checking v.CanAddr() is enough. It needs not to check if v is a named type.

@dsnet
Copy link
Member

@dsnet dsnet commented Jun 2, 2021

@googollee, thank you for your analysis. I agree with your assessment of why this isn't triggering. It seems that this is a bug that's existed for 10 years :)

We could try fixing this for the Go1.18 release cycle, but I'm always wary of how fixing bugs breaks a number of users who already depend on the current behavior.

@mvdan
Copy link
Member

@mvdan mvdan commented Jun 2, 2021

We could always merge this early in the 1.18 cycle, and see if any users complain. I would hope that noone is depending on the existing inconsistent behavior - I've been wrong on that assumption before, but not always :)

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 2, 2021

Change https://golang.org/cl/324469 mentions this issue: encoding/json: call UnmarshalJSON() when decoding with a unamed struct.

@googollee
Copy link

@googollee googollee commented Jun 2, 2021

Removing the check of named structs looses the limitation. I think it should be fine with existing codes.

I made a change with a unit test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants