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/xml: handling of anonymous pointer fields is broken #27240

Closed
mvdan opened this issue Aug 26, 2018 · 5 comments
Closed

encoding/xml: handling of anonymous pointer fields is broken #27240

mvdan opened this issue Aug 26, 2018 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Aug 26, 2018

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/FUox06lbRVn

As one can see, they're simply skipped. encoding/xml does something slightly different: https://play.golang.org/p/KWYc_IQR7p5

The 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:

  1. Behave like encoding/json, ignoring anonymous pointer fields which are nil.
  2. Keep the existing behavior, encoding zero-values without modifying the anonymous fields in-place.

I personally strongly prefer option 1, as that would make encoding/xml consistent with encoding/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:

Anonymous struct fields are usually marshaled as if their inner exported fields were fields in the outer struct, subject to the usual Go visibility rules amended as described in the next paragraph.

an anonymous struct field is handled as if the fields of its value were part of the outer struct.

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

@mvdan mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 26, 2018
@mvdan mvdan added this to the Go1.12 milestone Aug 26, 2018
@mvdan
Copy link
Member Author

mvdan commented Aug 26, 2018

Please also note that handling of these is broken in encoding/json at tip at the moment, which is how I ended up digging this bug. A fix for JSON is already up: https://go-review.googlesource.com/c/go/+/131376

@SamWhited
Copy link
Member

Behave like encoding/json, ignoring anonymous pointer fields which are nil.

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 *T.A string is effectively promoted to A *string in my mind (which would be ignored if the pointer is nil).

@mvdan
Copy link
Member Author

mvdan commented Aug 26, 2018

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.

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@mvdan
Copy link
Member Author

mvdan commented Sep 24, 2019

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 :)

@gopherbot
Copy link

Change https://golang.org/cl/196809 mentions this issue: encoding/xml: don't initialize struct fields when encoding

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@mvdan mvdan modified the milestones: Backlog, Go1.15 Nov 11, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Go1.16 May 19, 2020
@golang golang locked and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants