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

spec: clarify method sets and recursive promotions #15708

Open
mdempsky opened this issue May 16, 2016 · 6 comments

Comments

@mdempsky
Copy link
Member

commented May 16, 2016

My reading of the Go spec is that this is valid:

package p

type S1 struct { *S2 }
type S2 struct { *S1 }

func (*S1) M() {}

var _ = S1.M

Rationale:

  1. Because of func (*S1) M() {}, M is a member of *S1's method set.
  2. Because of the *S1 embedding into S2, M is also a member of S2 and *S2's method sets.
  3. Because of the *S2 embedding into S1, M should also be a member of S1's method set.

However, cmd/compile, gccgo, and gotype all reject it.

/cc @griesemer @ianlancetaylor

@mdempsky mdempsky added this to the Go1.8Maybe milestone May 16, 2016

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

If all three compilers reject it, I think it should remain invalid.

Also, if we accept this, then I think a consequence would be that is no longer the case that the method set of *T is a superset of the method set of T, although the spec says that that is the case.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

FWIW, more examples that are rejected by all three:

Here we show that it's not just the case that we don't recursively promote (*T).M to T.M, but that the presence of (*T).M precludes promoting any method as T.M:

package p

type S1 struct { S2 }
type S2 struct {}

func (*S1) M() {}
func (S2) M() {}

var _ = S1.M

Here we show that even though M is not a member of S2's method set, its presence in *S2s method set precludes S3.M from being promoted as S1.M. I.e., the fact that (*S1).M is ambiguous precludes S1.M too.

package p

type S1 struct { S2; S3 }
type S2 struct {}
type S3 struct {}

func (*S2) M() {}
func (S3) M() {}

var _ = S1.M
mdempsky added a commit to mdempsky/gocode that referenced this issue May 18, 2016
lookdot: new package for enumerating selectable objects
An extraction and rewrite of the logic from package suggest. In
particular, this code now handles many more subtle cases correctly.

Writing it also revealed multiple inconsistencies in the Go spec
and/or standard implementations: golang/go#15708, golang/go#15721,
golang/go#15722.

@griesemer griesemer self-assigned this May 18, 2016

@griesemer

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

There is a subtle interplay between the definition of method sets, how they are enlarged via embedding, and the rules for selector lookup. My reading of the spec is that there is a conflict, and that all compilers and go/types consistently decide these programs by looking at a) the basic definition of method sets (https://tip.golang.org/ref/spec#Method_sets), and b) the basic rules for selector lookup (https://tip.golang.org/ref/spec#Selectors), rather then how method sets are extended (which also historically was described later in the development of Go).

Specifically, in your first case, S1.M is a selector expression. When type-checking a selector expression, we look (per the spec) for the most shallow M defined directly with a (base) type. The most shallow such M is the one method directly associated with S1 (the receiver base type is S1). There is no further search at this point, and no consideration of how that M is used. But that M requires a *S1 receiver rather than a S1 (and there is no implicit indirection possible), hence the method expression is invalid.

In other words, the way method sets are extended by way of embedding is really implicitly defined by way of how method/field lookup works. We may want to be more careful in describing how method sets are extended in https://tip.golang.org/ref/spec#Struct_types. For instance, the notion of depth (as in "the shallowest") is not present there, yet it is crucial for lookup.

Thus, I believe the compilers and go/types are correct.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2016

I agree with Robert and Ian that the compilers and go/types are correct. It would be terribly confusing if (*S1).M and S1.M resolved to different method definitions M. Perhaps some rewording in the spec is needed, perhaps not, but the rules we agree on shouldn't change.

@rsc rsc changed the title spec: method sets and recursive promotions spec: clarify method sets and recursive promotions Nov 2, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 3, 2017

Bumping to Go 1.9, but feel free to send a CL this week for Go 1.8 if you want.

@bradfitz bradfitz modified the milestones: Go1.9, Go1.8Maybe Jan 3, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2017

Bumping to 1.10. Requires careful spec wording.

@griesemer griesemer modified the milestones: Go1.10, Go1.9 Jun 6, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 14, 2017

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.