Skip to content
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: spec: Allow conversion of deeply identical types #20621

Closed
c0b opened this issue Jun 9, 2017 · 11 comments
Closed

proposal: spec: Allow conversion of deeply identical types #20621

c0b opened this issue Jun 9, 2017 · 11 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@c0b
Copy link

c0b commented Jun 9, 2017

https://golang.org/ref/spec#Conversions the spec has an example of conversion between deep struct with same underlying types that indeed works,

type Person struct {
	Name    string
	Address *struct {
		Street string
		City   string
	}
}

var data *struct {
	Name    string `json:"name"`
	Address *struct {
		Street string `json:"street"`
		City   string `json:"city"`
	} `json:"address"`
}

var person = (*Person)(data)  // ignoring tags, the underlying types are identical

however, if one of the internal struct is defined as a type, it can no longer convert:

https://play.golang.org/p/8gE_0Vd3gr

type Address1 struct {
	Street string
	City   string
}

type Person struct {
	Name    string
	Address *Address1
}

var data *struct {
	Name    string `json:"name"`
	Address *struct {
		Street string `json:"street"`
		City   string `json:"city"`
	} `json:"address"`
}

var person = (*Person)(data) // ignoring tags, the underlying types are identical

func main() {
	fmt.Println("Hello, playground", person)
}

it failed with

tmp/sandbox899894781/main.go:25: cannot convert data (type *struct { Name string "json:\"name\""; Address *struct { Street string "json:\"street\""; City string "json:\"city\"" } "json:\"address\"" }) to type *Person

it's counter intuitive and makes the deep equal less useful, because if stick with the original Person definition,

type Person struct {
	Name    string
	Address *struct {
		Street string
		City   string
	}
}

every time to construct a Person need to carry the all fields of the internal struct, it's inconvenient and would be better to define the internal struct as its own type,

var p = Person{
	Name: "name",
	Address: &struct {
		Street string
		City   string
	}{"street", "city"},
}

however with the defined internal type the conversion no longer works.

the var data in this example is foreign data and doesn't know the type Person (and its internal Address type).

Related #19778

@gopherbot gopherbot added this to the Proposal milestone Jun 9, 2017
@dsnet
Copy link
Member

dsnet commented Jun 9, 2017

Thanks for the proposal. Language changes aren't really being considered until Go2.

@dsnet dsnet added v2 A language change or incompatible library change LanguageChange labels Jun 9, 2017
@c0b
Copy link
Author

c0b commented Jun 15, 2017

could you explain more why these #20621 #19778 are Language changes, if implement these in next Go1.x then what's the problem, to me it doesn't seem like will cause any existing code to not compilable

and I believe Golang team must be aware of these since beginning of this language design, could you talk more why these weren't implemented since beginning? what could be the problems if made it since beginning?

type A struct{
  x int
}

type X int
type B struct{
  x X
}

// types A and B are not convertible

@ianlancetaylor
Copy link
Contributor

Permitting code that was not permitted before is still a language change. We've realized that we don't want to make a long series of small incremental language changes, because we don't know where we will end up. We want to consider all changes together, to make sure they work well together.

Permitting type conversions between types that are the same other than their names can change the method set of the resulting value. That may be fine, but it's not something to adopt casually. The question with this change, as with all Go language changes, is what programs are affected? The way to make this change more convincing is to show real programs that are using workarounds to get around this inability to easily convert between identical underlying types.

@kaedys
Copy link

kaedys commented Jun 20, 2017

How is this proposal fundamentally different from #16085? Both open up new functionality with regards to type conversions that are logically consistent, and this proposal is an intuitive extension of #16085. #16085 was based on the observation that tags do not materially affect that memory layout of a type, and thus should not impair type conversions of that type. This proposal is based on the similar observation that naming a struct type does not materially change it relative to an (identical) anonymous type, and thus having a named struct type as a field should not behave differently from having an anonymous struct field when it comes to these types of conversions.

@ianlancetaylor
Copy link
Contributor

@kaedys The essential distinction is methods, and by extension the set of interfaces that a type implements. The presence or absence of a field tag does not affect the method set. Conversion between different named types with the same underlying type does affect the method set.

That does not mean that this proposal can not be accepted, I'm just explaining why there is a significant difference between this proposal and #16085.

@kaedys
Copy link

kaedys commented Jun 20, 2017

Yes, but #16085 already allows conversion between two completely distinct types so long as they have the same memory layout. There's no requirement that they have the same method set (ex: https://play.golang.org/p/HAo-k2BoRe). So levying such a complaint at this proposal falls flat when such a conversion is already possible due to #16085

@ianlancetaylor
Copy link
Contributor

We can already convert between different types, with potentially different method sets, at the top level, as long as they have identical underlying types. The only change in #16085 was to permit converting between different types that have underlying types that differ only in their field tags. That is, #16085 did not permit any method-set affecting change that was not already possible.

This proposal would permit changing the method sets not just of the top level type, the type being named in the conversion, but also the method sets of other types down in the underlying type, such as field types or parameter types.

@kaedys
Copy link

kaedys commented Jun 21, 2017

If the top level changes are already allowed, how is that a significant change? The types specified in the structure are still concrete, defined at compile time, so it's not like these changes can happen unexpectedly. The only thing this change would eliminate is the need for a helper method/function to do for fields what's already possible with top-level types. I just don't see how this is significant and novel enough of a change to justify slating it for 2.x when equivalent top-level conversions are already perfectly legal.

@dsnet
Copy link
Member

dsnet commented Jun 21, 2017

I have found the conversion of anonymous structs to concrete types to be somewhat brittle in practice. The technique is useful as a mechanism to assign arbitrary struct tags to certain fields so that it can be used with the json package (and the like), but it has a major downside of causing the code to be brittle. For example, if a field was added to the named type, then any code using this technique would break because the memory layouts of the types are no longer the same. This is the same reason why the Go1 compatibility document recommends against unkeyed struct literals and also for Rob's proposal to enforce it in #2794.

For this reason, expanding what is allowed for conversion seems to just allow for more cases of brittle code to be written. That being said, if we were to somehow only allow direct conversion of structs defined within your own package, then I would be in support for a change like this.

@ianlancetaylor
Copy link
Contributor

@kaedys You asked why this change is different than #16085, and I believe I have answered that. You are now shifting to argue that the change is not significant. Maybe so. My point is simply that it is clearly a change, and there is nothing like it in the language today. With that in mind, I encourage you to reread my comment of six days ago. Thanks.

@ianlancetaylor
Copy link
Contributor

The idea can make sense for struct types, but more generally seems problematic.

Consider

type Cowboy interface {
    DrawOn(Cowboy)
}

type Circle interface {
    DrawOn(Circle)
}

The proposal here would seem to permit Circle(Cowboy(nil)). Is that what we want?

If we in general permit conversions between types that are structurally equivalent ignoring type names (and associated method sets) we wind up with cases like that where the types seem quite different.

The current proposal seems too vague and permits too many conversions. It's not clear how to narrow down in a way that remains orthogonal. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

6 participants