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

spec: field tags have to be identical when casting #6858

Closed
lukescott opened this Issue Dec 1, 2013 · 28 comments

Comments

Projects
None yet
@lukescott

lukescott commented Dec 1, 2013

Run this:

package main

type Message struct {
    Id   int    `db:"id"`
    Text string `db:"text"`
}

type Message2 struct {
    Id   int    `json:"id" xml:"id,attr"`
    Text string `json:"text"`
}

func main() {
    msg := Message{1, "Hello"}
    msg2 := Message2(msg)
    _ = msg2
}

What is the expected output?

[no output]

What do you see instead?

cannot convert msg (type Message) to type Message2

Which compiler are you using (5g, 6g, 8g, gccgo)?

Which version are you using?  (run 'go version')

go version devel +d744da8c8cbf Wed Sep 25 16:18:33 2013 -0400 darwin/amd64

Please provide any additional information below.

I'm trying to decouple my database layer from my service layer into separate packages.
Tags in one package are only relevant to that package. What I'd like to do is re-define
the struct in my service layer and cast it from the database layer. The fields inside
the struct are otherwise exactly the same, besides the tags.

Unfortunately it won't cast unless the tags are also exactly the same. The only solution
I have at this point is put the structs in a "common" package that both the
database and service layers share. But then I'm mixing tags...
@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Dec 1, 2013

Member

Comment 1:

this is working as intended, or rather, working according to the spec.
http://golang.org/ref/spec#Type_identity
are you suggesting a language change?
Member

minux commented Dec 1, 2013

Comment 1:

this is working as intended, or rather, working according to the spec.
http://golang.org/ref/spec#Type_identity
are you suggesting a language change?
@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Dec 1, 2013

Contributor

Comment 2:

I think we could consider a language change here.  The current definition is
conservative.  We could loosen it without causing any valid programs to become invalid. 
Considering the way that tag use has evolved, it seems to me that there is a valid
argument for loosening it.

Labels changed: added languagechange.

Contributor

ianlancetaylor commented Dec 1, 2013

Comment 2:

I think we could consider a language change here.  The current definition is
conservative.  We could loosen it without causing any valid programs to become invalid. 
Considering the way that tag use has evolved, it seems to me that there is a valid
argument for loosening it.

Labels changed: added languagechange.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 3:

Labels changed: added release-none, removed go1.3maybe.

Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 4:

Labels changed: added repo-main.

Contributor

rsc commented Dec 4, 2013

Comment 4:

Labels changed: added repo-main.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Dec 13, 2013

Contributor

Comment 5:

This is worth discussion. I am content with what's defined now but there are situations
where the proposed change could be beneficial. It would be a backwards-compatible change.
I am voting neither for nor against at this point.

Owner changed to @griesemer.

Status changed to Accepted.

Contributor

robpike commented Dec 13, 2013

Comment 5:

This is worth discussion. I am content with what's defined now but there are situations
where the proposed change could be beneficial. It would be a backwards-compatible change.
I am voting neither for nor against at this point.

Owner changed to @griesemer.

Status changed to Accepted.

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Jul 11, 2015

Member

Referencing #11661 for completeness.

Member

dominikh commented Jul 11, 2015

Referencing #11661 for completeness.

@tantalic

This comment has been minimized.

Show comment
Hide comment
@tantalic

tantalic Dec 28, 2015

This seems like a very valuable change to the language in many situations I have encountered with very little downsides:

  1. Struct field tags have evolved in a very powerful way and used for numerous unrelated things things: file encoding (JSON, XML, YAML, INI, etc.), database/datastore definitions, environment variables, and so on.
  2. Often is desirable to have different portions application use the same data structure without needing to be coupled to the the other uses of field tags. For example a database layer should not need to be aware of the JSON representation used in an API.
  3. In some cases it may be desirable to have different field tags used at different layers in an application. For example, a struct may need to be serialized different JSON property names to save it to disk then is used for providing via an API.
  4. As mentioned by @ianlancetaylor and @robpike it would be backwards-compatible.

tantalic commented Dec 28, 2015

