struct conversion, new fields, and incompatibility

[Colin Arnott]

When working with wire formats, serialisation, and external types, it is useful to change the struct tags: https://play.golang.org/p/h6b6FmeDuaR. But when the original struct has a field added, this breaks: https://play.golang.org/p/VHmV9r2MxNt. It seems like a foregone conclusion that we suggest against adding a struct field because it is a breaking change. That said, we probably do not want to restrict the ability to convert structs as it is really helpful. Is this a known issue or previously discussed topic? If not what if anything should be done to clarify best practices?

[Axel Wagner]

Hi,

in some sense, every change to an exported symbol is a breaking change. So a straight-forward "does this change have the potential to break a reverse dependency" is simply not the best way to look at compatibility. We need more nuance.

In general, I believe it would be fair to tell the consumer of a struct that if they want to be guarded against this kind of breakage, they can't use the conversion-ignoring-struct-tags utility, but have to copy the fields one by one. That is similar to how we already tell the consumers of a struct that they need to use struct-literals with field-names.

To the producer I would say that they should consider how a struct is used, to decide if something is a breaking change or not. For example, I believe it would be fair to assume that an http.Server is not something that would be serialized so there would likely be little need to consider adding a struct field a breaking change. But something like a jwt token, or other plain-old-data struct types should probably be aware of this and might have to consider adding a field a breaking change.

In either case, I think you bring up an interesting case that I haven't thought about, before. Personally, I feel that at its core, the approach of controlling encoding behavior via struct tags just isn't friendly towards other packages re-using the same type, but changing encoding aspects. Allowing conversions to ignore struct tags was a way to remedy that, but as you demonstrate, that's still far from ideal.

So perhaps what we should do is discourage new encoding packages from coupling options to the type itself and instead encourage pass them to the encoder directly - or at least providing the option to do so. At the end of the day, Go gives authority over a type to the package defining it, both in terms of what the language allows and in terms of domains of breaking changes. So if we want to enable people to re-use a type with different encodings, we should have a way to customize the encoding behavior of types without having to touch them directly.

On Thu, Mar 11, 2021 at 6:15 PM 'Colin Arnott' via golang-nuts <golan...@googlegroups.com> wrote:

When working with wire formats, serialisation, and external types, it is useful to change the struct tags: https://play.golang.org/p/h6b6FmeDuaR. But when the original struct has a field added, this breaks: https://play.golang.org/p/VHmV9r2MxNt. It seems like a foregone conclusion that we suggest against adding a struct field because it is a breaking change. That said, we probably do not want to restrict the ability to convert structs as it is really helpful. Is this a known issue or previously discussed topic? If not what if anything should be done to clarify best practices?

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/dcdcbd6a-838e-47c1-8b3d-935d485d96b5n%40googlegroups.com.

[Robert Engels]

If you use protobufs and the current guidelines you can always add new fields. 

On Mar 11, 2021, at 12:43 PM, 'Axel Wagner' via golang-nuts <golan...@googlegroups.com> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAEkBMfGp%2B%2B90tK1A-Sti1wjap%3DXxBU-QfrE1ReUSCYDt75tmHQ%40mail.gmail.com.

[Axel Wagner]

That seems unrelated to this thread. If you add a field to a proto-message and try to do the same struct-conversion Colin mentions, you will run into exactly the same problem. We are talking about language facilities here, not third party code generators.

[Robert Engels]

I must be dense on this. I have no idea why there is any use of struct tags or JSON marshaling in the sample code. It will fail without any of that. 

On Mar 11, 2021, at 5:28 PM, Axel Wagner <axel.wa...@googlemail.com> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAEkBMfHQciGzQoVPnch%2BuyUCjR6gnGzfk%3DBZvnc6ncMF-KYS%3Dw%40mail.gmail.com.

[Axel Wagner]

On Fri, Mar 12, 2021 at 3:26 AM Robert Engels <ren...@ix.netcom.com> wrote:

I must be dense on this. I have no idea why there is any use of struct tags or JSON marshaling in the sample code.

To demonstrate why converting between different struct types is useful.

If you want take an struct existing type (defined in a different package) and change how it is marshalled (say, omit or rename a certain field) you'd have to create a new struct type with different tags and copy over the fields. This was deemed inconvenient, so it was changed to the current semantics in go 1.8. OP is pointing out that the change means adding a field to a struct is now a backwards incompatible change, as someone might to this conversion. Though to be clear, it would've been before the change as well, as someone can define a new struct type with the same tags and do the conversion and run into the same problem. It's just that the change made converting between different struct types a lot more common.

