x/tools/cmd/goimports: import incorrectly rewritten

[pkitszel]

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

$ go version
`go version go1.15.6 linux/amd64`

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/przemek/.cache/go-build"
GOENV="/home/przemek/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/przemek/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/przemek/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build097643344=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I just go-get'ed goimports to format my code as usual:
goimports -w -l *.go
Then one of imports was changed from "github.com/Komosa/persistent-cookiejar" to "net/http/cookiejar" and code no longer compiles.
Here is minimal example: https://play.golang.org/p/T6ix7FGFPCn

As a side note: I have both packages installed. While I was minimizing the example, sometimes my desired import was preserved (thus named cookiejar "github...."). But I was unable to see any rule for this behavior.

What did you expect to see?

I would expect no rewrite for already-working imports.

What did you see instead?

Import is changed.

[cagedmantis]

/cc @heschik

[heschi]

Yeah, this is unfortunate behavior, though I'm not sure I want to call it a bug. goimports tries to fix the imports for the file without needing to read anything from disk. To start, it ignores the persistent-cookiejar import (because it doesn't know the package name yet) and then is able to satisfy the import from the stdlib, at which point it can drop persistent-cookiejar. It should probably at least try to load the real package names in this situation. It's a little tricky because it still has to work if you refer to a nonexistant package.

The right workaround is to add the cookiejar name to the import statement.

[pkitszel]

Thanks for explanation, I use this workaround already :)

[heschi]

I thought some more about this and I don't think there's much room for improvement. At the moment, speed is a very high priority for goimports, and it puts a lot of effort into avoiding disk reads. To do that, it assumes that directory names correspond to package names, modified by some heuristics. If the directory name were cookiejar-persistent, the name would happen to agree with goimports' heuristics, and it would do the right thing. As it stands, this is a particularly unfortunate case where the directory name doesn't match, the real package name is in the standard library, and the APIs are the same because the package is a fork of the stdlib package.

The one idea I have is to require loads of package names where the heuristic is low confidence. But then if someone comes along with this package, but named persistentcookiejar (no hyphen), it's just as broken. So that's not too satisfying.

Sorry, but I think I'm going to just close this issue. I don't see a clear path forward and this is the first time I've seen this particular problem. We can absolutely revisit if it comes up again.