x/tools/cmd/goimports: incorrect removal of imports #29557
Comments
bradfitz
added
the
NeedsFix
|
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. |
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. |
|
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. |
Isn’t that what happened with Russ, above? |
|
Er, maybe. I thought he was deliberately exploring corner cases. Russ? |
|
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. |
|
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. |
|
Change https://golang.org/cl/158198 mentions this issue: |
|
Change https://golang.org/cl/158199 mentions this issue: |
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>
|
@ianthehat fixed this. |
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>
In GOPATH mode:
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
The text was updated successfully, but these errors were encountered: