x/tools/cmd/goimports: incorrectly walks GOPATH nested within the current module

[subash-a]

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

$ go version
go version go1.12beta1 darwin/amd64

Does this issue reproduce with the latest release?

Yes the issue is reproducible with the latest version of go (go1.12beta1)

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/subhash_sharma/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/subhash_sharma/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/subhash_sharma/.goversions/go1.12beta1"
GOTMPDIR=""
GOTOOLDIR="/Users/subhash_sharma/.goversions/go1.12beta1/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/subhash_sharma/subash/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 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/w0/d6l19r7j3dbg7fq56_nn4l340000gp/T/go-build083506298=/tmp/go-build -gno-record-
gcc-switches -fno-common"

What did you do?

The following testscript reproduces the issue:

# goimportsbug.txt

go mod download fruit.com@v1.0.0
go mod download
goimports -l main.go

-- go.mod --
module mod

require fruit.com v1.0.0

replace fruit.com => github.com/user/banana v1.0.0

-- main.go --
package main

import (
    "fmt"

    // missing import for core below
)

func main() {
    fmt.Println(core.Orange)
}
-- core/core.go --
package core

type T struct{}
var Orange T

# mod/fruit.com_v1.0.0.txt

-- .mod --
module fruit.com

-- .info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- go.mod --
module fruit.com

-- fruit/fruit.go --
package fruit

const Apple = "apple"
-- coretest/coretest.go --
// package coretest becomes a candidate for the missing 
// core import in main above
package coretest

const Mandarin = "mandarin"

# mod/github.com_user_banana_v1.0.0.txt

-- .mod --
module fruit.com

-- .info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- go.mod --
module fruit.com

-- fruit/fruit.go --
package fruit

const Apple = "apple"
-- coretest/coretest.go --
// package coretest becomes a candidate for the missing 
// core import in main above
package coretest

const Mandarin = "mandarin"

What did you expect to see?

Output:

main.go

because main.go is missing an import to satisfy the qualified identifier core..

What did you see instead?

go [list -e -json -compiled -test=false -export=false -deps=false -find=true -- ./core ./gopath/pkg/mod/fruit.com@v1.0.0/coretest ./gopath/pkg/mod/github.com/user/banana@v1.0.0/coretest .]: exit
status 1: go: directory gopath/pkg/mod/github.com/user/banana@v1.0.0/coretest outside available modules

Note that in this test, our GOPATH happens to be nested within the main module; GOPATH=$WORK/gopath and GOMOD=$WORK/go.mod.

goimports seems to unconditionally walk all subdirectories of the main module, including the nested GOPATH (which includes the pkg/mod subdirectory). This appears to be wrong; we should probably prune the GOPATH subdirectory tree for this phase of the walk.

Note the error triggered by this unconditional walking is not a goimports bug per se, but rather this issue

A clear workaround for this rather obscure edge case is to either not nest your GOPATH or prefix the nested GOPATH directory name with a hiding character (_ or .).

cc @heschik @bradfitz @ianthehat @myitcv

[heschi]

You could also put a .goimportsignore config file in the GOPATH root.

Thanks for the thorough bug report. I'm on the fence about fixing this. I guess I understand why, pre-modules, it might make sense to check in GOPATH to accomplish basically the same thing as /vendor. But what's the point with modules? Why not just leave GOPATH outside the repository?

[myitcv]

@subash-a and I work together so I'll chime in here too

Why not just leave GOPATH outside the repository?

Yes, totally. As @subash-a pointed out above, this is one solution, as is prefixing the name of the directory with a hiding character.

We raised this issue more out of completeness than anything. @subash-a come across this whilst tracking down #29550.

[heschi]

We raised this issue more out of completeness than anything.

Okay, thanks. I take that as meaning that this won't cause you serious trouble. Unless you think this is a common setup, I'm inclined to call this a one-off and close the issue. Comment if you disagree, of course.

If someone else is reading this because they had the same problem, please leave a note here so we can reconsider fixing it. Thanks.