encoding/xml: panics with Field index out of range when XMLName is in embedded struct

[SamWhited]

Please answer these questions before submitting your issue. Thanks!

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

go version devel +67f799c Tue Jul 26 00:18:42 2016 +0000 linux/amd64

But it also affects the version of 1.6 on the playground at the moment.

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

GOARCH="amd64"
GOBIN="/home/sam/bin"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/sam"
GORACE=""
GOROOT="/home/sam/src/go"
GOTOOLDIR="/home/sam/src/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build800221808=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

1. What did you do?

When attempting to unmarshal XML into a struct that is composed of another struct that has an XMLName field that has a field number greater than or equal to the number of fields in the original struct, finfo.Value panics because reflect tries to get the value of the field in the parent struct at the location of XMLName in the child struct (which has a field index that is out of bounds when applied to the parent struct).

For example, this contrived example panics:

package main

import (
    "encoding/xml"
)

type IQ struct {
    Type    string   `xml:"type,attr"`
    XMLName xml.Name `xml:"iq"`
}

func main() {
    resp := struct {
        IQ
    }{}
    xml.Unmarshal([]byte(`<iq/>`), &resp)
}

(Playground)

but if we move the fields around:

type IQ struct {
    XMLName xml.Name `xml:"iq"`
    Type    string   `xml:"type,attr"`
}

it works (Playground)

[SamWhited]

Now that it's not the middle of the night I decided to take a look at this; it appears that when composed fields are used in the parent structs metadata, their indexes are rewritten, but this was not done for xmlname fields (which are handled separately). This is a relatively easy fix (CL).

[gopherbot]

CL https://golang.org/cl/25242 mentions this issue.

[SamWhited]

I was going to post this on the CL, but I think GitHub or the dev list might be a better place (I never know where to post things). If this is incorrect, please let me know and I'll move it.

Currently the patch I have in the CL breaks two tests, but I think the behavior is wrong and the tests need to be changed. I'm not sure if this behavior is covered by the compatibility promise or not, but XMLNS fixes have been put in place in the past that changed the behavior of marshaling/unmarshaling with the rational that they were bug fixes and not part of the compatibility promise, so I'm hoping this will be simliar.

The issue is that right now if you unmarshal XML into a struct that contains an inner struct with an XMLName the XMLName will be matched against the XML correctly and the struct (and all its attributes) will be unmarshaled correctly however the XMLName field on the inner struct will not be set. If you go the other way and marshal that struct back to XML however the inner XML's XMLName tag will be used and the resulting XML will have the correct name and namespace.

Example — https://play.golang.org/p/bunS8dHMAO

This means that Marshal and Unmarshal are not inverse functions, which feels like a bug to me.

Thoughts? Is it okay to change this behavior so that the example above, instead of printing:

<outerstruct xmlns="urn:example:issue:16497"></outerstruct>
InnerStruct: {XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}
OuterStruct: {InnerStruct:{XMLName:{Space: Local:}}}
OuterOuterStruct: {OuterStruct:{InnerStruct:{XMLName:{Space: Local:}}}}

would print:

<outerstruct xmlns="urn:example:issue:16497"></outerstruct>
InnerStruct: {XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}
OuterStruct: {InnerStruct:{XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}}
OuterOuterStruct: {OuterStruct:{InnerStruct:{XMLName:{Space:urn:example:issue:16497 Local:outerstruct}}}}