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: document promotion of methods from embedded aliases of unnamed types #66540

Closed
adonovan opened this issue Mar 26, 2024 · 22 comments
Closed
Assignees
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Mar 26, 2024

I was surprised to learn yesterday that it is legal to embed an alias type whose underlying type is not a defined type.

type strings = []string
type S struct { strings } // equivalent to 'strings []string'
var _ = new(S).strings

This is legal according to the spec; there is supposed to be no promotion of methods from the alias's underlying type.

And yet promotion does occur:
https://go.dev/play/p/_bJZeiZW-cW

package main

import "io"

type S struct{ A }

var _ = new(S).A

type A = struct{ io.Reader }

func main() {
	if false {
		new(S).Read(nil) // not a legal program
	}
}

I expect go/types has the same issue.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 26, 2024
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 26, 2024
@Merovius
Copy link
Contributor

Personally, I don't find the promotion to be surprising. struct{ io.Reader } has a method Read, so I expect A to have one and I expect that to be promoted. Also, the spec says:

A field or method f of an embedded field in a struct x is called promoted if x.f is a legal selector that denotes that field or method f.

Which IMO does say that the method is promoted.

OTOH it also says two paragraphs later

Given a struct type S and a named type T, promoted methods are included in the method set of the struct as follows:

Which restricts promoted methods in the method set to those of named types.

So, arguably, new(S).Read is a valid selector expression, but *S is not an io.Reader (similar to how you can call pointer-methods on a value, but the value does not implement io.Reader). Or something?

@adonovan
Copy link
Member Author

adonovan commented Mar 27, 2024

Personally, I don't find the promotion to be surprising. struct{ io.Reader } has a method Read, so I expect A to have one and I expect that to be promoted.

I was surprised to learn that embedding a non-named type was legal at all, so my intuition may not be a good guide here. The specified (as opposed to implemented) behavior of promotion makes more sense to me because I have always thought of aliases as being largely melted away. But surprising or not, the spec disallows it (see below).

[spec] A field or method f of an embedded field in a struct x is called promoted if x.f is a legal selector that denotes that field or method f.
[merovius] Which IMO does say that the method is promoted.

That's the definition of promotion, not a description of when it applies. The paragraph that follows explains the mechanism:

[spec] Given a struct type S and a named type T, promoted methods are included in the method set of the struct as follows: [etc]

and the definition of named types is:

[spec] Predeclared types, defined types, and type parameters are called named types. An alias denotes a named type if the type given in the alias declaration is a named type.

So, an alias for (say) a slice type is not a named type, though it is a legal anonymous field, according to:

[spec] A field declared with a type but no explicit field name is called an embedded field. An embedded field must be specified as a type name T or as a pointer to a non-interface type name *T, and T itself may not be a pointer type. The unqualified type name acts as the field name.

@zigo101
Copy link

zigo101 commented Mar 27, 2024

Given a struct type S and a named type T,

I believe "named type" should be "type name" here. The "named type` term returned back to spec at Go 1.18.
Maybe, the typo was made at that time.

@Merovius
Copy link
Contributor

Merovius commented Mar 27, 2024

Nope.

Funnily enough, I proposed the "type name" phrasing. But @griesemer opted for "named type" instead. Maybe he has an opinion. IMO if the spec and implementation have been disagreeing for several Go versions, the spec is wrong.