There are still cases where converting between different struct types is useful - e.g. in the case of the person who filed the original issue, they control the different instances of that struct, so they don't suffer from breakages if they change it. But in general, these kinds of conversions seem inadvisable, if you don't control both instances of the struct.

[Colin Arnott]

Nobody seems in favour of making this a "technically" breaking change, like changing the value of a string constant, though I think there is an argument. Does it seem reasonable to add a vet lint for this type of struct type conversion? Or is more discussion warranted?

I am looking to eventually drive to a proposal for some doc, convention change to attempt to prevent such breakage in the future.

[Brian Candler]

On Friday, 12 March 2021 at 06:56:46 UTC axel.wa...@googlemail.com wrote:

On Fri, Mar 12, 2021 at 3:26 AM Robert Engels <ren...@ix.netcom.com> wrote:

I must be dense on this. I have no idea why there is any use of struct tags or JSON marshaling in the sample code.

To demonstrate why converting between different struct types is useful.

If you want take an struct existing type (defined in a different package) and change how it is marshalled (say, omit or rename a certain field) you'd have to create a new struct type with different tags and copy over the fields. This was deemed inconvenient, so it was changed to the current semantics in go 1.8. OP is pointing out that the change means adding a field to a struct is now a backwards incompatible change, as someone might to this conversion.

I think I am also being dense.  I can see two separate things under discussion - compatibility of struct types for conversion, and backwards-compatibility of serialization [JSON? Protobuf?] - but I can't see what's being proposed to change.

I don't follow the argument that "adding a field to a struct is now a backwards incompatible change".  Surely this was always the case, regardless of tags?  That is, if you have

type T1 struct {

    Foo string

}

type T2 struct {

    Foo string

    Bar string

}

then what would you expect to happen for:

    v1 := T1{}

    v2 := T2(v1)   ??

...

    v2 := T2{}

    v1 := T1(v2)  ??

These two structures have different shapes.  In the first case, I suppose it could copy the matching members and create zero values for the new ones.  In the second case, I suppose it could copy the matching members and silently(!) discard the missing ones.  But either way, that just means the compiler magically writing code which copies member-by-member.  I'd rather do this by hand.

Even with magic conversions, trying to use a *t1 as a *t2 or vice versa would definitely not work, as they have differing layout in memory. Indeed, even the ordering of fields in memory must be the same for two structs to be compatible:

https://play.golang.org/p/BTUc6mNJQKS

However, if two structures have the same members in the same order, and differ only by tags, then they have the same shape.  Then it seems reasonable to me to treat a T1 as compatible with T2; copying can be done as a blob of bytes.  The change in 1.8 to omit comparing tags has made types more compatible, not less.

Separately from that, there's the issue of serialization and forwards/backwards compatibility.  I don't see any problem here.  For example, if you serialize a type T1 to JSON, and then deserialize to T2 (which may have more or fewer fields), that all works fine.

https://play.golang.org/p/aJwObgyRhan

If you want to reject unexpected fields when deserializing, there are options for doing that.  But by default, they are compatible in both directions.  You can also re-order the fields in the struct, since the JSON deserialization is assigning elements individually.

[Colin Arnott]

My apologies this is already called out in apidiff's desiderata section under the Using an Identical Type Externally heading. https://pkg.go.dev/golang.org/x/exp/apidiff

This means that it is already apocrypha. Is there still discussion to be had about struct tags and marshaling, or should I open an issue for a vet lint?

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/oAjxpH-Qq2Y/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/36e1a048-2505-4841-8882-a9b16a33fd57n%40googlegroups.com.

[Brian Candler]

Maybe the argument goes something like this:

* package P exports a struct type T1

* package Q deals with values of type T1
* package Q wants to serialize objects of type T1, but T1 doesn't have the desired set of tags; so it defines a new type T2 which is the same as T1 but with different tags

* it uses v2 := T2(v1) and then proceeds to serialize it

* if package P changes the type T1, then this breaks package Q

If that's the problem statement, then I'd say:

1. if T1 has any private fields, it's impossible to make a T2 which is compatible with it anyway (AFAICS).

https://play.golang.org/p/5QND-BdsMuR

2. if T1 changes to add a new public field, then the consumer *must* make a decision as to what to do about this extra field.  Should it exclude it from the JSON representation, or include it, or include it as 'optional' (i.e. present if non-zero)?  What JSON key should it use?  Often you don't want the default capitalization of the name.

Hence breakage seems entirely reasonable to me, to force the user of T1 to decide how to handle it.

[Colin Arnott]

