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

Jump to bottom

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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rogpeppe
Copy link
Contributor

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 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 commented Dec 7, 2017 •
edited

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 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 bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Dec 7, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Dec 7, 2017

Hey @kazrakcom, could you look into this?

@mvdan
Copy link
Member

mvdan commented Dec 7, 2017

I pointed at the wrong CL then - apologies.

@rogpeppe
Copy link
Contributor Author

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

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

@fraenkel fraenkel mentioned this issue Dec 22, 2017
Closed
@fraenkel
Copy link
Contributor

fraenkel commented Jan 4, 2018

Can someone review the change?

@rileykarson rileykarson mentioned this issue Dec 12, 2018
Closed
@golang golang locked and limited conversation to collaborators Jan 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @rogpeppe @fraenkel @mvdan @gopherbot @henryas