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: custom UnmarshalTypeError no longer has struct context #27464

Closed
stephane-moreau opened this issue Sep 3, 2018 · 9 comments

Comments

Projects
None yet
6 participants
@stephane-moreau
Copy link

commented Sep 3, 2018

Please answer these questions before submitting your issue. Thanks!

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

Works with: go version go1.10.3 windows/amd64
Fail with: go version go1.11 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\steph\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\dev\source\catapult/go
set GORACE=
set GOROOT=C:\dev\tools\go1.10.3
set GOTMPDIR=
set GOTOOLDIR=C:\dev\tools\go1.10.3\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\steph\AppData\Local\Temp\go-build387032470=/tmp/go-build -gno-record-gcc-switches

(tested too on Mac OSX)

What did you do?

This used to wokr when using go.lang prior to version 1.11
https://play.golang.org/p/kbRsWxsQwE0

Looks like the way error handling changed in json.Unmarshal did change the errorContext for custom unmarshaller

What did you expect to see?

PS C:\dev\source\test\marhsal> $env:GOROOT="c:\dev\tools\go1.11"
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.11\bin\go version
go version go1.11 windows/amd64
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.11\bin\go run .\main.go
FIRST: An error was detected when parsing field:
-----------
SECOND: An error was detected when parsing field:

What did you see instead?

PS C:\dev\source\test\marhsal> $env:GOROOT="c:\dev\tools\go1.10.3"
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.10.3\bin\go version
go version go1.10.3 windows/amd64
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.10.3\bin\go run .\main.go
FIRST: An error was detected when parsing field: foo_tag
-----------
SECOND: An error was detected when parsing field: foo_tag
@myitcv

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

cc @mvdan

@gopherbot

This comment has been minimized.

Copy link

commented Sep 3, 2018

Change https://golang.org/cl/132955 mentions this issue: encoding/json: recover saved error context when unmarshalling

@mvdan

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

On go version devel +579768e078 Fri Aug 31 19:05:37 2018 +0000 linux/amd64 I get:

FIRST: An error was detected when parsing field: foo_tag
-----------
SECOND: An error was detected when parsing field:

I think the first case was fixed in 21e85c2.

The second one doesn't look to be fixed yet. This was likely introduced in a refactor that was merged in for 1.11 - in 74a92b8.

Certainly needs a fix. The other 1.11 json decoding error regression is being backported into 1.11.1, so I presume this one should too. /cc @bradfitz

@iand

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

Looking at this in more detail, I have doubts about the severity of the problem. It is only caused when the custom UnmarshalJSON method returns an empty UnmarshalTypeError. Changing the error type returned results in no regression from 1.10.3 (based on running in playground compared with tip): https://play.golang.org/p/PfSrNduIVmY

@iand

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

Returning an UnmarshalTypeError only seems to have worked by accident in 1.10.3. The error returned by Unmarshal has mostly empty field values with only the Struct and Field fields populated. See https://play.golang.org/p/B6g9HMMmOQ4

@stephane-moreau

This comment has been minimized.

Copy link
Author

commented Sep 3, 2018

Thanks all for your inputs,

@iand: This was a simplified code sample where other fields where populated
I was only expecting Struct and Field name to be populated by the json decoder,

as a side note : I Just tested this in 1.9.2 and 1.10 on windows released versions and both gave foo_tag as the field being the culprit of the unmarshaling error

If there is another mean to get the errorContext from decoder, I would be more than happy to use it instead of this.

@andybons andybons added the NeedsFix label Sep 4, 2018

@andybons andybons added this to the Go1.12 milestone Sep 4, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

@mvdan mvdan changed the title Potential regression in json custom unmarshalling error handling encoding/json: custom UnmarshalTypeError no longer has struct context Sep 6, 2018

@gopherbot gopherbot closed this in 22afb35 Sep 6, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@gopherbot please backport to 1.11.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 20, 2018

Backport issue(s) opened: #29364 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.