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

all: be consistent about the term "embedded field" #25684

Open
mvdan opened this issue Jun 1, 2018 · 8 comments

Comments

@mvdan
Copy link
Member

commented Jun 1, 2018

The spec very clearly calls fields with no names "embedded fields": https://golang.org/ref/spec#Struct_types

A field declared with a type but no explicit field name is called an embedded field.

However, the Go standard library and documentation mixes that term with "anonymous field":

$ cd tip
$ git grep -i 'embedded field' | wc -l
83
$ git grep -i 'anonymous field' | wc -l
38

In particular, I found it in https://golang.org/pkg/go/ast/#Field:

Names   []*Ident      // field/method/parameter names; or nil if anonymous field

Since the spec only mentions one, and it seems to dominate in usage anyway, I think we should try to be as consistent as possible. Specific cases have been fixed in the past, like b396d11.

Of course, this doesn't include names that would break Go1 if changed - it is mainly meant for godocs, comments, and errors messages given by go/* and cmd/*. I presume that old changelogs, such as doc/devel/weekly.html, don't need to be changed.

/cc @griesemer @rsc @mdempsky

@mvdan mvdan added the NeedsDecision label Jun 1, 2018

@griesemer griesemer added NeedsFix and removed NeedsDecision labels Jun 1, 2018

@griesemer griesemer added this to the Unplanned milestone Jun 1, 2018

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

@mvdan Yes, we probably want to be as consistent as possible. In the early days we used both terms; more recently we have started talking about 'embedded' fields because even though they are embedded they still have a name (the type name) and thus are not anonymous in the strict sense.

That said, note that ast.Fields are also used for parameters which indeed may be anonymous. So we want to be a little bit judicious before making unilateral changes throughout.

But feel free to send me CLs.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2018

That said, note that ast.Fields are also used for parameters which indeed may be anonymous. So we want to be a little bit judicious before making unilateral changes throughout.

I'm not sure I understand; could you please give an example of a field that is embedded and anonymous, and an example of a field that is embedded but not anonymous?

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

A Field is part of a FieldList. A FieldList is also used for function signatures (FuncType). Function parameters may be indeed anonymous and then Names may (will) be nil. In that case it makes sense to talk about anonymity (anonymous parameter). Fields are either embedded (originally called anonymous) or not.

In short, simply replacing "anonymous" with "embedded" everywhere will be confusing, too (there are no embedded parameters). When making those changes, the comments may need to be adjusted. That's all I'm saying.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2018

Ah, I understand now - thanks for the clarification. Would you prefer a few CLs grouping changes, or one single CL to clarify these?

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

Depends on how many and where the fixes are. Use good judgement.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 26, 2018

Change https://golang.org/cl/120556 mentions this issue: ast: refer to "embedded" rather than "anonymous" fields in

@gopherbot gopherbot closed this in 1f3c0ee Jun 26, 2018

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2018

Note that this issue was meant to track the inconsistencies across the entire repository, not just this occurrence in go/ast. As you can see in the numbers above, there are more than thirty other cases that should be checked, so I'd leave this issue open unless we are sure that all the others are correct.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

Reopening per @mvdan 's comment.

@griesemer griesemer reopened this Jun 26, 2018

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