Asymmetry in xml.Marshal/Unmarshal with anonymous fields

[Sam Whited]

Hi all,

I wanted to follow up on CL 25242
(https://go-review.googlesource.com/c/25242/) and ask the list about
expected behavior here. While implementing a fix for a panic in the
XML package when unmarshaling structs where the XMLName field was not
the first field (https://github.com/golang/go/issues/16497) I ran into
what I think is behavior that goes against the intention.

When unmarshalling structs with embedded structs the anonymous fields
are counted as part of the outermost struct, however, because XMLName
fields of type xml.Name are handled specially, they are skipped by the
code that does this. This means that Marshal and Unmarshal are not
symetric functions, which feels wrong to me:

https://play.golang.org/p/_Fclhx4vmv

It's been this way for a while, so it may be set in stone, but I don't
think it's expected and XML package changes have been made in the past
where they were considered bugs and not part of the API contract (as I
think this is).

Thoughts? Is this actually the desired behavior of the package? There
is specifically a test for this, but the behavior doesn't make a lot
of sense to me.

—Sam


--
Sam Whited
pub 4096R/54083AE104EA7AD3

[Sam Whited]

On Mon, Sep 19, 2016 at 9:56 AM, Sam Whited <s...@samwhited.com> wrote:
> This means that Marshal and Unmarshal are not
> symetric functions, which feels wrong to me:

Oops, sorry, this doesn't actually introduce asymmetry because Marshal
doesn't use the inner XMLName either. Either way, the behavior still
feels unexpected to me given that other anonymous structs are handled
differently.

[Sam Whited]

On Mon, Sep 19, 2016 at 9:56 AM, Sam Whited <s...@samwhited.com> wrote:

> I wanted to follow up on CL 25242
> (https://go-review.googlesource.com/c/25242/) and ask the list about
> expected behavior here.

Ping. Been a while so I wanted to bump to keep this on peoples radar;
I would sitll appreciate feedback, but won't bump again since I know
people are busy and the XML package may not be the top priority for
the 1.8 cycle.

Thanks,

[Nigel Tao]

On Tue, Sep 20, 2016 at 12:56 AM, Sam Whited <s...@samwhited.com> wrote:
> Thoughts? Is this actually the desired behavior of the package? There
> is specifically a test for this, but the behavior doesn't make a lot
> of sense to me.

The behavior might not be ideal, but unfortunately, we might be
constrained in what we can change. We have had to back out
encoding/xml namespace changes in the past, even if it made it more
consistent, as we broke existing programs. I can't remember the
details, but I do know that x/net/webdav has its own internal fork of
encoding/xml for this reason. There might be more details in issues
like https://github.com/golang/go/issues/13400 and
https://github.com/golang/go/issues/14407

[Sam Whited]

On Thu, Oct 6, 2016 at 6:50 AM, Nigel Tao <nige...@golang.org> wrote:
> The behavior might not be ideal, but unfortunately, we might be
> constrained in what we can change. We have had to back out
> encoding/xml namespace changes in the past, even if it made it more
> consistent, as we broke existing programs.

Thanks for the follow up; I beleive there have also been changes to
the XML package which could potentially break things but it was deemed
that the behavior was a bug (and not documented) and therefore not a
part of the compatibility promise. I'm not sure which category this
falls under. I have a hard time imagining that this change would break
existing programs, but I suppose it is possible and we couldn't be
sure one way or the other.