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: improve completion suggestions #37656

Closed
urandom opened this issue Mar 4, 2020 · 23 comments
Closed

x/tools/gopls: improve completion suggestions #37656

urandom opened this issue Mar 4, 2020 · 23 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@urandom
Copy link

urandom commented Mar 4, 2020

Hi,

This observation is from using gopls@0.3.3.

Suggestions for field values and function arguments aren't very useful.

If a completion is invoked for a field/func arg, logical suggestions such as empty-bodied functions (for func types), or map/array/struct composite literal expressions are never suggested. Instead, various variables, methods, functions, interfaces and base types that cannot possibly be used as values are suggested instead.

Some real examples:

(*chi.Mux).Use(| <- expects varargs of func(http.Handler) http.Handler

I would expect the first (or at least the top 3) suggestion to be func(http.Handler) { return nil }. Instead, the second suggestion is for a chi.Chain, which has a type of func (middlewares ...func(http.Handler) http.Handler) Middlewares. In fact, only the first suggestion has the correct type. Everything else has a completely different type, including curious suggestions such as module paths and builtin functions and types for some reason.

Another example:

(*mongo.Client).Database(| <- expects string

At the very least, I would expect an empty string as one of the top suggestions. Instead, the first suggestion is a function that returns a pointer to something, the second is also a function that accepts a pointer and returns another pointer, the third is a method, and the 4th is nil. Then I get various variables that are in scope but are not strings and I end up with the soup of builtins/module paths/interfaces and whatnot.

Returning some types or methods could be useful, because you can continue the expression to possibly get to the correct type. But they shouldn't be the first suggestions, and suggesting builtins or interfaces isn't helpful at all, because you cannot keep typing afterwards and end up with the needed type.

@gopherbot gopherbot added this to the Unreleased milestone Mar 4, 2020
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Mar 4, 2020
@gopherbot
Copy link

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.

@urandom urandom changed the title x/tools/gopls: improve suggestions x/tools/gopls: improve completion suggestions Mar 4, 2020
@stamblerre
Copy link
Contributor

Thank you for the report. Can you provide a link to the repo where you are seeing these issues? I appreciate the examples, but it is hard to understand them completely without the full context of the package.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Mar 4, 2020
@stamblerre stamblerre added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 4, 2020
@urandom
Copy link
Author

urandom commented Mar 4, 2020

Really, any piece of code will illustrate the problem. consider for example trying to pass in a func literal to something that expects an http.HandlerFunc, or this example:

	type fT struct { 
		F []string
	}

	f := fT{F: 

here, you won't get a []string{} as a completion suggestion.

But I built the original examples while working on https://github.com/globusdigital/feature-toggles (e.g.: https://github.com/globusdigital/feature-toggles/blob/master/storage/mongo.go#L85, and start typing s.client.Database( again)

@stamblerre
Copy link
Contributor

Thanks for the link. We'll take a look.

/cc @muirdm

@muirdm
Copy link

muirdm commented Mar 4, 2020

This is what I see (note the first two candidates):

Screen Shot 2020-03-04 at 11 38 58 AM

What editor are you using and what do you see as your candidates? I'm guessing the problem is that you don't have snippets enabled. We recently changed literal completions to require the client to support snippets.

For your original issue, there is another problem where literal completions don't work for variadic function args. I think we can fix that.

@muirdm
Copy link

muirdm commented Mar 4, 2020

At the very least, I would expect an empty string as one of the top suggestions.

The assumption is that typing the zero value for basic values is easy enough that you don't need a completion candidate.

@urandom
Copy link
Author

urandom commented Mar 5, 2020

The assumption is that typing the zero value for basic values is easy enough that you don't need a completion candidate.

For a string, yes. But for a struct, not so much. Unless that's also handled by these snippets you are talking about.

Screenshot 2020-03-05 at 09 07 32

These are the suggestions if the field is a struct:
Screenshot 2020-03-05 at 09 10 22

The first one is ok, but that is as far as the usefulness of these suggestions go. None of the other are even remotely useful as a suggestion at that cursor position

I'm using neovim 0.5 with the builtin lsp client here

@muirdm
Copy link

muirdm commented Mar 5, 2020

Literal completions (e.g. []string{} or some.Struct{}) require the client to have snippets supported/enabled. I would see if you can enable snippets in your client. You may have to install a separate package - I'm not too familiar with all the different Vim flavors/clients. You can ask on the #vim channel on gophers slack .

@stamblerre we may want to reconsider disabling all literal completions when the client doesn't have snippets.

@urandom
Copy link
Author

urandom commented Mar 5, 2020

@muirdm
What about all the suggested completions (except the first two) from the second screenshot? I don't see any way how one can produce something of type bT with any of those suggestions

@stamblerre
Copy link
Contributor

@stamblerre we may want to reconsider disabling all literal completions when the client doesn't have snippets.

@muirdm: I thought we had done that, or do you mean disabling the nil completion item in the first example?

What about all the suggested completions (except the first two) from the second screenshot? I don't see any way how one can produce something of type bT with any of those suggestions

This falls in the same category of literal completions, which are disabled if snippets are not supported by the client. @muirdm: Do you think we can come up with some best-effort literal completions that work without snippets?

@muirdm
Copy link

muirdm commented Mar 5, 2020

Literal completions already have non-snippet versions. We disabled them in https://go-review.googlesource.com/c/tools/+/216299, but there was some confusion in that decision (I had a critical typo in the related issue where I voted for the wrong change).

Regarding all the junk candidates that couldn't possibly be useful, I think we can start filtering some of them out. I can make a separate issue for that.

@stamblerre
Copy link
Contributor

Ah, I'm sorry I didn't realize that these were our non-snippet versions of the candidates. Maybe they could diverge further from the snippet versions like @myitcv suggested in the original bug (#36655):

What did you expect to see?

&protocol.SignatureHelpParams

What did you see instead?

&protocol.SignatureHelpParams{}

@muirdm
Copy link

muirdm commented Mar 5, 2020

Since the only issue is having to backtrack with the cursor, maybe we could do &protocol.SignatureHelpParams{. Optionally we could try to use an additional edit to place the closing curly after the cursor, as @myitcv originally suggested.

@stamblerre
Copy link
Contributor

Either works for me, though maybe we can start with just the opening brace if that's easier?

@myitcv
Copy link
Member

myitcv commented Mar 5, 2020

@muirdm

Optionally we could try to use an additional edit to place the closing curly after the cursor, as @myitcv originally suggested.

This would of course get my vote ;-)

@muirdm
Copy link

muirdm commented Mar 6, 2020

I played around with the idea of emulating simple snippets using insert text and additional edits: https://go-review.googlesource.com/c/tools/+/222198

Honestly I don't think it is worth the extra code. I think we should either leave the current behavior and encourage people to use snippets, or revert to the previous behavior where, if you don't have snippets, you get []foo{}<> inserted and have to move one character to the left if you want to put stuff in the literal. That is no more keypresses than completing to []foo or []foo{ and having to type the curlie(s) yourself.

@gopherbot
Copy link

Change https://golang.org/cl/222199 mentions this issue: internal/lsp/source: fix literal completions in variadic args

@myitcv
Copy link
Member

myitcv commented Mar 6, 2020

Thanks for putting together a CL, @muirdm.

Honestly I don't think it is worth the extra code

Just to refresh on the reasons I think this is worthwhile:

  • not all editors (read: at least Vim) have native snippet support, hence it relies on the user installing another plugin to add this support
  • modal editors like Vim make the "just move the cursor one to the left" option really painful. This option also kind of defeats the point of LSP, because it feels like we're saying "we could do this on the server but you can just work around it".

The code changes didn't look that invasive to my untrained eye?

@urandom
Copy link
Author

urandom commented Mar 6, 2020

@muirdm

tbh, i'm absolutely fine as a user if i just have to move the cursor back a character after insertion. it's an insignificant price to pay to get those suggestions back, at least until the time that neovim gets support for snippets.

@muirdm
Copy link

muirdm commented Mar 6, 2020

modal editors like Vim make the "just move the cursor one to the left" option really painful

Doesn't vim support the arrow keys these days? 😛

I'd rather the server not jump through hoops if the user can just install another package and get a better/fancier experience anyway. I know you @myitcv (and your users, and maybe other vim users) don't have that option yet, so I leave it up to @stamblerre.

@stamblerre
Copy link
Contributor

I agree with @muirdm that the extra code isn't worth it (note that the CL is https://golang.org/cl/222198, not https://golang.org/cl/222199 in #37656 (comment) above). Thank you for doing this work, though! I really appreciate the investigation. If it's possible to install a Vim plugin that supports snippets, I would suggest that as the way forward.

gopherbot pushed a commit to golang/tools that referenced this issue Mar 18, 2020
We now properly offer literal completions when completing the variadic
parameter. For example:

    func foo(...[]int) {}
    foo(<>) // now offers "[]int{}"

Updates golang/go#37656.

Change-Id: I91c47d455ae3f8051078c82a308f2b5ad4b3d4cd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222199
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@urandom
Copy link
Author

urandom commented Mar 30, 2020

@stamblerre
With this change, will gopls now suggest the literal completions even without snippet support?

@stamblerre
Copy link
Contributor

I don't believe so. I think we will stick to the approach @muirdm suggested in #37656 (comment), which is to recommend that Vim users install another package that adds snippet support.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

5 participants