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: adds import colliding with package-level variable #17437

Closed
ChimeraCoder opened this issue Oct 13, 2016 · 2 comments
Closed
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@ChimeraCoder
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

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

go 1.7.1

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

linux/amd64, darwin/amd64

What did you do?

(A complete program exhibiting the behavior described blow is available at https://github.com/ChimeraCoder/goimports-log-bug/commits/master. The first commit demonstrates a working program, and the second commit captures the effects of goimports.)

The logrus package (https://github.com/Sirupsen/logrus) provides a logger whose methods are superset of the functions from the stdlib log package. So, in theory, var log = logrus.New() at the package level should be a drop-in replacement for the log package - and this is true for the file in which that package-level variable log is defined.

However, goimports seems to ignore this for other files in the same package, instead adding import "log" in those files. Not only does this not have the desired effect (using the logrus package), but it turns a working build into a broken one:

./helpers.go:3: log redeclared in this block
        previous declaration at ./main.go:7

What did you expect to see?

Ideally, goimports could check before adding an import to see if it collides with an existing package-level identifier. (In this case, the identifier it collides with already provides the methods that were missing anyway).

Though more broadly, this is the first time that I can remember in which running goimports breaks an otherwise-working build. (I'm not sure if this is explicitly an invariant promised by goimports, but it's certainly been a safe assumption most of the time!).

What did you see instead?

./helpers.go:3: log redeclared in this block
        previous declaration at ./main.go:7
@bradfitz
Copy link
Contributor

Dup of #7463 which was fixed already? When was the last time you updated your goimports binary?

@dsnet dsnet added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 13, 2016
@dsnet dsnet added this to the Unreleased milestone Oct 13, 2016
@dsnet dsnet changed the title goimports adds import colliding with package-level variable x/tools/cmd/goimports: adds import colliding with package-level variable Oct 13, 2016
@ChimeraCoder
Copy link
Contributor Author

Apparently older than I'd thought it was. Thanks - sorry for the dupe!

@golang golang locked and limited conversation to collaborators Oct 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants