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 column number in build error for unused variable #21317

Closed
bradleyfalzon opened this issue Aug 5, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@bradleyfalzon
Copy link

commented Aug 5, 2017

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

go version go1.9rc1 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/bradleyf/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build365274238=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

package main

import "fmt"

func main() {
        n, err := fmt.Print(1)
}

Run this program with go build

What did you expect to see?

./main.go:6:2: n declared and not used
./main.go:6:5: err declared and not used

Note the column numbers.

What did you see instead?

./main.go:6:22: n declared and not used
./main.go:6:22: err declared and not used

Column 22 for the above program is the argument to the fmt.Print function. There's nothing wrong with the argument, the problem is with the returned arguments.

It appears this may be a known issue, mentioned in 2a5cf48 but it's not clear whether this is the same issue.

At the very least, when the editor is highlighting the argument but the error is elsewhere on the same line, it's confusing.

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

@bradleyfalzon

This comment has been minimized.

Copy link
Author

commented Aug 7, 2017

Another example which I most likely the same issue but it is confusing when the editor is highlighting the wrong identifier:

package main

func main() {
        var a int64 = 1
        Foo("SOMEVAR", a)
}

func Foo(_ string, _ int) {}
$ go build
./main.go:5:6: cannot use a (type int64) as type int in argument to Foo

Column 6, in this case, is the first argument to the Foo function call, but the problem is with the second argument.

@griesemer griesemer added the NeedsFix label Aug 7, 2017

@griesemer griesemer added this to the Go1.10 milestone Aug 7, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

Haven't checked if it's a duplicate but this can be improved for sure.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

This is what a code editor that displays issues inline shows the user:

image

Note that it visually points to the 3rd parameter, the empty string, very prominently.

However, the actual issue is that the logger variable is declared and not used. There is no issue with the empty string parameter.

Emitting an incorrect column is an issue that is magnified in the case of editors that visualize that information more prominently than what's visible by glancing at terminal output only.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

@griesemer This is a feature that's new to Go 1.9, and it has a bug. What's the reason this issue is added to Go 1.10 milestone instead of 1.9?

Is it because the bug is considered to be minor and happens only in a few cases, so the feature is still overall helpful and worth being included, despite the incorrect behavior in this particular case?

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2017

@shurcooL It's not a show stopper. Yes, it's a new feature, and we know it has bugs (mostly due to the nature of how the existing code deals with position information). It can be disabled with the -C cmd/compile flag. It's definitively minor, even though it's annoying and perhaps confusing if one is using an IDE and not very familiar with the language. We will fix all these over time and the intention is to have 100% accurate positions.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

Great news, seems like @mdempsky fixed this issue with commit f22ef70 and CL https://go-review.googlesource.com/c/66810/
Running it on tip 32f994a gives me

$ go version && go run main.go 
go version devel +32f994a Mon Nov 6 07:09:50 2017 +0000 darwin/amd64
# command-line-arguments
./main.go:6:9: n declared and not used
./main.go:6:12: err declared and not used

I'll submit a test to ensure we don't regress.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 6, 2017

Change https://golang.org/cl/76150 mentions this issue: cmd/compile: lock in test for column numbers in unused error

gopherbot pushed a commit that referenced this issue Nov 6, 2017

cmd/compile: lock in test for column numbers in unused error
Updates #21317

@mdempsky fixed issue #21317 with CL 66810,
so lock a test in to ensure we don't regress.

The test is manual for now before test/run.go
has support for matching column numbers so do
it old school and match expected output after
an exec.

Change-Id: I6c2a66ddf04248f79d17ed7033a3280d50e41562
Reviewed-on: https://go-review.googlesource.com/76150
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@odeke-em

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

Alright, the regress test addressed by CL https://go-review.googlesource.com/c/go/+/76150 and commit 0c55495. Please get the latest code.

@odeke-em odeke-em closed this Nov 6, 2017

@golang golang locked and limited conversation to collaborators Nov 6, 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.