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: provide function that gives list of candidate imports matching pattern #32749

Open
myitcv opened this issue Jun 24, 2019 · 14 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jun 24, 2019

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

$ go version
go version devel +44c9354c5a Fri Jun 21 05:21:30 2019 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20190620191750-1fa568393b23
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20190620191750-1fa568393b23

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="on"
GOARCH="amd64"
GOBIN="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin"
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build670036660=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Much like gopls offers completion candidates, it should provide a function that allows editors to tab-complete/fuzzy match/whatever packages to import.

For example, in govim we want to provide the command GOVIMAddImport such that when we type:

:GOVIMAddImport en<Tab>

(where <Tab> is us looking to complete the import), we should be able to call gopls and get a list of import candidates matching en (according to some algorithm)

Reference: govim/govim#317


cc @stamblerre @ianthehat

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 24, 2019

I think this fits into the functionality covered by #31906, or is there anything else that we should add to that issue?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 24, 2019

@stamblerre - I think they're different. In this case I'm looking to provide a command that can be run in normal mode (which is the mode in which you can issue commands to Vim).

:GOVIMAddImport en<Tab>

Reason being, not everyone does/will think that completion is the way to add an import.

But also because this will likely become the primary way of adding named imports, e.g.:

import qt "github.com/frankban/quicktest"

Would be added via:

:GOVIMAddImport github.com/frankban/quicktest=qt

or similar.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 24, 2019

What LSP request would this command map to?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 24, 2019

That's really part of the reason for me raising the issue. The short answer is: I don't know 😄

Is there not any scope for an LSP implementation providing additional methods, beyond the spec?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 27, 2019

Not that I'm aware of, though we can certainly raise this with https://github.com/microsoft/language-server-protocol. What's the justification behind users wanting to manually add imports rather than using goimports-type behavior?

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 27, 2019

As i mentioned in #32749 (comment), because this will be the primary (only?) way of adding a named import.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jun 27, 2019

My best guess for how this could work with the current state of gopls is that you automatically add an import using a quick fix, and then you rename it (I believe @suzmue intends to support that behavior, but it hasn't been added yet). govim would have to do the task of combining those 2 together.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 29, 2019

But this won't work where you already have an identifier in scope which has the name of the package being imported, which is exactly why I'd be looking to have a named import.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jun 29, 2019

Another case where this is required (just stumbled across it again, which reminded me): where you import a package for its side effects.

@suzmue

This comment has been minimized.

Copy link
Contributor

@suzmue suzmue commented Jul 1, 2019

It seems like it would require a change to the lsp spec to provide the exact function you are looking for, but there are a few ways I could see something similar potentially fitting in to gopls:

  1. autocomplete in import spec (import qt "gith<>)
  2. try to resolve imports by ignoring package name and provide the renamed package import (fix: Add import qt "github.com/pkg")
  3. Provide multiple quick fixes to choose from that would satisfy the missing import.

With finer control about what import fixes are applied, which we are working on supporting, these seem like things that would be possible.

@stamblerre stamblerre changed the title x/tools/cmd/gopls: provide function that gives list of candidate imports matching pattern x/tools/gopls: provide function that gives list of candidate imports matching pattern Jul 2, 2019
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jul 4, 2019

@suzmue - the description talks about providing a Vim command. Vim operates in different modes. Completion is possible in insert mode, at the point of the cursor. Commands operate in normal mode: the cursor position is not within document for such commands.

In any case, the intention is to call such a command when elsewhere in the code; i.e. I don't want to have to jump up to the import declarations to perform a completion on an import, only to then jump back.

Making a request to the LSP spec team sounds like a big step for something that I would think is better suited to an experiment for now. Is there a means by which we can extend the API of gopls for experimental methods such as this?

@bhcleek

This comment has been minimized.

Copy link
Contributor

@bhcleek bhcleek commented Jul 5, 2019

@myitcv vim-go provides this kind of functionality. I can help you understand how it works if needed.

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jul 5, 2019

Thanks @bhcleek - understanding how to do this is not where I'm stuck 😄. It's that I think this logic belongs in gopls, which is ultimately the source of truth for questions about syntax, types, packages, modules etc.

Of course each editor (plugin) could implement this sort of thing, but that kind of defeats the point of a language server to my mind.

As I mentioned in #32749 (comment), there are likely to be ideas/requirements like this that fall outside of the LSP spec. It makes sense to experiment with these at different levels:

  1. in editor (plugins), if possible
  2. gopls via a side-API, not part of the LSP spec
  3. proposing changes to the LSP spec

For this particular requirement, because it is not covered by the LSP spec, I'm suggesting we explore avenue 2, as a test case if you will.

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 8, 2019

We have not yet considered a plan for features outside of LSP in gopls, and I don't expect that will be a high priority for us in the near future. However, this is definitely a possibility we should consider.

Still, I'm not convinced that this feature is necessarily one that we need to provide separately from existing gopls features, since it is effectively a combination of quick fix import + rename.

In the meantime, I would definitely consider filing an issue against the LSP specification, as other languages may also be interested in this type of feature. It takes a while to make changes in the LSP spec, so definitely worth filing the issue, even if its not something they're interested in doing.

@stamblerre stamblerre added Suggested and removed Suggested labels Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.