New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Ignore tags in struct type conversions #16085

Closed
griesemer opened this Issue Jun 16, 2016 · 22 comments

Comments

Projects
None yet
9 participants
@griesemer
Contributor

griesemer commented Jun 16, 2016

Abstract
This document proposes to relax struct conversions such that struct tags are ignored. An alternative to the proposal is to add a new function reflect.StructCopy that could be used instead.

1. Background
The spec (https://codereview.appspot.com/1698043) and corresponding implementation change (https://golang.org/cl/1667048) submitted almost exactly six years ago made struct tags (https://golang.org/ref/spec#Struct_types) an integral part of a struct type by including them in the definition of struct type identity (https://golang.org/ref/spec#Type_identity) and indirectly in struct type conversions (https://golang.org/ref/spec#Conversions).

In retrospect, this change may have been overly restrictive with respect to its impact on struct conversions, given the way struct tag use has evolved over the years. A common scenario is the conversion of struct data coming from, say a database, to an equivalent (identical but for its tags) struct that can be JSON-encoded, with the JSON encoding defined by the respective struct tags. For an example of such a type, see https://github.com/golang/text/blob/master/unicode/cldr/xml.go#L6.

The way struct conversions are defined, it is not currently possible to convert a value from one struct type to an equivalent one. Instead, every field must be copied manually, which leads to more source text, and less readable and possibly less efficient code. The code must also be adjusted every time the involved struct types change.

#6858 discusses this in more detail. rsc@golang and r@golang suggest that we might be able to relax the rules for structs such that struct tags are ignored for conversions, but not for struct identity.

2. Proposal
The spec states a set of rules for conversions. The following rules apply to conversions of struct values (among others):

A non-constant value x can be converted to type T if:

  • x's type and T have identical underlying types
  • x's type and T are unnamed pointer types and their pointer base types have identical underlying types

The proposal is to change these two rules to:

A non-constant value x can be converted to type T if:

  • x's type and T have identical underlying types if struct tags are ignored (recursively)
  • x's type and T are unnamed pointer types and their pointer base types have identical underlying types if struct tags are ignored (recursively)

Additionally, package reflect is adjusted (Type.ConvertibleTo, Value.Convert) to match this language change.

In other words, type identity of structs remains unchanged, but for the purpose of struct conversions, type identity is relaxed such that struct tags are ignored.

3. Impact
This is is a backward-compatible language change since it loosens an existing restriction: Any existing code will continue to compile with the same meaning (*), and some code that currently is invalid will become valid.

Programs that manually copy all fields from one struct to another struct with identical type but for the (type name and) tags, will be able to use a single struct conversion instead.

More importantly, with this change two different (type) views of the same struct value become possible via pointers of different types. For instance, given:

type jsonPerson struct {
    name `json:"name"`
}

type xmlPerson struct {
    name `xml:"name"`
}

we will be able to access a value of *jsonPerson type

person := new(jsonPerson)
// some code that populates person

as a *Txml:

alias := (*xmlPerson)(&person)
// some code that uses alias

This may eliminate the need to copy struct values just to change the tags.

Type identity and conversion tests are also available programmatically, via the reflect package. The operations of Type.ConvertibleTo and Value.Convert will be relaxed for structs with different (or absent) tags:

Type.ConvertibleTo will return true for some arguments where it currently returns false. This may change the behavior of programs depending on this method.

Value.Convert will convert struct values for which the operation panicked before. This will only affect programs that relied on (recovered from) that panic.

(*) r@golang points out that a program that is using tags to prevent (accidental or deliberate) struct conversion would lose that mechanism. Interestingly, package reflect appears to make such use (see type rtype), but iant@golang points out that one could obtain the same effect by adding differently typed zero-sized fields to the respective structs.

4. Discussion
From a language spec point of view, changing struct type identity (rather than struct conversions only) superficially looks like a simpler, cleaner, and more consistent change: For one, it simplifies the spec, while only changing struct conversions requires adding an additional rule.

iant@golang points out (#11661) that leaving struct identity in place doesn’t make much difference in practice: It is already impossible to assign or implicitly convert between two differently named struct types. Unnamed structs are rare, and if accidental conversion is an issue, one can always introduce a named struct.

On the other hand, runtime type descriptors (used by reflect.Type, interfaces, etc) are canonical, so identical types have the same type descriptor. The descriptor provides struct field tags, so identical types must have identical tags. Thus we cannot at this stage separate struct field tags from the notion of type identity.

To summarize: Relaxing struct conversions only but leaving struct type identity unchanged is sufficient to enable one kind of data conversion that is currently overly tedious, and it doesn’t require larger and more fundamental changes to the run time. The change may cause a hopefully very small set of programs, which depend on package reflect’s conversion-related API, to behave differently.

5. Open question
Should tags be ignored at the top-level of a struct only, or recursively all the way down? For instance, given:

type T1 struct {
    x int
    p *struct {
        name string `foo`
    }
}

type T2 struct {
    x int
    p *struct {
        name string `bar`
    }
}

var t1 T1

Should the conversion T2(t1) be legal? If tags are only ignored for the fields of T1 and T2, conversion is not permitted since the tags attached to the type of the p field are different. Alternatively, if tags are ignored recursively, conversion is permitted.

On the other hand, if the types were defined as:

type T1 struct {
    x int
    p *P1
}

type T2 struct {
    x int
    p *P2
}

where P1 and P2 are identical structs but for their tags, the conversion would not be permitted either way since the p fields have different types and thus T1 and T2 have different underlying types.

The proposal suggests to ignore tags recursively, “all the way down”. This seems to be the more sensible approach given the stated goal, which is to make it easier to convert from one struct type to another, equivalent type with different tags. For a typical example where this matters, see https://play.golang.org/p/U73K50YXYk.

Furthermore, it is always possible to prevent unwanted conversions by introducing named types, but it would not be possible to enable those conversions otherwise.

On the other hand, the current implementation of reflect.Value.Convert will make recursive ignoring of struct tags more complicated and expensive. crawshaw@golang points out that one could easily use a cache inside the reflect package if necessary for performance.

6. Implementation
Tentative CL forthcoming, including spec, compiler, go/types, and reflect changes.

7. Alternatives to the language change
Even a backward-compatible language change needs to meet a high bar before it can be considered. It is not yet clear that this proposal satisfies that criteria.

One alternative is to do nothing. That has the advantage of not breaking anything and also doesn’t require any implementation effort on the language/library side. But it means that in some cases structs have to be explicitly converted through field-by-field assignment.

Another alternative that actually addresses the problem is to provide a library function. For instance, package reflect could provide a new function

func CopyStruct(dst, src Value, mode Mode)

which could be used to copy struct values that have identical types but for struct tags. A mode argument might be used to control deep or shallow copy, and perhaps other modalities. A deep copy (following pointers) would be a useful feature that the spec change by itself does not enable.

The cost of using a CopyStruct function instead of a direct struct conversion is the need to create two reflect.Values, invoking CopyStruct, and (inside CopyStruct) the cost to verify type identity but for tags. Copying the actual data needs to be done both in CopyStruct but also with a direct (language-based) conversion. The type verification is likely the most expensive step but identity of struct types (with tags ignored) could be cached. On the other hand, adonovan@golang points out that the added cost may not matter in significant ways since these kinds of struct copies often sit between a database request and an HTTP response.

The functional difference between the proposed spec change and a new reflect.CopyStruct function is that with CopyStruct an actual copy has to take place (as is the case now). The spec change on the other hand permits both approaches: a (copying) conversion of struct values, or pointers to different struct types that point to the same struct value via a pointer conversion. The latter may eliminate a copy of data in the first place.

@griesemer griesemer added the Proposal label Jun 16, 2016

@griesemer griesemer added this to the Go1.8Maybe milestone Jun 16, 2016

@griesemer griesemer self-assigned this Jun 16, 2016

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Jun 16, 2016

Contributor

For an implementation see: https://go-review.googlesource.com/24190.

Contributor

griesemer commented Jun 16, 2016

For an implementation see: https://go-review.googlesource.com/24190.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Jun 16, 2016

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

@griesemer griesemer modified the milestones: Proposal, Go1.8Maybe Jun 16, 2016

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Jun 17, 2016

Contributor

Are you planning to send a formal proposal as per the process? https://github.com/golang/proposal

Contributor

adg commented Jun 17, 2016

Are you planning to send a formal proposal as per the process? https://github.com/golang/proposal

gopherbot pushed a commit to golang/proposal that referenced this issue Jun 17, 2016

design: add design doc for relaxed struct conversions
For golang/go#16085.

Change-Id: I5ec55defa201dfc33774e8d80edd4070c1d3d4de
Reviewed-on: https://go-review.googlesource.com/24192
Reviewed-by: Robert Griesemer <gri@golang.org>
@adg

This comment has been minimized.

Show comment
Hide comment
@mewmew

This comment has been minimized.

Show comment
Hide comment
@mewmew

mewmew Jun 18, 2016

Contributor

Spotted a few minor typos, as illustrated by the following "diff".

 For instance, given:

    type jsonPerson struct {
        name `json:"name"`
    }

    type xmlPerson struct {
        name `xml:"name"`
    }

 we will be able to access a value of `*jsonPerson` type

    person := new(jsonPerson)
    // some code that populates person

- as a `*Txml```:
+ as a `*xmlPerson```:

-   alias := (*xmlPerson)(&person)
+   alias := (*xmlPerson)(person)
    // some code that uses alias

Note, person is already a pointer to jsonPerson as it is allocated with new, thus &person should not be required.

Contributor

mewmew commented Jun 18, 2016

Spotted a few minor typos, as illustrated by the following "diff".

 For instance, given:

    type jsonPerson struct {
        name `json:"name"`
    }

    type xmlPerson struct {
        name `xml:"name"`
    }

 we will be able to access a value of `*jsonPerson` type

    person := new(jsonPerson)
    // some code that populates person

- as a `*Txml```:
+ as a `*xmlPerson```:

-   alias := (*xmlPerson)(&person)
+   alias := (*xmlPerson)(person)
    // some code that uses alias

Note, person is already a pointer to jsonPerson as it is allocated with new, thus &person should not be required.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Jun 20, 2016

Contributor

@mewmew Thanks, fixed.

Contributor

griesemer commented Jun 20, 2016

@mewmew Thanks, fixed.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Sep 15, 2016

Contributor

Robert, can you please implement this for the Go 1.8 cycle too? Thanks.

Contributor

rsc commented Sep 15, 2016

Robert, can you please implement this for the Go 1.8 cycle too? Thanks.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Sep 15, 2016

Contributor

@rsc I have a pending CL, probably 90% there. There's a question as to the semantics change in reflect: Some conversions that were rejected before are now permissible and this observable via reflect. I assume we just document this?

Contributor

griesemer commented Sep 15, 2016

@rsc I have a pending CL, probably 90% there. There's a question as to the semantics change in reflect: Some conversions that were rejected before are now permissible and this observable via reflect. I assume we just document this?

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Sep 15, 2016

Contributor

It sounds like you are saying that reflect.Value.Convert must be updated. That sounds fine.

Contributor

rsc commented Sep 15, 2016

It sounds like you are saying that reflect.Value.Convert must be updated. That sounds fine.

@griesemer

This comment has been minimized.

Show comment
Hide comment
Contributor

griesemer commented Sep 15, 2016

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Oct 3, 2016

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

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Oct 3, 2016

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

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Oct 3, 2016

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

gopherbot pushed a commit that referenced this issue Oct 4, 2016

spec: ignore struct tags when converting structs
This is a backwards-compatible language change.

Per the proposal (#16085), the rules for conversions are relaxed
such that struct tags in any of the structs involved in the conversion
are ignored (recursively).

Because this is loosening the existing rules, code that compiled so
far will continue to compile.

For #16085.
Fixes #6858.

Change-Id: I0feef651582db5f23046a2331fc3f179ae577c45
Reviewed-on: https://go-review.googlesource.com/24190
Reviewed-by: Russ Cox <rsc@golang.org>

gopherbot pushed a commit that referenced this issue Oct 4, 2016

cmd/compile: ignore struct tags when converting structs
Implementation of spec change https://golang.org/cl/24190/.

For #16085.

Change-Id: Id71ef29af5031b073e8be163f578d1bb768ff97a
Reviewed-on: https://go-review.googlesource.com/30169
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

gopherbot pushed a commit that referenced this issue Oct 4, 2016

go/types: ignore struct tags when converting structs
Implementation of spec change https://golang.org/cl/24190/.

For #16085.

Change-Id: I17bbbce38d98a169bc64e84983a7ebfe7142f6e9
Reviewed-on: https://go-review.googlesource.com/30190
Reviewed-by: Alan Donovan <adonovan@google.com>

gopherbot pushed a commit that referenced this issue Oct 4, 2016

reflect: ignore struct tags when converting structs
Implementation of spec change https://golang.org/cl/24190/.

For #16085.

Change-Id: Ib7cb513354269282dfad663c7d2c6e624149f3cd
Reviewed-on: https://go-review.googlesource.com/30191
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Oct 4, 2016

Contributor

The spec has been adjusted and this has been implemented.

Contributor

griesemer commented Oct 4, 2016

The spec has been adjusted and this has been implemented.

@griesemer griesemer closed this Oct 4, 2016

@andlabs

This comment has been minimized.

Show comment
Hide comment
@andlabs

andlabs Oct 4, 2016

Contributor

Yet again I am late to the party, and since I don't see this asked anywhere, including the proposal doc: so my late question is: how does this now-implemented proposal differ from simply having two tags with different keys on the same field of the same struct? That is, how is

type jsonPerson struct {
    name `json:"name"`
}

type xmlPerson struct {
    name `xml:"name"`
}

different from

type Person struct {
    name `json:"name" xml:"name"`
}

for the purposes of behavioral equivalency? reflect.StrutTag already works this way. Is the idea two conflicting tags with the same key?

Contributor

andlabs commented Oct 4, 2016

Yet again I am late to the party, and since I don't see this asked anywhere, including the proposal doc: so my late question is: how does this now-implemented proposal differ from simply having two tags with different keys on the same field of the same struct? That is, how is

type jsonPerson struct {
    name `json:"name"`
}

type xmlPerson struct {
    name `xml:"name"`
}

different from

type Person struct {
    name `json:"name" xml:"name"`
}

for the purposes of behavioral equivalency? reflect.StrutTag already works this way. Is the idea two conflicting tags with the same key?

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Oct 4, 2016

Contributor
Contributor

adg commented Oct 4, 2016

@andlabs

This comment has been minimized.

Show comment
Hide comment
@andlabs

andlabs Oct 4, 2016

Contributor

Hm, I didn't think of that one. Thanks.

Contributor

andlabs commented Oct 4, 2016

Hm, I didn't think of that one. Thanks.

@c4milo

This comment has been minimized.

Show comment
Hide comment
@c4milo

c4milo Nov 2, 2016

Member

Can this be included for 1.8 or 1.9? It's becoming a nightmare to work with gRPC without being able to do struct type conversions due to the field tags being different.

Member

c4milo commented Nov 2, 2016

Can this be included for 1.8 or 1.9? It's becoming a nightmare to work with gRPC without being able to do struct type conversions due to the field tags being different.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Nov 2, 2016

Contributor

This is already in tip and will be in 1.8.

  • gri

On Wed, Nov 2, 2016 at 3:16 PM, Camilo Aguilar notifications@github.com
wrote:

Can this be included for 1.8 or 1.9? It's becoming a nightmare to work
with gRPC without being able to do struct type conversions due to the field
tags being different.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#16085 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIIkT72etocind8x2eUrs0I6pxoOV-j4ks5q6QvNgaJpZM4I31RU
.

Contributor

griesemer commented Nov 2, 2016

This is already in tip and will be in 1.8.

  • gri

On Wed, Nov 2, 2016 at 3:16 PM, Camilo Aguilar notifications@github.com
wrote:

Can this be included for 1.8 or 1.9? It's becoming a nightmare to work
with gRPC without being able to do struct type conversions due to the field
tags being different.


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#16085 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIIkT72etocind8x2eUrs0I6pxoOV-j4ks5q6QvNgaJpZM4I31RU
.

@fatih

This comment has been minimized.

Show comment
Hide comment
@fatih

fatih Jan 13, 2017

Member

@griesemer I see that https://go-review.googlesource.com/#/c/24190/ is the change implementing this. However while reading the spec today I've countered this line:

The tags are made visible through a reflection interface and take part in type identity for structs but are otherwise ignored.

Here is a picture of it:

screen-shot-2017-01-13-6-37-30_pm

It seems like this part was not changed: https://github.com/golang/go/blob/release-branch.go1.8/doc/go_spec.html#L1048 I might be wrong but tags are not anymore a part of the type identity right? Should we change that line as well? Feel free to fix me if that is not the case. Thanks!

Member

fatih commented Jan 13, 2017

@griesemer I see that https://go-review.googlesource.com/#/c/24190/ is the change implementing this. However while reading the spec today I've countered this line:

The tags are made visible through a reflection interface and take part in type identity for structs but are otherwise ignored.

Here is a picture of it:

screen-shot-2017-01-13-6-37-30_pm

It seems like this part was not changed: https://github.com/golang/go/blob/release-branch.go1.8/doc/go_spec.html#L1048 I might be wrong but tags are not anymore a part of the type identity right? Should we change that line as well? Feel free to fix me if that is not the case. Thanks!

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 13, 2017

Contributor

Type identity didn't change. What changed is the set of valid conversions.

Contributor

rsc commented Jan 13, 2017

Type identity didn't change. What changed is the set of valid conversions.

@fatih

This comment has been minimized.

Show comment
Hide comment
@fatih

fatih Jan 14, 2017

Member

Thanks @rsc for the clarification.

Member

fatih commented Jan 14, 2017

Thanks @rsc for the clarification.

gopherbot pushed a commit to golang/gofrontend that referenced this issue May 18, 2017

compiler: ignore struct field tags for type conversion
Go 1.8 includes a language change (https://golang.org/doc/go1.8#language):
in an explicit conversion from one struct type to another, any field
tags are ignored.

This CL implements this language change in the gofrontend.  The tests
for this are in the gc testsuite, which will be copied into the gccgo
repository in due course.

Updates golang/go#16085.

Change-Id: I64e49f103efced41a55757e2cb665f6c1098db80
Reviewed-on: https://go-review.googlesource.com/43614
Reviewed-by: Than McIntosh <thanm@google.com>

pmatos pushed a commit to LinkiTools/gcc that referenced this issue May 18, 2017

ian
compiler: ignore struct field tags for type conversion
    
    Go 1.8 includes a language change (https://golang.org/doc/go1.8#language):
    in an explicit conversion from one struct type to another, any field
    tags are ignored.
    
    This CL implements this language change in the gofrontend.  The tests
    for this are in the gc testsuite, which will be copied into the gccgo
    repository in due course.
    
    Updates golang/go#16085.
    
    Reviewed-on: https://go-review.googlesource.com/43614


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@248248 138bc75d-0d04-0410-961f-82ee72b054a4

pmatos pushed a commit to LinkiTools/gcc that referenced this issue May 18, 2017

ian
compiler: ignore struct field tags for type conversion
    
    Go 1.8 includes a language change (https://golang.org/doc/go1.8#language):
    in an explicit conversion from one struct type to another, any field
    tags are ignored.
    
    This CL implements this language change in the gofrontend.  The tests
    for this are in the gc testsuite, which will be copied into the gccgo
    repository in due course.
    
    Updates golang/go#16085.
    
    Reviewed-on: https://go-review.googlesource.com/43614


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gcc-7-branch@248249 138bc75d-0d04-0410-961f-82ee72b054a4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.