Skip to content

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

@mvdan

Description

@mvdan

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    FrozenDueToAgeNeedsDecisionFeedback is required from experts, contributors, and/or the community before a change can be made.

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions