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

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

Closed
kenan435 opened this issue Jul 24, 2017 · 13 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kenan435
Copy link

kenan435 commented Jul 24, 2017

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.

@gopherbot gopherbot added this to the Unreleased milestone Jul 24, 2017
@kenan435
Copy link
Author

kenan435 commented Jul 24, 2017

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 josharian changed the title x/tools/cmd/goimports: x/tools/cmd/goimports: eliminates imports whose package name doesn't match last path component Jul 24, 2017
@josharian
Copy link
Contributor

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

ALTree commented Jul 24, 2017

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 bradfitz added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 24, 2017
@bradfitz
Copy link
Contributor

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

ALTree commented Jul 24, 2017

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

@ALTree, thanks!

@josharian
Copy link
Contributor

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

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

Merovius commented Aug 3, 2017

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

ALTree commented Aug 3, 2017

@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.

@Merovius
Copy link
Contributor

Merovius commented Aug 3, 2017

@ALTree Yes, I can reproduce that now (with the additional step of checking out anything before c267afd3ecb on the kubernetes tree, of course). It shows a diff if the repo is checked out outside of $GOPATH and no diff, if it's inside $GOPATH.

I'm going to try and look whether I can find and fix this over the next couple of days, supported or not. Unless someone tells me to stop :)

@Merovius
Copy link
Contributor

Merovius commented Aug 5, 2017

Minimal repro:

mero@hix ~/goissues/21143/min_repro$ echo $GOPATH
/home/mero/goissues/21143/min_repro/gopath
mero@hix ~/goissues/21143/min_repro$ tree
.
├── b
│   ├── b.go
│   └── vendor
│       ├── c
│       │   └── a.go
│       └── d
│           └── d.go
└── gopath
    └── src
        └── b -> ../../b

7 directories, 3 files
mero@hix ~/goissues/21143/min_repro$ cat b/b.go 
package main

import (
	"c"
	"d"
	"fmt"
)

func main() {
	fmt.Println(a.Foo())
	fmt.Println(d.Bar())
}
mero@hix ~/goissues/21143/min_repro$ cat b/vendor/c/a.go 
package a

func Foo() string {
	return "Hello world"
}
mero@hix ~/goissues/21143/min_repro$ cat b/vendor/d/d.go
package d

func Bar() string {
	return "Hello Bar"
}
mero@hix ~/goissues/21143/min_repro$ goimports -d b/b.go 
diff -u b/b.go.orig b/b.go
--- b/b.go.orig	2017-08-05 18:12:23.647743753 +0200
+++ b/b.go	2017-08-05 18:12:23.647743753 +0200
@@ -1,7 +1,6 @@
 package main
 
 import (
-	"c"
 	"d"
 	"fmt"
 )
mero@hix ~/goissues/21143/min_repro$ goimports -d gopath/src/b/b.go
mero@hix ~/goissues/21143/min_repro$ 

Meaning this is #14566. goimports doesn't associate the "c" import with package "a" by heuristic and build.Import doesn't find the vendored directory. I don't think it's very reasonable to expect goimports to work around this. It already tries to find existing imports to see if the package name doesn't match (as seen by it working in $GOPATh) and uses a heuristic, if it can't.

I recommend closing this as a dupe of #14566 (I don't think I can do that myself).

@ALTree
Copy link
Member

ALTree commented Aug 7, 2017

Thanks for investigating.

Since it can only be reproduced when the dashboard repo is outside gopath, and the tool does not support this use case (see #14566), I'm closing the issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants