code review 33140043: encoding/xml: unmarshal into interfaces

encoding/xml: unmarshal into interfaces

Fixes issue 6836.

[josharian, 2013-12-14 02:22:42]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2013-12-18 17:56:42]

I am not convinced about this. This is different from the behavior for pointers.
If you had a pointer it would be replaced with a new instance, at least
according to the docs:

    Unmarshal maps an XML element to a pointer by setting the pointer to a
    freshly allocated value and then mapping the element to that value.

I am not convinced that the pointer and interface case should diverge in this
subtle way. Not supporting interfaces at all is at least less subtle.

[r, 2013-12-18 18:39:24]

On a related note, in retrospect I wish I had not added support for
interfaces in encoding/gob.

-rob

[josharian, 2013-12-19 22:32:12]

Issue 6836 was filed by a colleague of mine. I'll talk with him about possible
workarounds when he returns from vacation in mid-Jan 2014. In the meantime,
please leave this CL open.

[gobot, 2013-12-20 16:26:10]

Replacing golang-dev with golang-codereviews.

[mike10, 2014-02-12 01:39:22]

On 2013/12/18 17:56:42, rsc wrote:
> I am not convinced about this. This is different from the behavior for
pointers.
> If you had a pointer it would be replaced with a new instance, at least
> according to the docs:
> 
>     Unmarshal maps an XML element to a pointer by setting the pointer to a
>     freshly allocated value and then mapping the element to that value.
> 
> I am not convinced that the pointer and interface case should diverge in this
> subtle way. Not supporting interfaces at all is at least less subtle.

I agree it's inconsistent but interfaces in Go are nothing if not subtle :-)

I think that supporting unmarshaling through interfaces enables what I consider
to be a fairly powerful use case with very little code.  Imagine that I have a
generic envelope for an object.  I want to keep the envelope agnostic of the
object contained within it.  This obviously means using an interface. 
Marshaling works plainly enough because it traverses the interface pointer to
access the metadata in the referenced object.

Before unmarshaling, I can use other (e.g. header) information to identify the
desired type and instantiate the appropriate container to receive the payload. 
In the example I included in http://code.google.com/p/go/issues/detail?id=6836,
I instantiate the generic envelope "Result" with a pre-filled reference to a
type-appropriate "Address" object.

Honestly I'm not sure what a good alternative would be.  I think I'd either be
stuck writing custom unmarshalers or doing 2 levels of encoding (one for
envelope, and one for payload).  Hopefully you see the utility of this construct
in a generic message passing framework.

I'm surprised that you would want to support marshaling interfaces but not
unmarshaling them.  http://blog.golang.org/json-and-go#TOC_4. is what led me to
believe this is a reasonable supported behavior.  Obviously that was with JSON
but XML seems equally straightfoward.

hope this helps,
mike

[mike10, 2014-02-12 01:44:14]

the syntax highlighter didn't get the URL fragment right.  Here's the correct
link:
http://blog.golang.org/json-and-go#TOC_4%2e

[josharian, 2014-02-13 01:17:34]

PTAL

No code changes, just trying to get this to show up as "waiting for reviewer" in
the dashboard now that Mike (who filed the original issue) has weighed in.

[rsc, 2014-04-08 18:55:04]

LGTM

I did not realize that (1) this is what encoding/json does and (2) the code
in this CL was lifted from json. Next time lead with that. :-)

[rsc, 2014-04-08 18:55:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=1d51e13e0db0 ***

encoding/xml: unmarshal into interfaces

Fixes issue 6836.

LGTM=rsc
R=golang-codereviews, rsc, r, mike
CC=golang-codereviews
https://codereview.appspot.com/33140043

Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-04-08 19:08:56]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/5bc4422f7d7e8fee689d13efda30a563eac4f284

[mike10, 2014-04-09 01:27:54]

On 2014/04/08 19:08:56, gobot wrote:
> This CL appears to have broken the plan9-386-cnielsen builder.
> See http://build.golang.org/log/5bc4422f7d7e8fee689d13efda30a563eac4f284

The log shows that the tests were killed due to timeout, but the encoding/xml
tests finished in 0.084s so i'm inclined to think it was unrelated to this
patch.

[mike10, 2014-04-09 01:32:39]

On 2014/04/08 18:55:04, rsc wrote:
> LGTM
> 
> I did not realize that (1) this is what encoding/json does and (2) the code
> in this CL was lifted from json. Next time lead with that. :-)

rsc, thanks very much for including this.  Sorry it took so long to communicate.
 I did in fact lead off with this in my bug report
(https://code.google.com/p/go/issues/detail?id=6836) but for some reason this
patch submission and the bug report aren't very closely integrated and the
context was lost.  i'll make sure we are clear next time.

thanks again!