> Hence breakage seems entirely reasonable to me, to force the user of T1 to decide how to handle it.

This feels like saying, if a customer implements an interface and that interface adds a method, it is up to the user to decide how to handle it.

There is even a suggestion to use private method like how you suggested using private fields. The only difference is adding methods to all public interfaces IS defined as a breaking and generally anticanon.

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/oAjxpH-Qq2Y/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/d091ff96-0407-4e82-b098-ffd4b37f6377n%40googlegroups.com.

[Brian Candler]

I think it's clear that any change to an interface - including adding or removing methods, or changing the arguments or return types of any method - must be a breaking change.

- *Adding* a method to an interface means that a concrete type which implemented the interface before, won't satisfy it now

- *Removing* a method from an interface means that code which accepted a value of that interface type and called that method, won't be able to now

Structures are a bit different:

- Adding fields to a structure, or changing their order, will not break code that accesses it member-by-member (as long as the zero-value is a sensible default)

- But it *will* break code that depends on the exact shape of the structure, e.g. if you create another type with the same shape and assign the whole thing as a single value

ISTM that consumers shouldn't do that.  In the general case (where the struct is in a different package and contains unexported fields) they can't anyway.

An interface which contains unexported methods is different beast.  AFAICS, you can't satisfy such an interface unless you embed another type which satisfies it.

[Axel Wagner]

On Fri, Mar 12, 2021 at 10:16 AM Brian Candler <b.ca...@pobox.com> wrote:

I can see two separate things under discussion - compatibility of struct types for conversion, and backwards-compatibility of serialization [JSON? Protobuf?]

Backwards-compatibility of serialization is a red herring. It is off-topic as far as this thread is concerned, this is purely about language-level compatibility of types.

 

but I can't see what's being proposed to change.

I think the proposed change can be summarized as roughly

Either
1. Create consensus that adding an (exported) field to a struct should be considered a backwards incompatible change, or
2. Create consensus that converting one struct type into a different struct type should be considered deprecated, as it makes you vulnerable to getting broken by someone adding a field.
Take either consensus and put it into the Go 1 compatibility promise and potentially encode it into a vet check.

That is my understanding of what's "proposed". Personally, as I said, I'm not in favor of either. I think both statements lack nuance.

The logical conclusions of the arguments brought forth is that every publicly visible change is a breaking change. So if we follow that logic we'd end up (in my opinion) in an ecosystem where every module is constantly incrementing their major versions because "we technically broke the API". I think that is undesirable.

Given that every publicly visible change to an exported identifier can be considered a breaking change, I personally don't think it's all that useful to talk about interface-changes vs. struct changes and which of them are breaking. Both are - in some circumstances. And in some circumstance, neither are. It's more useful to talk about *how frequently* any given change leads to breakages. I think it's easy to make an argument that changing in interface type will lead to very frequent breakages - it will either break every implementation of the interface, or any usage of the interface, or both.

Meanwhile, I think it's also not that hard to make an argument that breakages causing by converting one struct type into another and then adding a field to one of them are comparatively rare. And even within that, there is nuance, where it is more frequent for some kind of types (POD - plain-old-data) than others (e.g. http.Server or other types used to abstract and bundle state).

That's why I don't think a vet check would work. It can't distinguish the case of a struct being POD vs. the struct bundling state. And I would even argue that generally, the "bundling state" case is more common in Go.

I don't follow the argument that "adding a field to a struct is now a backwards incompatible change".  Surely this was always the case, regardless of tags?  That is, if you have

type T1 struct {

    Foo string

}

type T2 struct {

    Foo string

    Bar string

}

then what would you expect to happen for:

    v1 := T1{}

    v2 := T2(v1)   ??

...

    v2 := T2{}

    v1 := T1(v2)  ??

These two structures have different shapes.  In the first case, I suppose it could copy the matching members and create zero values for the new ones.  In the second case, I suppose it could copy the matching members and silently(!) discard the missing ones.  But either way, that just means the compiler magically writing code which copies member-by-member.  I'd rather do this by hand.

Even with magic conversions, trying to use a *t1 as a *t2 or vice versa would definitely not work, as they have differing layout in memory. Indeed, even the ordering of fields in memory must be the same for two structs to be compatible:

https://play.golang.org/p/BTUc6mNJQKS

However, if two structures have the same members in the same order, and differ only by tags, then they have the same shape.  Then it seems reasonable to me to treat a T1 as compatible with T2; copying can be done as a blob of bytes.  The change in 1.8 to omit comparing tags has made types more compatible, not less.