This seems like a very valuable change to the language in many situations I have encountered with very little downsides:

  1. Struct field tags have evolved in a very powerful way and used for numerous unrelated things things: file encoding (JSON, XML, YAML, INI, etc.), database/datastore definitions, environment variables, and so on.
  2. Often is desirable to have different portions application use the same data structure without needing to be coupled to the the other uses of field tags. For example a database layer should not need to be aware of the JSON representation used in an API.
  3. In some cases it may be desirable to have different field tags used at different layers in an application. For example, a struct may need to be serialized different JSON property names to save it to disk then is used for providing via an API.
  4. As mentioned by @ianlancetaylor and @robpike it would be backwards-compatible.
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 4, 2016

Contributor

To be clear, the change would be about conversion rules, not type identity.

The implementation of package reflect uses differing struct tags specifically to make sure that various of its types cannot be converted from one to the other. So it's not clear to me that this really would be backwards compatible, although I see why it would be convenient.

If someone wants to do this, the next step would be to write a proposal (see golang.org/s/proposal).

Contributor

rsc commented Jan 4, 2016

To be clear, the change would be about conversion rules, not type identity.

The implementation of package reflect uses differing struct tags specifically to make sure that various of its types cannot be converted from one to the other. So it's not clear to me that this really would be backwards compatible, although I see why it would be convenient.

If someone wants to do this, the next step would be to write a proposal (see golang.org/s/proposal).

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Mar 24, 2016

Ignoring tags when checking converitbleness would be very useful for us.
Our usecase in Kubernetes is basically like that:

  • we have an internal representation of an object, and (potentially more than one) external representation of that object
  • the external one is used for "outside" world - it is stored in DB, it is how clients communicate with kubernetes, etc. - thus its fields contain tags e.g. for serialization (json tag and protobuf tag)
  • the internal one is used purely inside Kubernetes, and should never be serialized, thus we don't want tags on it

But still the are a bunch of objects that look exactly the same in the internal and external representation and we would like "reflect.ConvertibleTo" to work for them - currently we are not able to use it, because of tag differences.
@lavalamp @smarterclayton

wojtek-t commented Mar 24, 2016

Ignoring tags when checking converitbleness would be very useful for us.
Our usecase in Kubernetes is basically like that:

  • we have an internal representation of an object, and (potentially more than one) external representation of that object
  • the external one is used for "outside" world - it is stored in DB, it is how clients communicate with kubernetes, etc. - thus its fields contain tags e.g. for serialization (json tag and protobuf tag)
  • the internal one is used purely inside Kubernetes, and should never be serialized, thus we don't want tags on it

But still the are a bunch of objects that look exactly the same in the internal and external representation and we would like "reflect.ConvertibleTo" to work for them - currently we are not able to use it, because of tag differences.
@lavalamp @smarterclayton

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Mar 26, 2016

Contributor

The frequency with which tags appear in the wild is much higher than we expected when they were proposed, so if possible it is worth thinking about adapting the spec to fit usage patterns better.

Tags are part of the type, literally, so it made sense to make them part of type identity and it should stay that way. But for conversion, there seem to be valid reasons for relaxing the requirement. I am cautiously favorable. I am however certain that such a change would break things in subtle ways, so it needs to be approached very carefully and analyzed closely before proceeding, assuming proceeding is possible.

Contributor

robpike commented Mar 26, 2016

The frequency with which tags appear in the wild is much higher than we expected when they were proposed, so if possible it is worth thinking about adapting the spec to fit usage patterns better.

Tags are part of the type, literally, so it made sense to make them part of type identity and it should stay that way. But for conversion, there seem to be valid reasons for relaxing the requirement. I am cautiously favorable. I am however certain that such a change would break things in subtle ways, so it needs to be approached very carefully and analyzed closely before proceeding, assuming proceeding is possible.

@ardan-bkennedy

This comment has been minimized.

Show comment
Hide comment
@ardan-bkennedy

ardan-bkennedy Mar 28, 2016

Does this require a formal proposal to be moved forward? I’ll spend the time on the proposal if one is needed.

ardan-bkennedy commented Mar 28, 2016

Does this require a formal proposal to be moved forward? I’ll spend the time on the proposal if one is needed.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Mar 28, 2016

Contributor

