x/tools/cmd/goimports: Report package name != base src directory

[Dominik-K]

go version go1.9.2 darwin/amd64
golang.org/x/tools/cmd/goimports @ 64890f4

Using imported package surprise (in path a/packg) as packg, not surprise. goimports silently removes this import, so does Visual Studio Code (perhaps other IDEs as well).

File a/main.go:

package main

import "a/packg"

func main() {
	packg.SomeFunc()
}

and package file a/packg/lib.go:

package surprise

import "fmt"

func SomeFunc() {
	fmt.Println("Hello from surprise.")
}

This silent removing confuses a lot (again a new colleague/Go newbie stumbled upon this). That's why go build explicitly shows the package name difference to the base of the import path since Go 1.2 (see #5957):

# command-line-arguments
./main.go:3:8: imported and not used: "a/packg" as surprise
./main.go:6:2: undefined: packg

What did you expect to see?

An error on stderr like this:

./main.go:3:8: used as "packg" but package name is "surprise". Skipped removing (flag "--no-rm-pkg-name-differ" set)

and a flag --no-rm-pkg-name-differ (working title) to skip removing this import that IDEs can opt-in to show this error at this line instead.

I think it should be a part of goimports because IDEs will first run goimports before go build to add maybe needed imports (neat feature 👍, btw) which will otherwise fail on build.

Not using flags at all for goimports (see recent #23004 (comment)) and sticking to Go convention

Another convention is that the package name is the base name of its source directory; the package in src/encoding/base64 is imported as "encoding/base64" but has name base64[...]

would mean that goimports should fail in this scenario (also go build?!). This would make goimports backwards-incompatible and IDEs need to handle this. I think this is not the preferred way, so that's why I'm suggesting the flag and logging to stderr instead.

What did you see instead?

goimports -v main.go output:

2017/12/19 20:15:40.420475 fixImports(filename="main.go"), abs="/Users/dominik/go/src/a/main.go", srcDir="/Users/dominik/go/src/a" ...
2017/12/19 20:15:40.420845 importPathToNameGoPathParse("a/packg", srcDir="/Users/dominik/go/src/a") = "surprise", <nil>
2017/12/19 20:15:40.420991 open /Users/dominik/go/src/.goimportsignore: no such file or directory
2017/12/19 20:15:40.421005 scanning $GOPATH
2017/12/19 20:15:40.421117 scanning $GOROOT
2017/12/19 20:15:40.434210 scanned $GOROOT
2017/12/19 20:15:40.711721 goimports: scanning directory /Users/dominik/go/src: permission denied
2017/12/19 20:15:40.711735 scanned $GOPATH
package main

func main() {
	packg.SomeFunc()
}

[bradfitz]

We're not adding a flag.

I'm pretty sure goimports already correctly handles this case, and we have tests for it too.

Note that permission denied error in your log?

I suspect that goimports is unable to scan your $GOPATH so it's unable to learn that a/packg is package surprise and thus it falls back to assuming you named it conventionally, and then it discards it.

Fix your permission error and then let's see what happens.

[Dominik-K]

Couldn't figure out yet why there's a permission denied but saw with debugging that goimports walks through the $GOPATH. The 2nd line also shows that it could retrieve the package name:

importPathToNameGoPathParse("a/packg", srcDir="/Users/dominik/go/src/a") = "surprise", <nil>

Anyway, I worked around the permission denied and now see this extra helpful lines (full output below):

packg candidate 1/1: a/packg in /tmp/src/a/packg
loading exports in dir /tmp/src/a/packg (seeking package packg)
scan of dir /tmp/src/a/packg is not expected package packg (actually surprise)

However, these lines don't seem enough that an IDE can (easily) act on to show the deletion reason at the former line import "a/packg", does it? At least a hint to the file & line of the import in question would be helpful.

Sidenote: The 3 lines above don't show up if you use the imported package correctly, e.g. like this:

package main

import "a/packg"

func main() {
	surprise.SomeFunc()
}

Full output (from GOPATH=/tmp/ goimports -v main.go):

2017/12/20 11:47:42.591341 fixImports(filename="main.go"), abs="/tmp/src/a/main.go", srcDir="/tmp/src/a" ...
2017/12/20 11:47:42.591652 importPathToNameGoPathParse("a/packg", srcDir="/tmp/src/a") = "surprise", <nil>
2017/12/20 11:47:42.591796 open /tmp/src/.goimportsignore: no such file or directory
2017/12/20 11:47:42.591808 scanning $GOPATH
2017/12/20 11:47:42.591869 scanning $GOROOT
2017/12/20 11:47:42.592148 scanned $GOPATH
2017/12/20 11:47:42.601379 scanned $GOROOT
2017/12/20 11:47:42.602617 packg candidate 1/1: a/packg in /tmp/src/a/packg
2017/12/20 11:47:42.602652 loading exports in dir /tmp/src/a/packg (seeking package packg)
2017/12/20 11:47:42.603156 scan of dir /tmp/src/a/packg is not expected package packg (actually surprise)
package main

func main() {
	packg.SomeFunc()
}

[bradfitz]

However, these lines don't seem enough that an IDE can (easily) act on to show the deletion reason at the former line import "a/packg", does it? At least a hint to the file & line of the import in question would be helpful.

Those lines are for people debugging goimports while developing goimports. They're not for users or IDEs.

It looks like everything is happening properly: when you don't use the import it's removed, and when you do use it, it's not removed.

You want to get warnings, but goimports isn't in the business of warnings.

That is probably the job of a lint tool, as it's more of a style issue.

I'm going to close this, as I see nothing for goimports to do. Let me know if I misunderstand, though.