Separately from that, there's the issue of serialization and forwards/backwards compatibility.  I don't see any problem here.  For example, if you serialize a type T1 to JSON, and then deserialize to T2 (which may have more or fewer fields), that all works fine.

https://play.golang.org/p/aJwObgyRhan

If you want to reject unexpected fields when deserializing, there are options for doing that.  But by default, they are compatible in both directions.  You can also re-order the fields in the struct, since the JSON deserialization is assigning elements individually.

--

You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/36e1a048-2505-4841-8882-a9b16a33fd57n%40googlegroups.com.

[Robert Engels]

I don’t think you can separate the two - it seems the OP purpose of using the struct conversion is to facilitate a different serialization format. This is a place where Gos convenience (struct tags) is running into explicitness. The struct conversion is nothing more than a magic copy. Make this a method in reflect with parameters and call it a day. 

On Mar 12, 2021, at 6:15 AM, 'Axel Wagner' via golang-nuts <golan...@googlegroups.com> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAEkBMfE09%2BW0MU55LeJfm-9BNM_7SZhGkfRxD4wkP4Rj53Eq_A%40mail.gmail.com.

[Brian Candler]

Thanks for the clarification, and I agree completely.

There are plenty of cases in the Go standard library where there is a public struct for bundling state, and I don't see adding a new member (whether it's private or public) as a breaking change.  Nobody's going to attempt to mirror the entire type tree, and generally they can't because there are private members too.

Equally, it's reasonable to allow a package to export a plain-old-data struct, certainly between packages that comprise a user application.

[Ian Lance Taylor]

It may be worth noting that we already have a vet check for composite
literals that verifies that if a composite literal is used with a
struct defined in a different package, then the fields are explicitly
named. This check means that adding fields to a struct will never
break composite literals in other packages. This corresponds to this
comment in https://golang.org/doc/go1compat:

Struct literals. For the addition of features in later point
releases, it may be necessary to add fields to exported structs in the
API. Code that uses unkeyed struct literals (such as pkg.T{3, "x"}) to
create values of these types would fail to compile after such a
change. However, code that uses keyed literals (pkg.T{A: 3, B: "x"})
will continue to compile after such a change. We will update such data
structures in a way that allows keyed struct literals to remain
compatible, although unkeyed literals may fail to compile. (There are
also more intricate cases involving nested data structures or
interfaces, but they have the same resolution.) We therefore recommend
that composite literals whose type is defined in a separate package
should use the keyed notation.

I think it would be consistent with that to add a vet check that would
warn about attempts to convert a struct defined in a different package
to a locally defined struct.

Ian

[Colin Arnott]

Sweet, I will open up the proposal, unless there is mass descent to this idea.

I will call out that this WILL make implementing marshaling logic harder, but it seems orthogonal to the desire for a vet check to prevent random code breakage.

On Mon, 15 Mar 2021, 23:31 Ian Lance Taylor, <ia...@golang.org> wrote:

It may be worth noting that we already have a vet check for composite
literals that verifies that if a composite literal is used with a
struct defined in a different package, then the fields are explicitly
named.  This check means that adding fields to a struct will never
break composite literals in other packages.  This corresponds to this

comment in https://urldefense.proofpoint.com/v2/url?u=https-3A__golang.org_doc_go1compat&d=DwIBaQ&c=ncDTmphkJTvjIDPh0hpF_w&r=OaiRsMTFeJ0tMmMJQ5CwMuxNMvWoKXAJr-ebE3c5HU8&m=BwDgsUE5_LEEW5RxBIHVgubBBU6sjTmHByGINoQdumY&s=eo85CA9Hql7PSY99KGPgRBnHzFithGMWr8rpoxoSLeY&e= :

    Struct literals. For the addition of features in later point
releases, it may be necessary to add fields to exported structs in the
API. Code that uses unkeyed struct literals (such as pkg.T{3, "x"}) to
create values of these types would fail to compile after such a
change. However, code that uses keyed literals (pkg.T{A: 3, B: "x"})
will continue to compile after such a change. We will update such data
structures in a way that allows keyed struct literals to remain
compatible, although unkeyed literals may fail to compile. (There are
also more intricate cases involving nested data structures or
interfaces, but they have the same resolution.) We therefore recommend
that composite literals whose type is defined in a separate package
should use the keyed notation.

I think it would be consistent with that to add a vet check that would
warn about attempts to convert a struct defined in a different package
to a locally defined struct.

Ian

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/oAjxpH-Qq2Y/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAOyqgcXejyPtRM-sUN_JkcsP24Y2dqfPeqdensbOzmpbKF9r5Q%40mail.gmail.com.