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: incorrect removal of imports #29557

Closed
rsc opened this issue Jan 4, 2019 · 10 comments
Closed

x/tools/cmd/goimports: incorrect removal of imports #29557

rsc opened this issue Jan 4, 2019 · 10 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Jan 4, 2019

In GOPATH mode:

$ rm -rf $GOPATH/src/gopkg.in/russross/blackfriday.v2
$ echo 'package p; import "gopkg.in/russross/blackfriday.v2"; var _ blackfriday.Node' | goimports
package p

var _ blackfriday.Node
$ 

Apparently because the package does not exist locally, goimports can't find out its package name, so goimports decides to remove the import entirely! This is incorrect and should not happen.

Goimports should never remove an import statement for a package that it simply cannot find.

/cc @heschik @bradfitz

@gopherbot gopherbot added this to the Unreleased milestone Jan 4, 2019
@bradfitz bradfitz added the NeedsFix label Jan 4, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

This is exactly the kind of problem that adding names to mismatched imports is intended to help prevent. If the import were named blackfriday, goimports would know not to remove it. So this could only happen if a user ran goimports on a file for the first time (before the name was added) and if they were doing it in a situation where they can't actually compile the package.

I believe that goimports has always removed apparently-unused imports, even when the file isn't fully satisfied, and that's the behavior I tried to preserve in my rewrite. We could certainly change it not to remove imports from an unsatisfied file. But I'm nervous about making any change to behavior without a strong reason; so far, almost everything I've done, deliberately or not, has caused some trouble for somebody in ways I would not have predicted. I don't find this case particularly compelling.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 4, 2019

I believe that goimports has always removed apparently-unused imports, even when the file isn't fully satisfied, and that's the behavior I tried to preserve in my rewrite.

It's true that's always been the behavior, but I'd never considered having different behavior whether or not a likely-matching package is unavailable.

During development you tend to have the code you want to use.

But if you're editing somebody else's code (that you git cloned by hand rather than using go get?) then it'd be weird if we removed import statements even if they're plausible correct (e.g. "blackfriday.v2" contains the substring "blackfriday", and we're already doing that substring search when building up the list of candidate packages to scan and look for package names)

So I think would be a reasonable change.

@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

Considered or not, the old version still tried to load real package names and fell back to a guess when it couldn't, so I don't think this is a behavior change. I guess it doesn't matter what it used to do as long as we can agree on what the right thing is.

My thought was that it shouldn't remove imports unless the file was fully satisfied. I think that would cause trouble for Blaze/Bazel users, and generally anyone who adds generated code to a package but doesn't check it in. Any file that uses a global variable from a generated file, say a go_embed_data rule, will be permanently unsatisfied. So that's probably not a good idea.

This is only a problem to the extent that people aren't using goimports and adding import names during development. A heuristic like you describe would be pretty easy to add, but I'd personally rather wait for an actual user complaint first.

@josharian
Copy link
Contributor

@josharian josharian commented Jan 4, 2019

I'd personally rather wait for an actual user complaint first.

Isn’t that what happened with Russ, above?

@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

Er, maybe. I thought he was deliberately exploring corner cases. Russ?

@rsc
Copy link
Contributor Author

@rsc rsc commented Jan 7, 2019

Please assume that every bug I file is something I personally ran into, unless otherwise noted. I don't go out of my way looking for problems. I just run goimports on save and get pretty frustrated when it breaks my program.

@heschik
Copy link
Contributor

@heschik heschik commented Jan 7, 2019

Okay. I just confirmed that this behavior is unchanged since before I touched goimports, so I'm still skeptical that this is a common enough problem to address. But I'll add some kind of heuristic after I'm done with other goimports work. Hopefully it doesn't cause too much trouble.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 16, 2019

Change https://golang.org/cl/158198 mentions this issue: imports: don't eagerly guess package names

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 16, 2019

Change https://golang.org/cl/158199 mentions this issue: imports: don't eagerly guess package names

gopherbot pushed a commit to golang/tools that referenced this issue Jan 18, 2019
Before this change, we guessed package names during loadPackageNames
when necessary. That made it impossible to tell if we failed to load
a package's name, e.g. because its source code is unavailable. That
interferes with fixes for golang/go#29557. It also meant that each
implementation of package name loading needs to do the guessing,
which gets annoying in CL 158097.

Instead, leave package names unset unless we're certain about them, and
guess only in (*pass).importIdentifier when necessary.

Refactoring only; no user-visible changes intended.

Change-Id: Ia6072ada823e6e3a86981ad90228f30baa2ac708
Reviewed-on: https://go-review.googlesource.com/c/158199
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@heschik heschik assigned ianthehat and unassigned heschik Jan 22, 2019
@rsc
Copy link
Contributor Author

@rsc rsc commented Feb 5, 2019

@ianthehat fixed this.

@rsc rsc closed this Feb 5, 2019
phlogistonjohn added a commit to phlogistonjohn/heketi that referenced this issue Feb 5, 2019
It appears that in quite a few of our (totally valid) files the
goimports tool gets confused and removes valid imports.
The problem is similar to that mentioned in
  golang/go#29557
but I am not entirely sure that's the exact issue.
So until all our files are goimports safe or goimports behavior
is improved I'm disabling the check script.

FWIW this script was never run in travis ci as it appears goimports
tool was not added to our travis instances and was not
immediately caught when the script was added.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@golang golang locked and limited conversation to collaborators Feb 5, 2020
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.