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

cmd/compile: improve compiler error on embedded structs #23609

Closed
brendandburns opened this issue Jan 30, 2018 · 22 comments

Comments

Projects
None yet
9 participants
@brendandburns
Copy link

commented Jan 30, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9.2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Linux amd64

What did you do?

package main

type Foo struct {
    A string
}

type Bar struct {
    Foo
}

func main() {
    a := Bar{
       A: "bar",
    }
}

https://play.golang.org/p/6LDcycp9lPB

What did you expect to see?

An error like:
"The field 'A' doesn't exist in 'Bar' but is instead embedded in 'Foo'

What did you see instead?

"unknown field 'A' in struct literal of type Bar"

The motivation for this is often you will see encode:

var b Bar
...

Bar.A

Unless you go and actually pull the struct definition (which often is unnecessary) you can't tell that 'A' was embedded. If you try to create your own Bar then you get the error above, which is confusing since it's the same as if you misspelled the field name, or just got something wrong.

It's not hard to scan any embedded structs and see if there is a matching field. It's still legit to give an error, but you could definitely reduce confusion by having a better one.

@bradfitz bradfitz changed the title Feature: Improve compiler error on embedded structs cmd/compile: improve compiler error on embedded structs Jan 30, 2018

@bradfitz bradfitz added this to the Go1.11 milestone Jan 30, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

@odeke-em odeke-em self-assigned this Jan 30, 2018

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

This is definitely doable. I think the main issue is coming up with agreeable wording.

It's also worth noting that the promoted field might have been promoted through multiple field embeddings. For example:

type A struct { a int }
type B struct { A }
type C struct { B }

var _ = C{ a: 0 }

The error should probably properly mention that field a in C is promoted via B.A or something. I'm not sure how best to explain that to the user.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

@mdempsky Error message suggestion (for your specific case): "cannot set embedded field C.B.A.a directly in composite literal C".

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

@brendandburns, thanks for filing. I hit this myself not too long ago.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

I like @griesemer's suggestion with two nits:

  1. I think compiler error messages should try to match Go spec terminology when possible. For example, I'd suggest "initialize" or "assign" instead of the more informal "set". Also, B and A are embedded fields, but a itself is a promoted field. (Bonus: Searching the Go spec for "promoted field" finds "Promoted fields act like ordinary fields of a struct except that they cannot be used as field names in composite literals of the struct.")

  2. I don't think we need to repeat C. in the field description when it's already mentioned as the composite literal type.

I'd suggest:

cannot initialize promoted field B.A.a in struct literal of type C

or maybe:

cannot use promoted field B.A.a as key in struct literal of type C
@brendandburns

This comment has been minimized.

Copy link
Author

commented Jan 30, 2018

even if it is more precise, I don't think that using the word 'promoted' will help with understandability.

I've been programming Go for > 4 years, and I've never heard of 'promoted' until today.

I'd recommend favoring understandability over precision here.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

Thanks for that data point and input.

I think using a more precise, even if less common, term aids accessibility. As pointed out, searching the Go spec for "promoted field" explains what it means. Google searches for "golang promoted field" also turn up accessible explanations about embedding and promotion. Over time, I'd expect "promoted field" to become more commonly understood because of its use in this error message, and search results to be more helpful to new users in how to fix the error.

Also, I think today's users who may be unfamiliar with the term "promoted field" will still understand the rest of the error. I think that's better than using an incorrect term like "embedded field" which will increase misunderstanding.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

Agree with @brendandburns - never heard of it either. Perhaps the error should make the meaning of "promoted" obvious, at least until the term is more widely known and understood.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

Any suggestions for wording that's more understandable or that makes the meaning of "promoted field" more obvious?

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

On second thought, I agree with the point that using "embedded field" will make everything more confusing in the long run. I can't come up with a reasonably sized error message that would make the meaning of "promoted field" more obvious, so I take that back.

@brendandburns

This comment has been minimized.

Copy link
Author

commented Jan 31, 2018

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2018

I don't really care about understanding the language spec.

🤦‍♂

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

Hrm, the answer to 'making an error useful' is 'go use a search engine'?

I see largely two classes of users without a priori familiarity with the term "promoted field" who might see this error message:

  1. New users who do not understand the issue with using promoted fields in struct literals. Whether we say "promoted field" or something else, they're unlikely to immediately understand the problem. Further, it's out of the scope of the compiler to explain this to them in an error message. The best we can do for these users is to give a precise error message that they can search for or ask experienced users about.

  2. Experienced users who generally understand embedding and that promoted fields cannot be used in struct literals, but aren't familiar with the term "promoted field". However, I expect many of these users are going to find the B.A.a text by itself sufficient to understand the error and how to fix it. Moreover, I expect some of these users to learn from context that B.A.a is properly termed a "promoted field", or maybe to show interest in learning about that term and looking it up in the spec or elsewhere, though I don't think that's vital to them resolving the error.

I don't see using less precise language as substantially improving either class of users' experience in resolving their issue. If you think otherwise, it would help me understand your point of view if you could describe a user (or group thereof) with certain familiarities with Go, and how and why you think they would react to different error messages.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2018

Let's not bike-shed the actual choice of words here. We can fine-tune the message in an actual CL. The direction seems clear: If the code indeed attempts to assign to a promoted field, the error message should probably mention the explicit path to that field (e.g., B.A.a).

@brendandburns

This comment has been minimized.

Copy link
Author

commented Jan 31, 2018

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

I'd like to have a look at this if no one else is working on it.

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

Whoops - now I see it's assigned.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 15, 2018

@ChrisALiles oh, not a problem at all, thank you for upcoming contribution, this is great! I'll yield to you and unassign myself, all yours :)

@odeke-em odeke-em removed their assignment Feb 15, 2018

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

OK thanks (I think).

@gopherbot

This comment has been minimized.

Copy link

commented Feb 26, 2018

Change https://golang.org/cl/97076 mentions this issue: cmd/compile: improve compiler error on embedded structs

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

@gopherbot gopherbot closed this in 42ecf39 Mar 6, 2018

@mdempsky

This comment has been minimized.

Copy link
Member

commented Mar 6, 2018

For the record, the submitted change produces errors of the form

cannot use promoted field B.A.a as key in struct literal of type C

@golang golang locked and limited conversation to collaborators Mar 6, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.