x/tools/cmd/goimports: do not prefix packages from GOROOT if it is inside a module

[misha-ridge]

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

go version go1.12beta2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

Details

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/dottedmag/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/dottedmag/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/dottedmag/tectonic/_deps/go-1.12beta2"
GOTMPDIR=""
GOTOOLDIR="/Users/dottedmag/tectonic/_deps/go-1.12beta2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bw/6hyp_vyj68v973qbhlbcvlcr0000gn/T/go-build721712428=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a GOROOT inside a module (a per-repository Go installation for a hermetic build).

I run goimports, it tries to resolve a package (say, reflect), it loads packages from GOROOT, and they end up being prefixed with a path inside the current module.

What did you expect to see?

package mypkg

import "reflect"

type x reflect.Type

What did you see instead?

package mypkg

import "my.module/_deps/go-1.12beta2/src/reflect"

type x reflect.Type

How to fix

[Patch redacted by @bcmills. Please send a PR or CL so that we can verify CLA compliance.]

[heschi]

Yep, that's a bug. We can't accept patches in issues, but a PR or CL would be welcome. See https://golang.org/doc/contribute.html. (A test would be good too.)
@ianthehat

[rebrendov]

I would like to tackle this bug, if noone is working on it yet.

[heschi]

Absolutely.

I think the ideal place for a fix is internal/gopathwalk/walk.go; somewhere in there it should skip over $GOROOT while walking a root of type RootGOPATH. The current API, with just a flat list of Roots, may make that awkward. It might make sense to pass GOROOT explicitly to Walk.

[misha-ridge]

I haven't seen this bug for a while. Obsolete?