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

cmd/go2go: generic struct embedding doesn't seem to work as expected #39678

Closed
beoran opened this issue Jun 18, 2020 · 7 comments
Closed

cmd/go2go: generic struct embedding doesn't seem to work as expected #39678

beoran opened this issue Jun 18, 2020 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@beoran
Copy link

beoran commented Jun 18, 2020

I was playing arund in the go2go playground to see if I could generically add methods to a type,
using struct embedding, but it didn't work as I expected it would.
See the following example, it does not compile with an error: "./prog.go2:9: undefined: T",
but I think it should work correctly. I also can't use the BaseData type as I expected I could.
Is this by design or is there another way to do what I want to do?

package main

import (
	"fmt"
)

// Trying to generically add methods to a data type.
type Base(type T) struct {
	T
}

// The method generically implemented
func (b Base(T)) Foo() {
	fmt.Printf("Foo: %v\n", b.T)
}

type Data struct {
	Count int
}

type BaseData Base(Data)

func main() {
	// p := BaseData{} : doesn't work
	p := Base(Data){}
	p.Count = 7
	p.Foo()
}

https://go2goplay.golang.org/p/PpLEuAoRWIm

@griesemer
Copy link
Contributor

This code is accepted by the type checker. Of course, when you use p := BaseData{}, the p.Foo() call will fail as in that case that Foo method is not present.

There is a an issue with the translator.

But there is also a question as to whether this should be permitted in the first place: If Base is locally instantiated (inside a function) with a type parameter from the enclosing function, it's not exactly clear what that means with respect to methods. Does B have the methods of the constraint associated with the enclosing function's type parameter? (Right now the type checker ignores those). There are various open questions around such scenarios that we need to understand better.

@griesemer griesemer added this to the Unreleased milestone Jun 18, 2020
@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 18, 2020
@beoran
Copy link
Author

beoran commented Jun 18, 2020

First of all, I fail to see why p := BaseData{}, p.Foo() should fail, since BaseData, IMO, should have the method Foo, from the declaration func (b Base(T)) Foo(). I find it counter-intuitive with my experience from other programming languages that this is not the case.

I think "what it should mean" for my example depends on practical use. I think this should be permitted, because this would be extremely handy in several situations. For example; imagine a GUI library where I have a generic type Widget (type T) { T ; WidgetData }, with many methods defined generically on Widget(T). It would be very natural, and convenient to be able to write type Button Widget(ButtonData), and then be able to use all the generic Widget methods on Button as well, as a form of "static inheritance".

@beoran
Copy link
Author

beoran commented Jun 18, 2020

This is the minimal program that has the issue:
https://go2goplay.golang.org/p/3SsjcAKB0A9

package main

type Base(type Embed) struct {
	Embed // Embed // Remove the first pair of // to make it compile
}

type Data struct {
}

func main() {
	_ = Base(Data){}
}

@griesemer
Copy link
Contributor

@beoran BaseData is a declared as a new, defined type, with the underlying type being the underlying type of Base(Data). Even in regular Go, new, defined types don't inherit the methods associated with the defined type the declaration has on the RHS (they only get methods that might be present through embedding in that RHS type). And in this case, Foo is associated with Base. So this seems to be working as one would expect.

Your minimal program that has the issue is valid and accepted by the type checker; this is still a translator issue. (That is, it type-checks with Embed as an embedded field.)

Finally, regarding what it should mean: Naturally, we want to define the meaning in a way that is useful. But we also have existing rules in Go, and it may well be that some of the semantics that we are trying to define for type parameters actually follows from existing rules. We are still exploring this. For instance, some of the modeling of type parameters in the type checker, based on "usefulness", turned out to be incorrect as it lead to inconsistencies.

@beoran
Copy link
Author

beoran commented Jun 18, 2020

Thanks for your explanation. I realize that misunderstood. What I want to do is type BaseData = Base(Data), however, iirc, this is not allowed with the current design, although I think it would be useful.

And yes, consistency is important.

Therefore, for this issue, let's focus on the problem with the translator, and see how we can fix this.

@gopherbot
Copy link

Change https://golang.org/cl/238857 mentions this issue: [dev.go2go] go/go2go: handle fields promoted through embedded type parameter

@ianlancetaylor
Copy link
Contributor

This is now fixed on the dev.go2go branch.

I doubt that I have all the cases right here, but at least this case should now work.

gopherbot pushed a commit that referenced this issue Jun 18, 2020
…rameter

Fixes #39678

Change-Id: Ib74d15795ab85ad0b1e58096302a9a88810242c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/238857
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants