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: "unknown field" error for unexported field in struct literal is misleading #31053

Open
cben opened this issue Mar 26, 2019 · 8 comments
Milestone

Comments

@cben
Copy link

@cben cben commented Mar 26, 2019

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

$ go version
go version go1.12beta2 linux/amd64

Does this issue reproduce with the latest release?

yes, same on Playground

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/bpaskinc/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/bpaskinc/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/golang"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build091900586=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main
import "net/http"
var _ = http.Server{doneChan: nil}  // this field does exist but isn't public

For a full matrix of (outside/inside package X exists/unknown X Public/private X struct literal/object.field) see: https://play.golang.org/p/cWl3JV8pSNL

What did you expect to see?

An error message indicated the problem is I'm trying to set an unexported field (from outside the package). Something like:
cannot set unexported field 'doneChan' in struct literal of type http.Server
If the field really doesn't exist, the current message is OK.

This is only about struct literals. When simply reading (or assigning) object.field, the messages already make the distinction between can't access vs really doesn't exist:
s.doneChan undefined (cannot refer to unexported field or method doneChan)
s.noSuchPrivate undefined (type http.Server has no field or method noSuchPrivate)

What did you see instead?

error message: unknown field 'doneChan' in struct literal of type http.Server
which IMHO is misleading because the field does exist, I just can't see it from this place.


This is a followup to #25727, the necessary change is probably similar. cc @odeke-em who's improved these messages before (thanks!).
I can submit a PR if nobody gets to it first.

@cben

This comment has been minimized.

Copy link
Author

@cben cben commented Mar 26, 2019

Strawman defense: A purist could argue it's none of my business which private fields exist and which don't. But as a human I do care! I may be the author of both packages and just forgot where I am. I may be in position to make the field public. I may need to use an exported factory function instead of a literal...
Anyway the wording "unknown" when the field clearly was there left me derailed me for several minutes and only figured it with StackOverflow...
Plus the messages for regular access already make the "unexported field" / "has no field" distinction.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Mar 29, 2019

I'm sympathetic to making the error message more helpful.

One distinction I'll point out about the s.doneChan case is that the error is primarily "s.doneChan undefined" (which is the real error), and then parenthetically mentions that doneChan is a non-exported field (which is additional detail that may or may not help the user).

In the case of the struct literal element, "unknown field" is the primary error. We could add more details about the similarly-named non-exported field, though the message as-is is already kinda wordy.

Maybe we can change the error message to something like:

invalid key doneChan: cannot refer to unexported field or method doneChan
invalid key noSuchPrivate: type http.Server has no field or method noSuchPrivate
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Mar 30, 2019

Are there any security implications of leaking the fact that there is a private field with such a name ? I guess it doesn't matter anyways since we are doing away with binary-packages.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Mar 30, 2019

Are there any security implications of leaking the fact that there is a private field with such a name ?

No, it's outside of Go's security model to keep non-exported fields confidential. There are numerous ways to enumerate them anyway; e.g., go/types or reflect.

@julieqiu julieqiu added this to the Go1.13 milestone Apr 22, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Oct 17, 2019

Thank you for the ping @cben and my apologies for the late reply, I was drowning in a bunch of work but I am finally catching up :) Good stuff advocating for better user experience in compiler error messages and sorry that it consumed your time.

Thank you @agnivade for the question about revealing what's exported or unexported: I've always wondered about that question too, and I think that in most contexts it is a valid security issue e.g. when doing an RPC, one can start to phish for fields, for example if am trying to guess the size of a struct that's remote and I want to examine what fields it has e.g. with Go Cloud Functions, but that might be too contrived and also the ease of use is what comes first so I agree with @mdempsky here that we shouldn't be sweating about the security model, but also in deed, one can use go/types or reflect.

@mdempsky thank you for the error message suggestions

invalid key doneChan: cannot refer to unexported field or method doneChan
invalid key noSuchPrivate: type http.Server has no field or method noSuchPrivate

However, I am not too fond of using "invalid key". "key" is valid for a struct field but for a method, that becomes awkward. I can whip up a CL shortly, that'll be simple and we can discuss on there what the messages should look like.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 17, 2019

Change https://golang.org/cl/201657 mentions this issue: cmd/compile: improve error when setting unexported fields

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 17, 2019

However, I am not too fond of using "invalid key". "key" is valid for a struct field but for a method, that becomes awkward.

"Key" is what the spec calls the syntax before the colon in a composite literal element: https://golang.org/ref/spec#Composite_literals

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Oct 17, 2019

Thanks Matthew! Yes, I am aware :) Perhaps I should have said that using "Key" in a message about a "method" seemed awkward to me, but on re-evaluating SGTM.

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