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: autocomplete in import blocks #35877

Closed
stamblerre opened this issue Nov 27, 2019 · 25 comments
Closed

x/tools/gopls: autocomplete in import blocks #35877

stamblerre opened this issue Nov 27, 2019 · 25 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 27, 2019

Suggested in microsoft/vscode-go#2918.

@muirdm
Copy link

@muirdm muirdm commented Nov 27, 2019

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.

@heschik
Copy link
Contributor

@heschik heschik commented Nov 27, 2019

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
Copy link
Contributor Author

@stamblerre stamblerre commented Nov 27, 2019

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

@nbaztec
Copy link

@nbaztec nbaztec commented Dec 20, 2019

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
Copy link

@muirdm muirdm commented Dec 20, 2019

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
Copy link
Contributor Author

@stamblerre stamblerre commented Dec 20, 2019

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
Copy link

@nbaztec nbaztec commented Dec 21, 2019

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
Copy link

@nbaztec nbaztec commented Dec 22, 2019

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
Copy link
Contributor Author

@stamblerre stamblerre commented Dec 23, 2019

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

@nbaztec
Copy link

@nbaztec nbaztec commented Dec 27, 2019

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
Copy link
Contributor Author

@stamblerre stamblerre commented Dec 27, 2019

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

@stamblerre stamblerre modified the milestones: gopls unplanned, gopls completion Jan 29, 2020
@stamblerre stamblerre removed this from the gopls/v0.5.0 milestone Jun 24, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 19, 2020

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

gopherbot pushed a commit to golang/tools that referenced this issue Aug 20, 2020
Add a new autocomplete function that completes based on import path
prefix rather than package name prefix.

Updates golang/go#35877.

Change-Id: Ib768080ee99debfff1c8c870d22dc7b7459deadd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/249419
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Danish Dua <danishdua@google.com>
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 27, 2020

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:

Screen Shot 2020-08-27 at 2 20 51 PM

/cc @dandua98

@muirdm
Copy link

@muirdm muirdm commented Aug 27, 2020

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
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 27, 2020

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
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 27, 2020

#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
Copy link

@gopherbot gopherbot commented Aug 27, 2020

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

@muirdm
Copy link

@muirdm muirdm commented Aug 27, 2020

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
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 27, 2020

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
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 27, 2020

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
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 27, 2020

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.

gopherbot pushed a commit to golang/tools that referenced this issue Aug 28, 2020
Our metadata reloading model makes typing out import paths manually
very slow. We can avoid some of the slowness by not invalidating
metadata when a new import path is obviously invalid.

Updates golang/go#35877

Change-Id: Ifcf9ebaac0b146a2098ef8d411fa85fefa7ba6ca
Reviewed-on: https://go-review.googlesource.com/c/tools/+/251086
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Danish Dua <danishdua@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 31, 2020

@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.

@heschik
Copy link
Contributor

@heschik heschik commented Sep 1, 2020

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

@dandua98
Copy link
Member

@dandua98 dandua98 commented Sep 12, 2020

@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.

@dandua98 dandua98 self-assigned this Sep 12, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 17, 2020

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

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.