(I also completely forgot that I reported it and I used exactly @adonovan's example to argue for that change)

@zigo101
Copy link

zigo101 commented Mar 27, 2024

The embedding-related wording is really subtle. I still think there are some inaccuracies there: #22005

@griesemer
Copy link
Contributor

Per this phrase, where we now use "named type" (formerly "defined type"), it seems to me that the implementation is not correct. Per the spec, an alias is only a named type type if it aliases a named type, such as a defined type:

Predeclared types, defined types, and type parameters are called named types. An alias denotes a named type if the type given in the alias declaration is a named type.

But in this case A is an alias for a type literal (an unnamed struct). I'd think that the methods of that type shouldn't be promoted (that was the original meaning, before aliases).

Can we change (fix) this? Maybe. It seems that the chance that code such exists in the wild is pretty slim; at the very least it's not going to be a lot of code. But we'd need to do a little investigation first.

@Merovius
Copy link
Contributor

@griesemer I'll point out again that in the issue I linked above I observed this behavior of promoted method and prompted that change. So IMO that change was made specifically to bring the spec in line with this behavior. I don't remember if I filed that issue based on a real world example.

At the time, I suggested "type name", specifically because it includes aliases that are not named types (and thus to address my example). I'm not sure why you chose "named type" instead; neither the issue description, nor the commit message mention.

@zigo101
Copy link

zigo101 commented Mar 29, 2024

Per the fact that an interface can embed any types, including struct literals, maybe the current implementation is preferred?

@bjorndm
Copy link

bjorndm commented Mar 30, 2024

I used this in a project for my job, no source available. It is convenient and makes sense.

@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 1, 2024
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 1, 2024
@griesemer
Copy link
Contributor

@Merovius I see. The commit message says "The types of embedded fields must be named, but they don't need to be defined types" which implies that I meant any type with a name (i.e., with a type name). Yet, the change mentions named type, which excludes aliases denoting type literals, and - which I believe - was my intention as it simply used the more general term (named type) for the original term (defined type). Sigh.

The implementation does something else, the spec says something else in most places, and so maybe this issue is fixed by simply changing "named type" to "type name" in "Given a struct type S and a name type T, ...".

(It's really unfortunate, and a mistake in my mind, that we allowed aliases for type literals. There was no need for it, and it needlessly complicates/confuses things.)

@adonovan
Copy link
Member Author

adonovan commented Apr 1, 2024

The implementation does something else, the spec says something else in most places, and so maybe this issue is fixed by simply changing "named type" to "type name" in "Given a struct type S and a name type T, ...".

That would solve the inconsistency, but it seems like a pragmatic admission of defeat relative to your original intent, which makes more sense to me.

It's ... a mistake in my mind, that we allowed aliases for type literals.

Inessential they may be, but type aliases for unnamed types are quite useful as documentable abbreviations for verbose types that have no need for declared methods, function types in particular.

@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label May 30, 2024
@griesemer griesemer modified the milestones: Go1.23, Go1.24 May 30, 2024
@griesemer
Copy link
Contributor

Too late for 1.23. Moving to 1.24.

@gopherbot
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.24.
That time is now, so a friendly reminder to look at it again.

@rsc rsc added this to Proposals Jul 25, 2024
@rsc rsc moved this to Incoming in Proposals Jul 25, 2024
@rsc rsc changed the title cmd/compile: spurious promotion of methods from embedded aliases of unnamed types proposal: cmd/compile: spurious promotion of methods from embedded aliases of unnamed types Jul 25, 2024
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 25, 2024
@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

Added to proposal process to decide what the right answer is. It may be that this has been this way for so long that the spec should be considered wrong.

@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Jul 25, 2024
@Merovius
Copy link
Contributor

Merovius commented Jul 25, 2024

FWIW to me, the behavior as implemented is still correct.

  1. I think the ability to alias unnamed types type literals is really useful, as it allows to introduce short names for composite types for documentation purposes, without influencing type-identity. I also at least always thought about it as an escape hatch to be able to embed composite types into a struct at all (though I admittedly haven't had an actual use case for that yet, despite besides idly pondering what is possible).
  2. To me, it seems useful and obvious, that struct{ io.Reader } has a promoted method Reader. In particular, I use this occasionally to do things like return struct{ io.Reader; io.Writer }{r, w} to combine a Reader and a Writer into a ReadWriter.
  3. Hence, if A has a method Read and A is embedded into S, I would expect S to have a Read method as well. The status of A as an alias doesn't have a bearing on that, as far as I'm concerned.

Personally, I find it honestly puzzling that this would seem the wrong behavior. I would find it a bit confusing to have a type embedded into a struct and not get its methods promoted - just because I happen to refer to it via an alias.

Given that changing the implemented behavior is a breakage I'm wondering what the arguments in favor of doing so even are. ISTM the burden of evidence to break compatibility should be relatively high. Are there technical reasons to change the compiler here, beyond "I find the current behavior surprising" (and I'd claim that, if it took 14 Go versions to be surprised by that, it can't be that big of a problem)?

@rsc
Copy link
Contributor

rsc commented Aug 7, 2024

It sounds like everyone agrees that we can't and shouldn't change the behavior. The spec needs to be updated, and @adonovan and @griesemer will work on the wording.

@ianlancetaylor
Copy link
Contributor

No API changes here, so taking this out of the proposal process.

@ianlancetaylor ianlancetaylor changed the title proposal: cmd/compile: spurious promotion of methods from embedded aliases of unnamed types spec: document promotion of methods from embedded aliases of unnamed types Aug 8, 2024
@ianlancetaylor ianlancetaylor added Documentation and removed early-in-cycle A change that should be done early in the 3 month dev cycle. compiler/runtime Issues related to the Go compiler and/or runtime. labels Aug 8, 2024
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 8, 2024
@adonovan adonovan self-assigned this Aug 9, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/603958 mentions this issue: spec: clarify prose for embedded struct fields

@rsc rsc added this to Proposals Aug 14, 2024
@rsc rsc moved this to Active in Proposals Aug 14, 2024
@rsc
Copy link
Contributor

rsc commented Aug 14, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

We ended up deciding this was a spec mistake and not a proposal change, but marking this accepted as the best approximation to the proposal outcome.

@dmitshur
Copy link
Contributor

@griesemer Do you think there's anything to be said in Go 1.24 release notes about this? Or is it fine not to mention it there since this a clarification only? Thanks.

@griesemer
Copy link
Contributor

@dmitshur I don't think this warrants a release note. It's not something one would casually notice in the first place, nor is it a change in behavior. The spec change is really just about being more precise about a behavior that existed from the beginning when alias types were introduced. Thanks.

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. Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests