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
Open

spec: clarify method sets and recursive promotions #15708

mdempsky opened this issue May 16, 2016 · 6 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
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
Copy link
Contributor

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.

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc 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 added Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 17, 2016
@rsc rsc changed the title spec: method sets and recursive promotions spec: clarify method sets and recursive promotions Nov 2, 2016
@bradfitz
Copy link
Contributor

bradfitz 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
Copy link
Contributor

Bumping to 1.10. Requires careful spec wording.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants