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: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. constants, integer and string literals #28393

Open
deanveloper opened this Issue Oct 25, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@deanveloper

deanveloper commented Oct 25, 2018

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

go version go1.11.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN="/home/24G/.bin"
GOCACHE="/home/24G/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/24G/go"
GOPROXY=""
GORACE=""
GOROOT="/snap/go/2890"
GOTMPDIR=""
GOTOOLDIR="/snap/go/2890/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-build434234612=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/uU1XNXaFYG-

What did you expect to see?

This isn't a bug report, that's exactly what I expected haha. But the purpose of this issue is to get a more helpful error message.

What did you see instead?

cannot take the address of 5

This is quite repulsive toward beginners. People who encounter this error may not understand exactly how constant values work, and what the differences between constants and normal values (after all, they look a lot like variables). Pointers are extremely hard for beginners to deal with, and changing this error message will make the language a bit more approachable.

This is especially unintuitive because &StructValue{...} is okay, but &5 is not.

I propose instead that the message is changed to cannot take the address of the constant value <const>. This should at least explain a reason why the value is not addressable.

If there are cases where this is error message change would be unhelpful, that would be important to consider.

@gopherbot gopherbot added this to the Proposal milestone Oct 25, 2018

@gopherbot gopherbot added the Proposal label Oct 25, 2018

@odeke-em odeke-em changed the title from proposal: cmd/compile: use a slightly more useful error message for taking the address of a constant to cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. an untyped constant Oct 25, 2018

@odeke-em

This comment has been minimized.

Member

odeke-em commented Oct 25, 2018

Thank you @deanveloper for filing this bug and welcome to the Go project!

Sounds like a plan, I've updated the title of this issue with the premise of providing a more explanatory error message for trying to take the address of an unaddressable value

Kindly paging @griesemer @mdempsky, what might you think of these error message suggestions?

a)

cannot take the address of integer literal 5

b)

cannot take the address of 5 (unaddressable because it is an integer literal)

I can work on this sometime soon when I get some free cycles.

EDIT:

const s = 5 is a still constant and &s would be still valid.

Oh my, I really need to start sleeping enough, I didn't mean to type that and even thought about it in my head as a counter example but mis-coordinated the typing

@odeke-em odeke-em changed the title from cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. an untyped constant to cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. integer and string literals Oct 25, 2018

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 25, 2018

I'm sympathetic to making error messages more accessible to new users, but it's not obvious to me that this proposal accomplishes that.

I'm not sure I understand how "cannot take the address of the constant value 5" or "cannot take the address of integer literal 5" is clearer to beginners than "cannot take the address of 5", or how it explains why the value is not addressable. All it does is further emphasize that "5" is a constant value or integer literal, which I expect users already know. (If not, should we emphasize that every time they misuse 5?)

I also don't really like the "unaddressable because it is an integer literal." It's actually unaddressable because it's not one of the cases the Go spec enumerates as addressable: https://golang.org/ref/spec#Address_operators

Is there objective evidence that any of these error messages would be more intuitive to users? What error messages do compilers for other languages emit in similar situations?

@deanveloper

This comment has been minimized.

deanveloper commented Oct 25, 2018

It's also noteworthy that constants are not addressable, not just literals.

A better error message would be a lot more useful though in the following case, which is probably what I should have led with: https://play.golang.org/p/G_IxBn_xRz1

I don't have any objective evidence right now. Currently doing this from my phone, I'll look into it when I can though.

@odeke-em odeke-em changed the title from cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. integer and string literals to cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. constants, integer and string literals Oct 25, 2018

@odeke-em odeke-em changed the title from cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. constants, integer and string literals to proposal: cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. constants, integer and string literals Oct 25, 2018

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 25, 2018

I'll admit named constants are a little more compelling, since names can also refer to variables, which are addressable. Similarly, for certain indexing expressions (map and string indexing, and indexing of non-addressable arrays), since other syntactically identical index expressions are addressable.

I think elaborating the error messages could be reasonable when syntax alone is insufficient to explain why the expression isn't addressable.

However, I'm curious what error folks suggest to use for:

var m map[int][10]struct{ f int }
_ = &m[0][0].f  // ERROR

Maybe something like:

cannot take address of m[0][0].f (m[0] is map index expression)

I.e., drill down past the field selections and array indexing to identify the root non-addressable expression.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Oct 26, 2018

What @mdempsky said.

I think the error message is pretty clear, but we could go along the route chosen for binary operators. For instance, for "a" - "b" we get:

invalid operation: "a" - "b" (operator - not defined on string)

Analogously, here we could report:

invalid operation: &5 (operator & not defined on non-addressable 5)

But it really says the same thing, slightly less elegantly.

But I'm also not clear which part of the error message is "repulsive" to newcomers. If one is truly a newcomer, I suspect it's not even clear that & means "take the address" - so a peek into the spec is warranted anyway. If one comes from C, the error message seems "obvious". Maybe this is better:

invalid operation: &5 (constant 5 is not addressable)

Either way, this doesn't need to be a proposal. If a convincingly better error message comes along, I'm ok with changing this; but I don't see the current error as overly problematic.

@griesemer griesemer closed this Oct 26, 2018

@griesemer

This comment has been minimized.

Contributor

griesemer commented Oct 26, 2018

Closed by mistake. Re-opening.

@griesemer griesemer reopened this Oct 26, 2018

@griesemer griesemer added NeedsDecision and removed Proposal labels Oct 26, 2018

@gopherbot gopherbot added the Proposal label Oct 26, 2018

@griesemer griesemer modified the milestones: Proposal, Unplanned Oct 26, 2018

@griesemer griesemer removed the Proposal label Oct 26, 2018

@griesemer griesemer changed the title from proposal: cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. constants, integer and string literals to cmd/compile: provide a more explanatory error message on trying to take the address of an unaddressable value e.g. constants, integer and string literals Oct 26, 2018

@deanveloper

This comment has been minimized.

deanveloper commented Oct 26, 2018

invalid operation: "a" - "b" (operator - not defined on string)

invalid operation: &5 (operator & not defined on non-addressable 5)

These aren't quite the same, if we make the second one more like the first, the error message is a lot more useful:

with literal values

invalid operation: &5 (operator & not defined on literal values)

with named constants

invalid operation: &foo (operator & not defined on constant expressions)

with function calls

invalid operation: &bar() (operator & not defined on function calls)

a "catch all" error message for expressions that really fit with any the above

invalid operation: &(-foo) (operator & not defined on non-addressable expressions)

I think cannot take address of <expression> (<explanation>) is a bit more clear, but invalid operation is a bit more consistent.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 26, 2018

I'd argue against using "invalid operation" for more error messages. While technically accurate (the expression is an operation expression and it's invalid), I think when most users see "invalid operation," they think of https://www.google.com/search?tbm=isch&q="invalid+operation" and we might as well just say "error".

I think "cannot take address of foo (foo is a constant)" is better than "invalid operation: &foo (operator & not defined on constants)". The former tells you directly what the error was (cannot take address), and gives you additional details as to why that's the case (foo is a constant). The latter instead tells you something that experienced Go programmers already know (you can't take the address of constants) and leaves it to you to infer how that's relevant here (foo is a constant).

Also, I think the error message "invalid operation: &m[0][0].f (operator & not defined on map index expressions)" is misleading, because m[0][0].f is not a map index expression; it's a field selection expression. But it's non-addressable because of the m[0] map index expression.

@deanveloper

This comment has been minimized.

deanveloper commented Oct 26, 2018

Also, I think the error message "invalid operation: &m[0][0].f (operator & not defined on map index expressions)" is misleading, because m[0][0].f is not a map index expression; it's a field selection expression. But it's non-addressable because of the m[0] map index expression.

You're definitely right about that, and I've now removed that example. It should just be caught by the "catch-all" if we do the "invalid operation" route.

Something great about the cannot take address of foo (foo is a constant) is that it is an explanation to newcomers, and it's informative to experienced users (maybe we didn't know foo was a constant). As another upside, it doesn't sound condescending. It's a pretty hard balance to strike a balance like that when you're communicating through a text console.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment