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: incorrectly adds imports for variables that match package names #13023

Closed
cep21 opened this issue Oct 22, 2015 · 9 comments
Closed
Milestone

Comments

@cep21
Copy link
Contributor

@cep21 cep21 commented Oct 22, 2015

Please see the following console output as a test case. log is a varaible defined in another file and therefor does not need a package import. The expected behavior is for goimports to detect no issues.

11:19:35 jack@Jacks-MacBook-Pro.local:/tmp/b
< go get -u golang.org/x/tools/cmd/goimports
11:20:02 jack@Jacks-MacBook-Pro.local:/tmp/b
< ls -la
total 16
drwxr-xr-x   4 jack  wheel   136B Oct 22 11:18 .
drwxrwxrwt  32 root  wheel   1.1K Oct 22 11:17 ..
-rw-r--r--   1 jack  staff   111B Oct 22 11:18 a.go
-rw-r--r--   1 jack  staff    82B Oct 22 11:18 a_test.go
11:20:05 jack@Jacks-MacBook-Pro.local:/tmp/b
< cat a.go 
package main

import "github.com/Sirupsen/logrus"

var log *logrus.Logger

func init() {
    log = logrus.New()
}
11:20:07 jack@Jacks-MacBook-Pro.local:/tmp/b
< cat a_test.go 
package main

import "testing"

func TestB(t *testing.T) {
    log.Printf("Hello")
}
11:20:09 jack@Jacks-MacBook-Pro.local:/tmp/b
< goimports -d .
diff a_test.go gofmt/a_test.go
--- /var/folders/5r/x8pdxnqs0sdg7w_262278m800000gn/T/gofmt282742701 2015-10-22 11:20:12.000000000 -0700
+++ /var/folders/5r/x8pdxnqs0sdg7w_262278m800000gn/T/gofmt424960296 2015-10-22 11:20:12.000000000 -0700
@@ -1,6 +1,9 @@
 package main

-import "testing"
+import (
+   "log"
+   "testing"
+)

 func TestB(t *testing.T) {
    log.Printf("Hello")
@mdempsky mdempsky changed the title goimports incorrectly adds imports for variables that match package names x/tools/cmd/goimports: incorrectly adds imports for variables that match package names Oct 22, 2015
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Oct 22, 2015
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 22, 2015

I think we're probably going to have to chalk this up to heuristic failure, but if somebody can figure out a good fix, by all means go for it.

@cep21
Copy link
Contributor Author

@cep21 cep21 commented Oct 22, 2015

In my case, an option to reorder imports but not add new ones would do.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 22, 2015

@cep21 gofmt reorders imports without adding/removing them. Does that do what you want?

@cep21
Copy link
Contributor Author

@cep21 cep21 commented Oct 22, 2015

I don't currently see that behavior in go1.5.1. Is it new?

@cep21
Copy link
Contributor Author

@cep21 cep21 commented Oct 22, 2015

I don't observe gofmt reordering imports like goimports

15:24:59 jack@Jacks-MacBook-Pro.local:/tmp/b
< cat a.go 
package main

import (
    "fmt"
    "github.com/Sirupsen/logrus"
)

var log *logrus.Logger

func init() {
    log = logrus.New()
    fmt.Printf("Hello world")
}
15:25:02 jack@Jacks-MacBook-Pro.local:/tmp/b
< goimports -d a.go
diff a.go gofmt/a.go
--- /var/folders/5r/x8pdxnqs0sdg7w_262278m800000gn/T/gofmt609869718 2015-10-22 15:25:05.000000000 -0700
+++ /var/folders/5r/x8pdxnqs0sdg7w_262278m800000gn/T/gofmt029612029 2015-10-22 15:25:05.000000000 -0700
@@ -2,6 +2,7 @@

 import (
    "fmt"
+
    "github.com/Sirupsen/logrus"
 )

15:25:05 jack@Jacks-MacBook-Pro.local:/tmp/b
< gofmt -s -d a.go 
15:25:08 jack@Jacks-MacBook-Pro.local:/tmp/b
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 22, 2015

Ah, I suppose gofmt doesn't separate stdlib imports from third-party imports like goimports does. If you separate them yourself, it should still sort contiguous runs of imports.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 22, 2015

@ianlancetaylor Maybe goimports could run go/types on the AST and only consider ast.Idents that don't resolve to a variable/const/function/type?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 23, 2015

Dup of #7463

@bradfitz bradfitz closed this Oct 23, 2015
@golang golang locked and limited conversation to collaborators Oct 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.