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: incorrect name in error message #21747

Closed
dsnet opened this issue Sep 3, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@dsnet
Copy link
Member

commented Sep 3, 2017

Consider the following (invalid) Go code:

type x struct{}

func (x) Read()

The Go1.9 compiler complains with:

./main.go:7:6: missing function body for "_"

On Go1.8, it complained with:

./main.go:7: missing function body for "x.Read"

Edit: added type x struct{}

@dsnet dsnet added the NeedsFix label Sep 3, 2017

@dsnet dsnet added this to the Go1.9.1 milestone Sep 3, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Sep 3, 2017

@dsnet it seems like the new error is a premature report, but on tip it currently reports undefined: x but after the first seemingly misleading message. Please take a look at the version comparisons below:

Go1.8

$ go version && go run main.go 
go version go1.8.3 darwin/amd64
# command-line-arguments
./main.go:3: undefined: x

Go1.9+

$ go version && go run main.go 
go version devel +f74b52c Fri Sep 1 00:42:21 2017 +0000 darwin/amd64
# command-line-arguments
./main.go:3:6: missing function body for "_"
./main.go:3:7: undefined: x

I guess we should report 3:7 before 3:6 or suppress the content of 3:6.
/cc @mdempsky @griesemer

@mdempsky

This comment has been minimized.

Copy link
Member

commented Sep 3, 2017

Interesting. I'm inclined to just truncate the for %q part of the error message. With position information, I don't think it's necessary to include the name.

We can continue including the name, but it's actually complicated somewhat because of type aliases (which is why this changed in 1.9).

@gopherbot

This comment has been minimized.

Copy link

commented Sep 3, 2017

Change https://golang.org/cl/61311 mentions this issue: cmd/compile: simplify "missing function body" error message

@gopherbot gopherbot closed this in b2e8630 Sep 3, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Sep 3, 2017

For posterity, the two changes that happened in 1.9 that are relevant to this issue:

  1. Methods have names like "T.m". We used to compute these immediately during parsing; but because of type aliases, we have to compute them during type checking instead. E.g., "func (T) m()" might end up needing the name "(*U).m" if we later find "type T = *U".
  2. We used to check for missing bodies during code generation, but we're trying to move all user errors into the compiler frontend. The parser was the natural place to relocate this check to.

If we decide we want to restore the function name, moving the error to type checking would probably work too.

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2017

I think dropping the function name is sufficient since the line should be sufficient. Usually there isn't more than one function declaration on a single line.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Sep 3, 2017

Reopening for cherry picking to the Go1.9.1 branch.

@odeke-em odeke-em reopened this Sep 3, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Sep 4, 2017

@dsnet Yeah, and even if there were multiple functions on a single line, we now have column information. We also don't include function name in any other (standard) error messages. (We do include it in some optional diagnostics used for debugging the compiler itself.)

@rsc rsc modified the milestones: Go1.9.1, Go1.9.2 Oct 4, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

This is a bad-error-on-invalid. I'm not sure this qualifies for a 1.9 backport.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

CL 61311 OK for Go 1.9.2.

Not really worth fixing, but the fix is hilarious.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 15, 2017

Change https://golang.org/cl/70972 mentions this issue: [release-branch.go1.9] cmd/compile: simplify "missing function body" error message

gopherbot pushed a commit that referenced this issue Oct 25, 2017

[release-branch.go1.9] cmd/compile: simplify "missing function body" …
…error message

Fixes #21747.

Change-Id: I6a68370be3b7510ce364ddd1e41a1d767ce3a67f
Reviewed-on: https://go-review.googlesource.com/61311
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-on: https://go-review.googlesource.com/70972
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

go1.9.2 has been packaged and includes:

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:13 UTC

@rsc rsc closed this Oct 26, 2017

@golang golang locked and limited conversation to collaborators Oct 26, 2018

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.