x/tools/gopls: autocomplete in import blocks

[stamblerre]

Suggested in microsoft/vscode-go#2918.

[muirdm]

Is there a use case for this once unimported completions are enabled by default? The only case I can think of is manually correcting an (incorrect) ambiguous import, but even that should be pretty rare.

[heschi]

I agree the justification for this is weak. Maybe if you're exploring an import hierarchy, like you can't remember the exact package name you're looking for but you know what repo it was in?

[stamblerre]

@heschik: That's what I was thinking. Definitely not pressing, but I thought it wasn't a bad idea.

[nbaztec]

I can try to take a look into this one, if someone can point me in the right direction so as where to take a look. Thanks!

We too have long import names, of which sometimes only a part is valid, eg: github.com/foo/bar/my-super-valid-name/v1/lib/http and an autocompletion would certainly be of help

[muirdm]

Another not-mutually-exclusive option is to make unimported package name completion match against the full package path instead of just the package name (i.e. completing package names in the code, not in import specs). Using your example path, the user could type supervalid<> to see unimported packages whose path fuzzily matches "supervalid", or supervalidhttp<> to complete straight to the desired package. This way the user can explore things without resorting to manually futzing with the import spec.

[stamblerre]

Thanks, @nbaztec! The code for completion can be found here, but I think you'd be specifically interested in unimported completions, which are done here.

[nbaztec]

Thanks a bunch @stamblerre !
I tried to look into some docs on how to best debug/test gopls and right now the only way I'm aware of is to compile a copy and try it out in vs-code. Is there any way I can perhaps use a debugger/delve while testing? That would help me a ton!

[nbaztec]

Ok I tried asking some questions and figured out some code, however would really appreciate some help in how can I debug the application, via tests. Documentation seems to be missing on how to write my expectations for the following case:

package foo

import "fm{}"  // trigger suggestions

Any direction is highly appreciated, thanks!

[stamblerre]

@nbaztec: I saw that you got some guidance on Slack - were you able to figure out the issue?

[nbaztec]

Yes, I happened to figure out the above. Also, is a completion snippet the only way to replace the text on the current line?
Eg: replace import "fm_" directly by fmt?

[stamblerre]

Completion items have a TextEdit field that indicates what should be filled in for a completion item (see here for more details).

[gopherbot]

Change https://golang.org/cl/249419 mentions this issue: internal/imports: support completing import paths

[stamblerre]

https://golang.org/cl/250301 implements this feature, but I think there is still some work left here.

In my opinion, the biggest blocker is the fact that gopls treats changes in imports as metadata invalidations, so gopls needs to reload all of the invalidated packages in the workspace before the completion can be triggered. One quick option might be not treating import paths ending in a / as metadata invalidations, since we know they are incomplete. I will look into that.

A few remaining other issues:

• The cursor should be placed within the quotes if the path is incomplete (using snippets).
• The completion label should be just the next element of the path - otherwise the label is too long and is cut off:

/cc @dandua98

[muirdm]

gopls treats changes in imports as metadata invalidations,

Maybe we only need to invalidate metadata if we lost or gained a valid package path (?). In other words, if before and after the change the file has the same 5 valid import paths and 1 different invalid import path, we don't need to invalidate the metadata.

Adding my last note from CL here since CL is no long active:

I was playing around with this on a big repo and when I did "github.com/<>" I could sometimes get it to list the candidates, but when I tried to type to narrow the candidates (e.g. "github.com/s<>"), or do "github.com/somecandidate/<>" I never got any completions. I didn't work reliably for packages within my module, either. I assume the issue is there are too many packages and it is timing out but I didn't investigate.

I had similar results with "golang.org". For example I could get "golang.org/x/<>" to list candidates sometimes, but after typing "s", "golang.org/x/s<>" did not list anything. If I deleted and retyped the "s" and completed again, it would list things.

[stamblerre]

Maybe we only need to invalidate metadata if we lost or gained a valid package path (?).

The issue is that we don't really know if the package path was valid until after we've loaded it. But I think in the case of the terminal / we can be certain, so that might at least be a starting point.

I'm guessing that the latency you're seeing is that package loads - I was seeing something similar. I basically had to leave the completion request active until gopls had "caught up" and then it could work.

[stamblerre]

#36918 is also relevant here. Our package metadata reloading model is generally an obstacle to manually typed imports. I have a small CL that will help when a user types with only one starting quote, but most editors insert the closing quote automatically. @heschik may have some other ideas here.

@muirdm: You can see the actual speed of this feature by triggering completions at in an import path that has already been typed out.

[gopherbot]

Change https://golang.org/cl/251086 mentions this issue: internal/lsp/cache: don't invalidate metadata for new invalid imports

[muirdm]

we don't really know if the package path was valid until after we've loaded it

Ah, okay.

Maybe it is worth it to do a simple/fast go list -find <pkg> call on the modified import to first see if it exists? Additionally, if it is already a "known" package path we could skip the additional "go list -find" call and proceed with the full metadata refresh.

[stamblerre]

it is already a "known" package path we could skip the additional "go list -find" call and proceed with the full metadata refresh.

Oh that's a great suggestion - let me try it out. We can also detect if the change is on-disk or not, so we can avoid a ton of go list calls when a user changes branches.

[stamblerre]

Ah, actually unfortunately, I don't think we'll be able to do that. I completely forgot that go list might download things, so any invalid package path will take go list a long time, and we can't afford to do that while a user types.

[stamblerre]

We may be able to use known packages to see if the user is typing a package path within the main module, but that may be error prone. Something to consider in a follow-up.

[stamblerre]

@heschik has suggested that we might try loading with GOPROXY=off by default so that the package.Loads are not prohibitively slow. I think a lot of our logic will have to change if #40728 is accepted, so I'd say that it's not worth making any changes until we know what the behavior of go list will be like in 1.16. Then, we make any necessary changes in a cohesive way.

[heschi]

For the record, I think the go mod tidy code lens is also getting involved.

[dandua98]

@stamblerre should we close this issue and open new issues for import label and cursor placement? I think it would make it easy to track those bugs.

[gopherbot]

Change https://golang.org/cl/255837 mentions this issue: internal/lsp/source/completion: improve import suggestion labels