Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/cmd/goimports: prefer main module requirements #31030

Open
myitcv opened this issue Mar 25, 2019 · 1 comment

Comments

@myitcv
Copy link
Member

commented Mar 25, 2019

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

$ go version
go version go1.12.1 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
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPROXY=""
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
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-build898038110=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran testscript on the following:

# install goimports
env GOMODPROXY=$GOPROXY
env GOPROXY=
go install golang.org/x/tools/cmd/goimports

# switch back to our local proxy
env GOPROXY=$GOMODPROXY

# "warm" the module (download) cache
go get gopkg.in/tomb.v1
go get gopkg.in/tomb.v2

# test goimports
cd mod
go mod tidy
exec goimports main.go
stdout '\Q"gopkg.in/tomb.v2"\E'

-- go.mod --
module goimports

require golang.org/x/tools v0.0.0-20190322203728-c1a832b0ad89

-- mod/go.mod --
module mod

require gopkg.in/tomb.v2 v2.0.0

-- mod/main.go --
package main

import (
	"fmt"
)

func main() {
	fmt.Println(tomb.Tomb)
}

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/.mod --
module gopkg.in/tomb.v1

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/.info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/go.mod --
module gopkg.in/tomb.v1

-- .gomodproxy/gopkg.in_tomb.v1_v1.0.0/tomb.go --
package tomb

const Tomb = "A great package v1"

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/.mod --
module gopkg.in/tomb.v2

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/.info --
{"Version":"v2.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/go.mod --
module gopkg.in/tomb.v2

-- .gomodproxy/gopkg.in_tomb.v2_v2.0.0/tomb.go --
package tomb

const Tomb = "A great package v2"

What did you expect to see?

A passing run.

What did you see instead?

A failed run.

goimports should be "consulting" the main module for matches before dropping down to a module cache-based search. Here that would have resulted in gopkg.in/tomb.v2 being correctly resolved, instead of gopkg.in/tomb.v1.

This will, I suspect, also massively improve the speed of goimports in a large majority of cases.

cc @heschik

@gopherbot gopherbot added this to the Unreleased milestone Mar 25, 2019

@heschik

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

@ianthehat

This would be fairly easy to do as a separate pass. I don't think it would make a huge performance difference for most people, but maybe if you have a gigantic module cache or are looking for a very generic package like "util" or something. For me, the stronger argument is just that we should be biased against adding new deps when an existing dep suffices.

@gopherbot gopherbot added the Tools label Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.