-
Notifications
You must be signed in to change notification settings - Fork 18k
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/xml: handling of anonymous pointer fields is broken #27240
Comments
Please also note that handling of these is broken in |
This is how I read the docs anyways and seems sane to me. It also makes the package more internally consistent: when the children of an anonymous field are promoted they're still behind a pointer so |
Agreed. Moreover, trying to access the embedded field in plain Go would never give you an empty value - it would panic - so I think the current behavior is unexpected. |
I ended up dropping the ball on this a bit. The good news is that I had a mostly working fix last year, which I'm going to rebase, clean up a bit, and send. I still think that option 1 is the correct solution here, which is the fix I implemented. I'm not sure if this bug is worth fixing, or if it's the correct fix, but it's not really doing anything sitting in a local branch for a year :) |
Change https://golang.org/cl/196809 mentions this issue: |
This happens on both Go 1.10.4 and
go version devel +c21ba224ec Sat Aug 25 23:59:43 2018 +0000 linux/amd64
.This playground link shows what
encoding/json
does when it encounters anonymous pointer fields which are nil: https://play.golang.org/p/FUox06lbRVnAs one can see, they're simply skipped.
encoding/xml
does something slightly different: https://play.golang.org/p/KWYc_IQR7p5The XML package tries to print the zero values instead. Which seems fine in theory, but here's the first weird behavior - as you can see via the print statements, the encoded value has been modified in-place.
But here's an even worse problem - if the encoded value isn't addressable, encoding panics via reflect: https://play.golang.org/p/7ORK8Vw8o6Q
I think these two problems need to be fixed. I see two possibilities:
encoding/json
, ignoring anonymous pointer fields which are nil.I personally strongly prefer option 1, as that would make
encoding/xml
consistent withencoding/json
. Both are documented in the exact same way when it comes to anonymous fields, so I believe consistency should be important. Here are the relevant docs for JSON and XML respectively:The only problem with option 1 is that this may break some existing programs, in which case option 2 may be better in Go 1.x. Although it would still be possible to achieve the existing behavior, if
encoding/xml
users changed their code to ensure that there aren't any nil anonymous pointer fields.Whatever we settle for, the fix should be pretty simple - happy to put in the work.
cc @rsc @niemeyer @SamWhited @rogpeppe
The text was updated successfully, but these errors were encountered: