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: fails to distinguish package from global variable #7463

Closed
gopherbot opened this issue Mar 4, 2014 · 16 comments
Closed

x/tools/cmd/goimports: fails to distinguish package from global variable #7463

gopherbot opened this issue Mar 4, 2014 · 16 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 4, 2014

by peterbourgon:

$ go version
go version go1.2.1 darwin/amd64

$ cat a.go
package main

import (
        logpkg "log"
        "os"
)

var (
        log = logpkg.New(os.Stdout, "", 0)
)

func main() {
        b()
}

$ cat b.go
~/src/tmp/logerr cat b.go
package main

func b() {
        log.Printf("hello")
}

$ goimports -d b.go
goimports -d b.go
diff b.go gofmt/b.go
--- /var/folders/lj/1qllh78d2qqbyxkdg5jldcym0000gn/T/gofmt154060612     2014-03-04
14:56:26.000000000 +0100
+++ /var/folders/lj/1qllh78d2qqbyxkdg5jldcym0000gn/T/gofmt436718547     2014-03-04
14:56:26.000000000 +0100
@@ -1,5 +1,7 @@
 package main

+import "log"
+
 func b() {
        log.Printf("hello")
 }

goimports doesn't detect that log.Printf in b.go refers to the global variable
"log" defined in a.go, and erroneously imports the log package. Put another
way, the expected diff is nothing.
@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 7, 2014

Comment 1:

goimports works on a file-by-file basis like gofmt; and as such it does not and cannot
know that file a.go and b.go belong to the same package.
Put another way, this is a user error for the tool as is.
Leaving to bradfitz to decide whether to close or convert to a feature request.

Owner changed to @bradfitz.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 7, 2014

Comment 2:

Labels changed: added repo-tools.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 8, 2014

Comment 3:

I'm not motivated to change this. Seems rare enough. goimports strives to be convenient
most of the time, but it doesn't need to be perfect.
I wouldn't argue against somebody else fixing it if it can be fixed simply, but I'm not
sure how.

Owner changed to ---.

Status changed to Thinking.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Apr 8, 2014

Comment 4 by peterbourgon:

> Seems rare enough.
I use this idiom in most of my servers, to aid in testing.
@rsc
Copy link
Contributor

@rsc rsc commented May 21, 2014

Comment 5:

Labels changed: added release-none.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jun 12, 2014

Comment 6 by pj@born2code.net:

> Seems rare enough.
I use this idiom in most of my packages as well
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 12, 2014

Comment 7:

Labels changed: added restrict-addissuecomment-commit.

@golang golang locked and limited conversation to collaborators Dec 8, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed the release-none label Apr 10, 2015
@rsc rsc changed the title go.tools/cmd/goimports: fails to distinguish package from global variable x/tools/cmd/goimports: fails to distinguish package from global variable Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@golang golang unlocked this conversation Oct 23, 2015
@abourget
Copy link

@abourget abourget commented Mar 2, 2016

I'm hitting the same issue here.. exactly with that log thing..

Also, not recognizing the global var massively slows down goimports, because it seems to initiate a search in all $GOPATH code (there's quite a bit in there).

@abourget
Copy link

@abourget abourget commented Mar 2, 2016

When working with such a package, goimports actually becomes a hinderance, because it adds unwanted things that then need to be fixed manually, on each run.

I understand we want to keep goimports work on a single file.. could we annotate things, then ? (not a superb solution either.. bbbuuuttt! :)

@powerman
Copy link

@powerman powerman commented Apr 18, 2016

Well, import _ "log" will make goimports happy.

@graffic
Copy link

@graffic graffic commented May 7, 2016

I must say that this happens only some times. I have a package with multiple files. One file has the famous log variable.

For months I didn't hit this bug, but today it started to happen in one file only.

@powerman
Copy link

@powerman powerman commented May 7, 2016

For months I didn't hit this bug, but today it started to happen in one file only.

Probably you have used some other fields/methods on your log variable which helps goimports to distinguish between correct and wrong log packages, but this has change and now you're using only fields/methods compatible with standard log package in this file, so this changed goimports result.

@graffic
Copy link

@graffic graffic commented May 9, 2016

Probably you have used some other fields/methods on your log variable which helps goimports to distinguish between correct and wrong log packages, but this has change and now you're using only fields/methods compatible with standard log package in this file, so this changed goimports result.

Spot on.

Anyway, I solved the issue changing the variable name from log to logger.

@briantkennedy
Copy link

@briantkennedy briantkennedy commented May 25, 2016

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 26, 2016

CL https://golang.org/cl/23444 mentions this issue.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 21, 2016

CL https://golang.org/cl/25140 mentions this issue.

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
8 participants
You can’t perform that action at this time.