I will take care if this. Thanks for the offer, though.

  • gri

On Mon, Mar 28, 2016 at 10:42 AM, William Kennedy notifications@github.com
wrote:

Does this require a formal proposal to be moved forward? I’ll spend the
time on the proposal if one is needed.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6858 (comment)

Contributor

griesemer commented Mar 28, 2016

I will take care if this. Thanks for the offer, though.

  • gri

On Mon, Mar 28, 2016 at 10:42 AM, William Kennedy notifications@github.com
wrote:

Does this require a formal proposal to be moved forward? I’ll spend the
time on the proposal if one is needed.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6858 (comment)

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Mar 29, 2016

Thanks a lot for looking into it!

We were able to workaround it on our side for now (so it's not very urgent). But please let me know if you need any input from "customer point of view". :)

wojtek-t commented Mar 29, 2016

Thanks a lot for looking into it!

We were able to workaround it on our side for now (so it's not very urgent). But please let me know if you need any input from "customer point of view". :)

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Mar 29, 2016

I think the next logical thing people will want to do, if you allow this convertibility, is to use the reflect package to make a "copy" of a type which allows mutating the struct tags. It seems like a really bad idea to allow mutability of tags in general--it's a terrible place to store global variables!--but I can see people wanting to do something like this to get various encoding/decoding systems to play nicely with each other. I'm guessing you won't want to do this so mostly I'm asking to explain why not in your proposal :)

(And to reiterate what @wojtek-t said, there's no burning need for this for Kubernetes at the moment, so don't rush on our account.)

lavalamp commented Mar 29, 2016

I think the next logical thing people will want to do, if you allow this convertibility, is to use the reflect package to make a "copy" of a type which allows mutating the struct tags. It seems like a really bad idea to allow mutability of tags in general--it's a terrible place to store global variables!--but I can see people wanting to do something like this to get various encoding/decoding systems to play nicely with each other. I'm guessing you won't want to do this so mostly I'm asking to explain why not in your proposal :)

(And to reiterate what @wojtek-t said, there's no burning need for this for Kubernetes at the moment, so don't rush on our account.)

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Mar 29, 2016

Contributor

Thanks for the feedback. This is not a super-urgent issue on our side either, given that it's a potential language change we want to be very careful here, and consider all input.

I will likely start on a proposal around the 1.7 freeze (in ~1 month) as there are more important issues to deal with at the moment.

Contributor

griesemer commented Mar 29, 2016

Thanks for the feedback. This is not a super-urgent issue on our side either, given that it's a potential language change we want to be very careful here, and consider all input.

I will likely start on a proposal around the 1.7 freeze (in ~1 month) as there are more important issues to deal with at the moment.

@dionb

This comment has been minimized.

Show comment
Hide comment
@dionb

dionb May 16, 2016

This would be HUGELY useful to me in the near future. I am working on a program that will need to convert between json and xml formats for about a dozen different systems. These formats mostly only vary in the naming or excluding of values, so the conversion could be handled nicely with tags once this change has been made.

dionb commented May 16, 2016

This would be HUGELY useful to me in the near future. I am working on a program that will need to convert between json and xml formats for about a dozen different systems. These formats mostly only vary in the naming or excluding of values, so the conversion could be handled nicely with tags once this change has been made.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer May 16, 2016

Contributor

I believe the agreement in support of this change is pretty broad.
Hopefully for 1.8.

This would be HUGELY useful to me in the near future. I am working on a
program that will need to convert between json and xml formats for about a
dozen different systems. These formats mostly only vary in the naming or
excluding of values, so the conversion could be handled nicely with tags
once this change has been made.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6858 (comment)

Contributor

griesemer commented May 16, 2016

I believe the agreement in support of this change is pretty broad.
Hopefully for 1.8.

This would be HUGELY useful to me in the near future. I am working on a
program that will need to convert between json and xml formats for about a
dozen different systems. These formats mostly only vary in the naming or
excluding of values, so the conversion could be handled nicely with tags
once this change has been made.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6858 (comment)

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike May 16, 2016

Contributor

There's still the observation that the change may break some code, such as the reflect package. Before pushing on this as a proposal, I suggest a quick hack to the compiler to make the conversion legal and seeing what happens.

Contributor

robpike commented May 16, 2016

There's still the observation that the change may break some code, such as the reflect package. Before pushing on this as a proposal, I suggest a quick hack to the compiler to make the conversion legal and seeing what happens.

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer May 17, 2016

Contributor

Yes, of course we have to experiment with this first.
On Mon, May 16, 2016 at 9:51 PM Rob Pike notifications@github.com wrote:

There's still the observation that the change may break some code, such as
the reflect package. Before pushing on this as a proposal, I suggest a
quick hack to the compiler to make the conversion legal and seeing what
happens.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6858 (comment)

Contributor

griesemer commented May 17, 2016

Yes, of course we have to experiment with this first.
On Mon, May 16, 2016 at 9:51 PM Rob Pike notifications@github.com wrote:

There's still the observation that the change may break some code, such as
the reflect package. Before pushing on this as a proposal, I suggest a
quick hack to the compiler to make the conversion legal and seeing what
happens.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6858 (comment)

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 6, 2016

Contributor

For reference, the reflect package comment on rtype says

// rtype is the common implementation of most values.
// It is embedded in other, public struct types, but always
// with a unique tag like `reflect:"array"` or `reflect:"ptr"`
// so that code cannot convert from, say, *arrayType to *ptrType.

If we drop the conversion restriction, code using the reflect package will be able to do the latter conversion. However, I haven't figured out how code could actually get the required types. All it can easily get is *rtype, not *arrayType or *ptrType.

Contributor

ianlancetaylor commented Jun 6, 2016

For reference, the reflect package comment on rtype says

// rtype is the common implementation of most values.
// It is embedded in other, public struct types, but always
// with a unique tag like `reflect:"array"` or `reflect:"ptr"`
// so that code cannot convert from, say, *arrayType to *ptrType.

If we drop the conversion restriction, code using the reflect package will be able to do the latter conversion. However, I haven't figured out how code could actually get the required types. All it can easily get is *rtype, not *arrayType or *ptrType.

@dionb

This comment has been minimized.

Show comment
Hide comment
@dionb

dionb Jun 7, 2016

That issue could be solved by only allowing conversion if the 'reflect' tag is the same. It should still be safe to ignore all user defined tags, assuming they don't create one called 'reflect', which they probably shouldn't be doing anyway.

dionb commented Jun 7, 2016

That issue could be solved by only allowing conversion if the 'reflect' tag is the same. It should still be safe to ignore all user defined tags, assuming they don't create one called 'reflect', which they probably shouldn't be doing anyway.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 7, 2016

Contributor

@dionb Interesting idea, but I think that is much too special purpose for a language like Go.

Anyhow it's easy enough to implement the same restriction by adding differently-named zero-sized fields to the relevant types.

Contributor

ianlancetaylor commented Jun 7, 2016

@dionb Interesting idea, but I think that is much too special purpose for a language like Go.

Anyhow it's easy enough to implement the same restriction by adding differently-named zero-sized fields to the relevant types.

@dionb

This comment has been minimized.

Show comment
Hide comment
@dionb

dionb Jun 7, 2016

@ianlancetaylor I like that solution as being a quick easy way to implement the functionality we want, but would it not be better to create defined behaviour around a particular tag being used within language defining packages, rather than relying on a side effect of adding a field to internal type definitions?

To be clear, if I were to implement this for my own use I would probably do it that way, but for future language development I think that would be a dangerous way to do it. Differently-named zero-sized fields would require that the people working on the standard libraries/compiler understand the particular implementation rather than a behaviour of the language.

dionb commented Jun 7, 2016

@ianlancetaylor I like that solution as being a quick easy way to implement the functionality we want, but would it not be better to create defined behaviour around a particular tag being used within language defining packages, rather than relying on a side effect of adding a field to internal type definitions?

To be clear, if I were to implement this for my own use I would probably do it that way, but for future language development I think that would be a dangerous way to do it. Differently-named zero-sized fields would require that the people working on the standard libraries/compiler understand the particular implementation rather than a behaviour of the language.

@mdempsky

This comment has been minimized.

Show comment
Hide comment
@mdempsky

mdempsky Jun 7, 2016

Member

Ian is talking about changing package reflect's ptrType and sliceType to something like:

type ptrType struct {
    _ptr [0]byte
    rtype
    elem *rtype
}

type sliceType struct {
    _slice [0]byte
    rtype
    elem *rtype
}

_ptr and _slice are different names, which means ptrType and sliceType have different underlying types. In turn, that means you cannot convert between *ptrType and *sliceType (which is the goal here).

This is not an implementation detail. It's part of the Go language spec already.

Member

mdempsky commented Jun 7, 2016

Ian is talking about changing package reflect's ptrType and sliceType to something like:

type ptrType struct {
    _ptr [0]byte
    rtype
    elem *rtype
}

type sliceType struct {
    _slice [0]byte
    rtype
    elem *rtype
}

_ptr and _slice are different names, which means ptrType and sliceType have different underlying types. In turn, that means you cannot convert between *ptrType and *sliceType (which is the goal here).

This is not an implementation detail. It's part of the Go language spec already.

@dionb

This comment has been minimized.

Show comment
Hide comment
@dionb

dionb Jun 13, 2016

@mdempsky I understand exactly what Ian was talking about. This is an implementation detail in the sense that if you wanted to add another type (or 2+) that is of the underlying structure {rtype, elem *rtype} then you have to add a vestigial (for want of a better word) field with a unique name (and thus must know what all others have been created in this manor) to not potentially break the compiler, or your program.

This is not adding a defined behaviour to the language to create the functionality we want, it is relying on a side effect another feature of the language. This becomes more evident when you consider the case of defining types in normal programming that have the same underlying struct (which is different to the one above) but have different methods attached. To make sure they can't be converted between there is no documented feature of the language, you must use a side effect of a feature designed for storing data.

dionb commented Jun 13, 2016

@mdempsky I understand exactly what Ian was talking about. This is an implementation detail in the sense that if you wanted to add another type (or 2+) that is of the underlying structure {rtype, elem *rtype} then you have to add a vestigial (for want of a better word) field with a unique name (and thus must know what all others have been created in this manor) to not potentially break the compiler, or your program.

This is not adding a defined behaviour to the language to create the functionality we want, it is relying on a side effect another feature of the language. This becomes more evident when you consider the case of defining types in normal programming that have the same underlying struct (which is different to the one above) but have different methods attached. To make sure they can't be converted between there is no documented feature of the language, you must use a side effect of a feature designed for storing data.

@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.

@dionb

This comment has been minimized.

Show comment
Hide comment
@dionb

dionb Aug 22, 2016

Could we remove the need for this if we instead modified the json and xml (and sql?(I don't use the core sql package directly)) packages to allow you to specify a tag to use when un/marshalling?
It would mean that you can end up with your code for defining variable names in external interfaces in weird places (not in the package which implements the interface) but it would mean that we aren't changing the way the language handles casting.
I understand that there are other use cases, but does this cover most of the benefit of this change for people?

dionb commented Aug 22, 2016

Could we remove the need for this if we instead modified the json and xml (and sql?(I don't use the core sql package directly)) packages to allow you to specify a tag to use when un/marshalling?
It would mean that you can end up with your code for defining variable names in external interfaces in weird places (not in the package which implements the interface) but it would mean that we aren't changing the way the language handles casting.
I understand that there are other use cases, but does this cover most of the benefit of this change for people?

@griesemer

This comment has been minimized.

Show comment
Hide comment
@griesemer

griesemer Aug 22, 2016

Contributor

@dionb Maybe but it would only solve the problem for json and xml and require a library change. The language change seems to be pretty uncontroversial and straight-forward. It also makes sense that conversion would allow to "override" struct tags which don't really have an impact on the value of the encoded data.

Contributor

griesemer commented Aug 22, 2016

@dionb Maybe but it would only solve the problem for json and xml and require a library change. The language change seems to be pretty uncontroversial and straight-forward. It also makes sense that conversion would allow to "override" struct tags which don't really have an impact on the value of the encoded data.

@gopherbot gopherbot closed this in 5c7a005 Oct 4, 2016

@golang golang locked and limited conversation to collaborators Oct 4, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.