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
Closed

cmd/compile: incorrect name in error message #21747

dsnet opened this issue Sep 3, 2017 · 11 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone

Comments

@dsnet
Copy link
Member

dsnet 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 The path to resolution is known, but the work has not been done. label Sep 3, 2017
@dsnet dsnet added this to the Go1.9.1 milestone Sep 3, 2017
@odeke-em
Copy link
Member

odeke-em 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
Copy link
Member

mdempsky 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
Copy link

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

@mdempsky
Copy link
Member

mdempsky 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
Copy link
Member Author

dsnet 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
Copy link
Member

odeke-em commented Sep 3, 2017

Reopening for cherry picking to the Go1.9.1 branch.

@odeke-em odeke-em reopened this Sep 3, 2017
@mdempsky
Copy link
Member

mdempsky 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
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 13, 2017
@rsc
Copy link
Contributor

rsc commented Oct 13, 2017

CL 61311 OK for Go 1.9.2.

Not really worth fixing, but the fix is hilarious.

@rsc rsc added release-blocker CherryPickApproved Used during the release process for point releases and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 13, 2017
@gopherbot
Copy link

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
…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
Copy link
Contributor

rsc 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 as completed 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.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants