encoding/xml: serialization of `interface{}` attribute is unsupported, even when dynamic type implements relevant interfaces

[silkeh]

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

$ go version
go version go1.17.1 linux/amd64

Also reproduces on the playground (see below).

Does this issue reproduce with the latest release?

Yes.

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/silex/.cache/go-build"
GOENV="/home/silex/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/silex/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/silex/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib64/golang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib64/golang/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="x86_64-solus-linux-gcc"
CXX="x86_64-solus-linux-g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build858878585=/tmp/go-build -gno-record-gcc-switches"

What did you do?

When marshalling a struct with an interface field to an XML attribute, an error is returned. Simple example, extended example.

What did you expect to see?

No error, and for the simple example:

XML: <Test a="2009-11-10T23:00:00Z"></Test>

The extended example should return:

<Test a="hello" b="there"><a>hello</a><b>there</b></Test>    (err: %!s(<nil>))
<ITest><a>hello</a><b>there</b></ITest>                      (err: %!s(<nil>))
<ITest a="hello" b="there"><a>hello</a><b>there</b></ITest>  (err: %!s(<nil>))

What did you see instead?

Error: xml: unsupported type: time.Time

This error is incorrect (time.Time implements encoding.TextMarshaler), so should be supported.

Additionally, this does work with values instead of attributes, as shown in the extended example:

<Test a="hello" b="there"><a>hello</a><b>there</b></Test>    (err: %!s(<nil>))
<ITest><a>hello</a><b>there</b></ITest>                      (err: %!s(<nil>))
                                                             (err: xml: unsupported type: main.A)

[gopherbot]

Change https://golang.org/cl/352229 mentions this issue: encoding/xml: fix 'unsupported type' error on interface{} attributes

[mknyszek]

If your type actually implements the XML marshaller interface, why not just make the field of type encoding.TextMarshaler or xml.MarshalerAttr? If it could be one or the other, why not define a new interface like

type myInterface interface{
    xml.MarshalerAttr
    encoding.TextMarshaler
}

?

It might be a usability improvement to just have the code check if the empty interface implements one of these, but should it check this for any interface type? What's special about empty interface here?

CC @rsc via https://dev.golang.org/owners

[silkeh]

If your type actually implements the XML marshaller interface, why not just make the field of type encoding.TextMarshaler or xml.MarshalerAttr? If it could be one or the other, why not define a new interface like

type myInterface interface{
    xml.MarshalerAttr
    encoding.TextMarshaler
}

Well, my type does, but types like int and float don't implement those interfaces. Note that those do work properly, because the de-reference of the interface happens before trying to marshal them.

It might be a usability improvement to just have the code check if the empty interface implements one of these, but should it check this for any interface type? What's special about empty interface here?

What's special about the empty interface is that the behaviour is different depending on if it is a value (which works) or an attribute, and then also if the attribute is a basic type (also works) or not. This seems to be caused by the fact that the marshalling of XML values and attributes happens somewhat differently: marshalValue de-references any interfaces before checking if the supported interfaces are implemented (here), while marshalAttr does it after. The PR I submitted (#48625) simply moves this check up, which fixes this specific inconsistency (the de-referencing still works differently, so the behaviour is not identical).

Also note that the given error is incorrect, which seems like a proper bug to me. This is caused by the error being created after de-referencing the interface.

[mknyszek]

Oof. That does seem like an inconsistency, given that it checks for both basic types or if it's a value. Thanks for the additional info.

[gopherbot]

Change https://go.dev/cl/606495 mentions this issue: encoding: add full support for marshalers with pointer receivers