x/tools/cmd/goimports: eliminates imports whose package name doesn't match last path component

[kenan435]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8.1 linux/amd64

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

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

What did you do?

Used goimports instead of gofmt tool to format https://github.com/kubernetes/dashboard codebase.

What did you expect to see?

Fixing imports and format code in the same style as gofmt.

What did you see instead?

Code got formated and imports nicely ordered where standard libraries are listed first and 3rd party imports second. Unfortunately in several files legit imports got removed causing errors and failed build. Naming the imports by hand fixes the problem but the behavior is unpredictable as I don't see why the specific files were affected.

[kenan435]

Seems like the imports which have "-" get deleted. Naming them solves the problem. Is this intended behavior?

for ex. "github.com/emicklei/go-restful" gets removed while "restful github.com/emicklei/go-restful" does not.

[josharian]

Thanks for the bug report. (Btw, please update to Go 1.8.3, and try out the 1.9 betas!)

goimports assumes that the package's name (package restful) matches the last component of the package path (go-restful). This allows it to do much less work when scanning GOPATH. So in general, this is expected behavior.

That said, it seems to me:

(1) if you already have the import present in your file, goimports should always check that import path when looking for matches
(2) goimports might want to also consider common prefixes, like 'go-' (although I'm pretty sure @bradfitz won't be keen on this)

[ALTree]

This is #17546, which was recently closed.

Note that in that case he was using worker-pool in the path and workerpool as package name. In your case, even ignoring dashes in goimports won't fix the issue because your path have dashes plus an alphanumeric prefix.

I'm not too convinced it's worth complicating goimports matching rules with new ignore-prefixes rules and whatnot when we have a simple rule (package name must match) and a simple workaround for when the rule causes issues (use a named import).

[bradfitz]

@kenan435, can you (or anybody else here) prepare a reproducible example with versions?

Which git version of goimports?
Which git checkout of Kubernetes?
Running goimports on which file?

[ALTree]

$ cd $GOPATH/src/golang.org/x/tools
$ git log | head -n3
commit 3da34b1b520a543128e8441cd2ffffc383111d03
Author: Matthew Dempsky <mdempsky@google.com>
Date:   Mon Jul 17 14:44:42 2017 -0700

So it's the current (2017-07-24) tip of golang/tools.

To reproduce on github.com/kubernetes/dashboard at current tip (currently 43ee97eeaf9b84354bf4e7bccafe84f2a5f8064e):

$ git clone https://github.com/kubernetes/dashboard.git
$ cd dashboard/src/app/backend
$ goimports -d client/manager.go

diff -u client/manager.go.orig client/manager.go
--- client/manager.go.orig	2017-07-24 18:25:27.890511175 +0200
+++ client/manager.go	2017-07-24 18:25:27.890511175 +0200
@@ -20,7 +20,6 @@
 	"log"
 	"strings"
 
-	"github.com/emicklei/go-restful"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"

The dashed import was removed but that path is required because it provides the restful package, which is used in the file.

[bradfitz]

@ALTree, thanks!

[josharian]

I'm not too convinced it's worth complicating goimports matching rules with new ignore-prefixes rules and whatnot when we have a simple rule

I'm on the fence about this. In any case, I do think goimports should check existing imports before deleting them (my option 1 above). So let's leave this bug open for that.

[bradfitz]

Definitely seems like there's a bug here. I haven't investigated, though, and won't have time to (https://groups.google.com/forum/#!topic/golang-dev/5QdnNBf1v68).

[Merovius]

@ALTree I tried to reproduce with those versions, but couldn't. Specifically, I did:

export GOPATH=$(mktemp -d)
go get -d golang.org/x/tools/cmd/goimports github.com/kubernetes/dashboard/src/app/backend/client
cd $GOPATH/src/golang.org/x/tools/
git checkout 3da34b1b520a543128e8441cd2ffffc383111d03
go install golang.org/x/tools/cmd/goimports
cd $GOPATH/src/github.com/kubernetes/dashboard/
git checkout 43ee97eeaf9b84354bf4e7bccafe84f2a5f8064e
cd src/app/backend/client
$GOPATH/bin/goimports -d manager.go

This shows no diffs, however.

I also tried to build a minimum working example (on the hypothesis above, that this is happening if a package contains a dash and the package name deviates from the last component) by having this (and a couple of variations on it):

mero@hix /tmp/tmp.psB4lsxaXa$ echo $GOPATH
/tmp/tmp.psB4lsxaXa
mero@hix /tmp/tmp.psB4lsxaXa$ tree
.
└── src
    ├── b
    │   └── b.go
    └── go-a
        └── a.go

3 directories, 2 files
mero@hix /tmp/tmp.psB4lsxaXa$ cat src/b/b.go 
package main

import (
	"fmt"
	"go-a"
)

func main() {
	fmt.Println(a.Foo())
}
mero@hix /tmp/tmp.psB4lsxaXa$ cat src/go-a/a.go 
package a

func Foo() string {
	return "Hello world"
}
mero@hix /tmp/tmp.psB4lsxaXa$ goimports -d src/b/b.go 

My go installation itself is currently on tip, but I also couldn't reproduce with go 1.8.1. Did you use a clean GOPATH?

(I found this issue looking for something productive to contribute for the first time; so I might be missing something completely obvious and/or stupid :) )

[ALTree]

@Merovius Thanks for double checking. I still can reproduce this but now I notice that in the instructions I gave above I didn't go get the kubernetes tool, I just git cloned the repository outside gopath since it's not strictly a Go package. Here's a clearer reproducer that works for me (I didn't bother checking out the old goimport version because I still can reproduce the issue):

$ export GOPATH=$(mktemp -d)
$ go get -d golang.org/x/tools/cmd/goimports
$ cd $GOPATH/src/golang.org/x/tools/
$ go install golang.org/x/tools/cmd/goimports
$ cd 
$ git clone https://github.com/kubernetes/dashboard.git
$ git checkout 43ee97eeaf9b84354bf4e7bccafe84f2a5f8064e
$ cd dashboard/src/app/backend/
$ $GOPATH/bin/goimports -d client/manager.go
diff -u client/manager.go.orig client/manager.go
--- client/manager.go.orig	2017-08-04 00:21:33.507463531 +0200
+++ client/manager.go	2017-08-04 00:21:33.507463531 +0200
@@ -20,7 +20,6 @@
 	"log"
 	"strings"
 
-	"github.com/emicklei/go-restful"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"

I don't recall if this use case is supported by goimports.