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: neighboring files cause wrong import decision #23001

Closed
rogpeppe opened this issue Dec 5, 2017 · 8 comments
Closed

x/tools/cmd/goimports: neighboring files cause wrong import decision #23001

rogpeppe opened this issue Dec 5, 2017 · 8 comments

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Dec 5, 2017

go version devel +9a13f8e Tue Nov 28 06:47:50 2017 +0000 linux/amd64

If I run goimports bar.go with the following files under $GOPATH, it imports a/y, not b/y, even though only b/y exports the function G. It seems that the import in foo.go is causing goimports to ignore the constraints that the calls are imposing. I would expect the imported package to provide all the required names - that is, even though neighboring files might be good as a starting hint, I think that goimports should still verify that the required names exist in that package, perhaps only falling back to the starting hint only if no package at all is found that satisfies the requirements (because then it's likely to be a spelling mistake).

This is a particular issue when running goimports on a file in a directory with lots of random independent main packages (e.g. my $HOME/src directory where I keep hundreds of tiny scratch Go programs).

==> src/x/bar.go <==
package x

func Q() {
	y.G()
}

==> src/x/foo.go <==
package x

import (
	"a/y"
)

func P() {
	y.F()
}

==> src/b/y/y.go <==
package y

func G() {
}

==> src/a/y/y.go <==
package y

func F() {
}
@gopherbot gopherbot added this to the Unreleased milestone Dec 5, 2017
@mvdan
Copy link
Member

@mvdan mvdan commented Dec 7, 2017

This is likely caused by #17557. /cc @henryas @fraenkel @bradfitz

I agree that the prioritization of closer packages should still take into account whether or not all the required names are available.

@henryas
Copy link

@henryas henryas commented Dec 7, 2017

I can't seem to replicate the problem. I copy-pasted your code and recreated the directories, goimports correctly inserts "b/y" into bar.go.

Have you tried reinstalling goimports?

Since I am running the test on Windows, I wonder whether this is Linux specific bug?

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Dec 7, 2017

The problem is actually caused by
golang/tools@e011c10

It seems there is no validation that the sibling's choice is actually valid. Once I remove the sibling check, the answer becomes b/y.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 7, 2017

Hey @kazrakcom, could you look into this?

@mvdan
Copy link
Member

@mvdan mvdan commented Dec 7, 2017

I pointed at the wrong CL then - apologies.

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Dec 7, 2017

I can't seem to replicate the problem. I copy-pasted your code and recreated the directories, goimports correctly inserts "b/y" into bar.go.

I just tried again with latest Go tip (9ce6b5c) and latest x/tools tip (7165768) and I still see the issue.

FWIW the problem is not exhibited if you run goimports with standard input, because then it doesn't take neighbouring files into account.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 8, 2017

Change https://golang.org/cl/82875 mentions this issue: imports: sibling imports must have matching references

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Jan 4, 2018

Can someone review the change?

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