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/gopls: normalize feature sets of unimported completions and organize imports #36077

Open
SteelPhase opened this issue Dec 11, 2019 · 10 comments
Labels

Comments

@SteelPhase
Copy link

@SteelPhase SteelPhase commented Dec 11, 2019

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

go version go1.13.4 darwin/amd64

Does this issue reproduce with the latest release?

Can't test on latest release, will update when brew makes go1.13.5 available

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

go env Output
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/steelphase/Library/Caches/go-build"
GOENV="/Users/steelphase/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="go.company.org"
GONOSUMDB="go.company.org"
GOOS="darwin"
GOPATH="/Users/steelphase/Code/Go"
GOPRIVATE="go.company.org"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
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/0b/zk7t542s1sj_31mwpwyk3cy81gy8z8/T/go-build062639987=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Pasted in some code that referenced a dependency that needed to be imported.

What did you expect to see?

import to be autocompleted for direct dependency go.company.org/org/lib/v4/errors

What did you see instead?

import autocompleted to transitive dependency github.com/pkg/errors

Additional Details

This is the go.mod of the module I'm working in

module go.company.org/org/thing

go 1.13

require (
	go.uber.org/zap v1.13.0
	go.company.org/org/lib/v4 v4.1.0
)

output of go mod why

steelphase@macbook:lib$go mod why github.com/pkg/errors
# github.com/pkg/errors
go.company.org/org/thing/cmd/thing
go.uber.org/zap
go.uber.org/zap.test
github.com/pkg/errors
@gopherbot gopherbot added this to the Unreleased milestone Dec 11, 2019
@gopherbot gopherbot added the Tools label Dec 11, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls label Dec 11, 2019
@SteelPhase
Copy link
Author

@SteelPhase SteelPhase commented Dec 11, 2019

@gopherbot FeatureRequest

@heschik
Copy link
Contributor

@heschik heschik commented Dec 11, 2019

Partly a followup to https://golang.org/cl/204203 -- we should prefer direct deps more than transitive. Also, maybe look into having imports on save do the same sorting.

@stamblerre stamblerre changed the title x/tools/gopls: Prefer direct dependencies when automatically adding imports x/tools/gopls: prefer direct dependencies when automatically adding imports Dec 11, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 11, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 19, 2019

Change https://golang.org/cl/212021 mentions this issue: internal/imports: consider direct mod deps more relevant

gopherbot pushed a commit to golang/tools that referenced this issue Dec 19, 2019
As a followup to CL 204203, prefer direct dependencies over indirect.
This should improve results for common names like "log" and "errors".

Updates golang/go#36077.

Change-Id: I3f8cfa070832c2035aec60c4e583ee1c0abf5085
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212021
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@heschik heschik changed the title x/tools/gopls: prefer direct dependencies when automatically adding imports x/tools/gopls: apply the sorting used by unimported completions to organize imports Dec 20, 2019
@heschik heschik changed the title x/tools/gopls: apply the sorting used by unimported completions to organize imports x/tools/gopls: normalize feature sets of unimported completions and organize imports Dec 20, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Dec 20, 2019

Retitling to reflect the remaining work.

Unimported completions are missing the "sibling imports" trick. This would be a nice feature. I'm not sure whether to reuse the implementation from the imports package or rewrite it from scratch. Sharing code would be nice, but gopls already has all the parsing done. Just pass all the sibling files to GetPackageExports?

Organize Imports is missing the sorting by relevance. This should be easy to plug in to addExternalCandidates, as long as we want it to affect goimports. Sibling imports will take precedence, but that's probably appropriate.

@heschik
Copy link
Contributor

@heschik heschik commented Dec 20, 2019

...except that addExternalCandidates goes to great lengths to return the first usable candidate it finds for speed reasons. It would be very nice to prioritize the scan in the right order, rather than sorting afterward.

@muirdm
Copy link

@muirdm muirdm commented Jul 17, 2020

It would be nice if gopls could scan the entire workspace for import aliasing patterns rather than only sibling files.

I also imagine it would be convenient if the aliased unimported packages were addressable by both the alias and the package name since the user may or may not anticipate the alias.

@atombender
Copy link

@atombender atombender commented Jul 22, 2020

Any movement on this? It's quite an annoying issue.

@heschik
Copy link
Contributor

@heschik heschik commented Jul 22, 2020

Please be specific about what you're seeing and why it's annoying.

@atombender
Copy link

@atombender atombender commented Jul 22, 2020

Sorry, I wasn't under the impression that the issue covered more than one bug. What I'm seeing is that import-organization-on-save adds imports that aren't direct dependencies (in go.mod) or even transitive deps (in go.sum).

For example, assert often pulls in gotest.tools/assert, which is in my local mod cache but nowhere else, instead of github.com/stretchr/testify/assert, which is in go.mod.

It's very annoying when writing tests, because I prefer to just type a bunch of tests and have imports be resolved on save, instead of tediously autocompleting every import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.