x/tools/cmd/goimports: adds stdlib import conflicting with existing external import

[leighmcculloch]

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

$ go version
go version go1.16.3 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="auto"
GOARCH="amd64"
GOBIN="/home/vscode/.local/bin"
GOCACHE="/home/vscode/.cache/go-build"
GOENV="/home/vscode/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/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"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/workspaces/experimental-payment-channels/demos/demo/go.mod"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build849870043=/tmp/go-build -gno-record-gcc-switches"

What did you do?

1. Add import for github.com/libp2p/go-libp2p-gorpc which contains an rpc package, and use rpc package in code.
2. Run goimports -d .

module test

go 1.16

require (
	github.com/libp2p/go-libp2p v0.13.0
	github.com/libp2p/go-libp2p-core v0.8.0
	github.com/libp2p/go-libp2p-gorpc v0.1.3
)

package main

import (
	"context"
	"fmt"

	"github.com/libp2p/go-libp2p"
	"github.com/libp2p/go-libp2p-core/protocol"
	"github.com/libp2p/go-libp2p-gorpc"
)

func main() {
	ctx := context.Background()
	host, _ := libp2p.New(ctx)
	server := rpc.NewServer(host, protocol.ID("/p2p/rpc/x"))
	fmt.Println(server)
}

What did you expect to see?

I expected to see goimports make no changes to the code since all the imports are in use, there are no missing imports, and the code is well formatted.

What did you see instead?

Goimports adds an import for net/rpc, which causes the code not to compile because there are now two imports that import a package named rpc.

diff -u main.go.orig main.go
--- main.go.orig        2021-06-04 18:47:17.400075656 +0000
+++ main.go     2021-06-04 18:47:17.400075656 +0000
@@ -3,6 +3,7 @@
 import (
        "context"
        "fmt"
+       "net/rpc"
 
        "github.com/libp2p/go-libp2p"
        "github.com/libp2p/go-libp2p-core/protocol"

[leighmcculloch]

It's worth noting that in slightly different conditions goimports does the right thing. It seems like goimports thinks the stdlib package is more appropriate under some conditions even if an existing imported package exists. Both the net/rpc and libp2p packages have NewServer and NewClient functions. The params are different.

If I call NewServer or NewClient with two params that confuses goimports and it wants to import net/rpc even though the import is unnecessary.

If you modify the above example to call NewClient with three params, goimports does the correct behavior. See below.

package main

import (
	"context"
	"fmt"

	"github.com/libp2p/go-libp2p"
	"github.com/libp2p/go-libp2p-core/protocol"
	"github.com/libp2p/go-libp2p-gorpc"
)

func main() {
	ctx := context.Background()
	host, _ := libp2p.New(ctx)
	server := rpc.NewClient(host, protocol.ID("/p2p/rpc/x"), rpc.WithClientStatsHandler(nil))
	fmt.Println(server)
}

diff -u main.go.orig main.go
--- main.go.orig        2021-06-04 19:08:43.987034887 +0000
+++ main.go     2021-06-04 19:08:43.987034887 +0000
@@ -6,7 +6,7 @@
 
        "github.com/libp2p/go-libp2p"
        "github.com/libp2p/go-libp2p-core/protocol"
-       "github.com/libp2p/go-libp2p-gorpc"
+       rpc "github.com/libp2p/go-libp2p-gorpc"
 )
 
 func main() {

[heschi]

I feel like this is a dupe, but I couldn't find it if so.

This is roughly working as intended. goimports tries to satisfy imports without reading anything from disk, and when rpc.NewClient is the only symbol it's looking for, net/rpc is a viable candidate. If it's also looking for rpc.WithClientStatsHandler, it's not, and resorts to reading disk. At that point it discovers that github.com/libp2p/go-libp2p-gorpc is named rpc, adds the name to that import name, and is satisfied.

There's probably some tweak to the process that would fix this case, so I'll leave this open, but it's tough to deal with all the corner cases while also continuing to work on files that have typos in import paths.