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

go/types: what is the (types.Object) identity of embedded methods? #34421

Open
griesemer opened this issue Sep 20, 2019 · 3 comments

Comments

@griesemer
Copy link
Contributor

commented Sep 20, 2019

Up to Go 1.13, embedded interfaces could not have overlapping methods. The corresponding go/types implementation, when computing method sets of interfaces, reused the original methods in embedding methods. For instance, given:

type A interface {
   m()
}

type B interface {
   A
}

the go/types method Func Object for m in B was the same as in A. We know that some (external) tests depended on this property. A consequence of this is that the source position of B.m is the same as the one of A.m; it is not the position of the embedded A in B.

With Go 1.14, embedded interfaces may overlap. The same method m (with identical signature), may be embedded in an interface through multiple embedded interfaces:

type C interface {
   A
   A2  // A2 also defines m()
   A3  // A3 also defines m()
}

Now, it is not clear which "original" method m should be used in the method set for C. It could be the "first" one (as in the one embedded via the earliest embedded interface providing m in the source, so A in this example), or it could be undefined. Or it could be a new method Func Object. Note that the latter choice wouldn't be of much help when deciding which position should be assigned to m as it could be any of the embedded interfaces that provide m.

This boils down to what kind of API guarantee should be provided by go/types.

@griesemer griesemer added this to the Go1.14 milestone Sep 20, 2019
@griesemer griesemer self-assigned this Sep 20, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Sep 20, 2019

Change https://golang.org/cl/196561 mentions this issue: go/types: don't clone interface methods when embedding them

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

@alandonovan, @katiehockman do you have any opinion on this?

@adonovan

This comment has been minimized.

Copy link

commented Sep 20, 2019

The crucial property is that it is deterministic: a lookup of C.m must return the same object each time. I don't think we need to guarantee that it is identical to A.m or A2.m or A3.m; an implementation could use lexical order, lexicographic order, or any other order.

What matters for most interface operations is the set of methods, not the identities of the method objects. interface{f()} is identical to interface{f()} no matter how many times you write out a new declaration of f and thus create a new Object.

gopherbot pushed a commit that referenced this issue Sep 22, 2019
https://golang.org/cl/191257 significantly changed (and simplified)
the computation of interface method sets with embedded interfaces.
Specifically, when adding methods from an embedded interface, those
method objects (Func Objects) were cloned so that they could have a
different source position (the embedding position rather than the
original method position) for better error messages.

This causes problems for code that depends on the identity of method
objects that represent the same method, embedded or not.

This CL avoids the cloning. Instead, while computing the method set
of an interface, a position map is carried along that tracks
embedding positions. The map is not needed anymore after type-
checking.

Updates #34421.

Change-Id: I8ce188136c76fa70fba686711167db29a049f46d
Reviewed-on: https://go-review.googlesource.com/c/go/+/196561
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.