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: Multiple issues with decoding of nulls into ",string" tagged fields. #22177

Open
ainar-g opened this Issue Oct 8, 2017 · 2 comments

Comments

Projects
None yet
3 participants
@ainar-g
Contributor

ainar-g commented Oct 8, 2017

I've been playing around with the megacheck tool, and found that NullTestStrings type in encoding/json's tests is unused. It seems to be aimed at checking decoding of nulls into ",string" fields, but there were no tests, which is the first issue. The second is that if you create one, it fails: https://play.golang.org/p/5XjdhoDDkq.

--- FAIL: TestUnmarshalStringNulls (0.00s)
	main.go:126: Unmarshal of null values failed: MustNotUnmarshalJSON was used
	main.go:138: Unmarshal of null did not clear nulls.Map
	main.go:141: Unmarshal of null did not clear nulls.Slice
	main.go:144: Unmarshal of null did not clear nulls.Interface
	main.go:147: Unmarshal of null did not clear nulls.PRaw
	main.go:150: Unmarshal of null did not clear nulls.PTime
	main.go:153: Unmarshal of null did not clear nulls.PBigInt
	main.go:156: Unmarshal of null did not clear nulls.PText
	main.go:159: Unmarshal of null did not clear nulls.PBuffer
	main.go:162: Unmarshal of null did not clear nulls.PStruct
	main.go:166: Unmarshal of RawMessage null did not record null: 123
FAIL

It seems to me that there are some inconsistencies between decoding null into a simple field and decoding "null" into a ",string" field.

Formalities:

$ go version
go version devel +ca360c3 Sat Oct 7 22:12:36 2017 +0000 linux/amd64
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ainar/go"
GORACE=""
GOROOT="/home/ainar/go/gotip"
GOTOOLDIR="/home/ainar/go/gotip/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build220310604=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Oct 9, 2017

Thanks for this - seems like we should either remove that struct, or add tests for the behavior as it currently stands?

@ainar-g

This comment has been minimized.

Contributor

ainar-g commented Oct 9, 2017

I can mail a CL that adds the test or removes the struct, but I think that the fact that null with a simple field and "null" with a ",string" field behave differently is rather concerning. I think the behaviour should be the same. This will bite someone sooner or later. If you are afraid that it will break someone's code, I think this is covered by the promise:

Bugs. If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment