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: comment completion not detecting prefix #41596

Closed
muirdm opened this issue Sep 23, 2020 · 9 comments
Closed

x/tools/gopls: comment completion not detecting prefix #41596

muirdm opened this issue Sep 23, 2020 · 9 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.

Comments

@muirdm
Copy link

muirdm commented Sep 23, 2020

On master (463111b), in some cases the completion prefix within comments is not detected properly so you get too many candidates:

package main

import (
	"context"
)

func foo(_ context.Context) {
	// z
}

Completing after "z" I expect to get no completions but I get "foo".

Probably not the right place to discuss this, but even ignoring this issue I get too many completions popping up when typing in comments. I think we should consider making the matching more conservative by at least required an exact prefix match instead of fuzzy match, and possible requiring a minimum prefix match length (e.g. no comment completions unless exact prefix match at least three characters). The minimum match length requirement could be skipped when completing the first word of a comment.

/cc @heschik

@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 Sep 23, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 23, 2020
@stamblerre stamblerre removed this from the Unreleased milestone Sep 23, 2020
@stamblerre
Copy link
Contributor

/cc @dandua98

@golang golang deleted a comment from gopherbot Sep 29, 2020
@golang golang deleted a comment from gopherbot Sep 29, 2020
@dandua98
Copy link

https://go-review.googlesource.com/c/tools/+/258322 should resolve this. I always assumed c.item calls c.matcher.Score to validate an item for prefix (or fuzzy match) but I guess not. In fact, we only call c.matcher.Score in some places so wonder if there are similar issues with other completions on emacs and other clients. VSCode ignores completions not matching prefix by default so that's why I never saw this. /cc @heschik

Regarding using an exact prefix match instead of fuzzy match, technically we weren't using any match at all right now (as mentioned above) so hopefully this resolves it. We should respect user defaults unless agreed that comment completion should override and use only prefix match.

@dandua98
Copy link

... possible requiring a minimum prefix match length (e.g. no comment completions unless exact prefix match at least three characters)

I don't think this would work with current LSP spec. I believe you currently have comment suggestions enabled all the time (what LSP spec calls 24/7 completion) instead of invoked (like ctrl + space). The LSP spec recognizes both these triggers as of the kind Invoked (see https://microsoft.github.io/language-server-protocol/specification#textDocument_completion), so there's no way to distinguish between them. When manually invoked by a user (using ctrl+space), we should show all completions and not have a set matching requirement like matching 3 characters, but that also means we can't support this for always on completion. Hopefully the LSP spec would distinguish between these cases at some point.

@muirdm
Copy link
Author

muirdm commented Sep 29, 2020

When manually invoked by a user (using ctrl+space), we should show all completions and not have a set matching requirement like matching 3 characters

I don't agree. I don't see any problem with returning no completions until we are confident the user actually wants completions. With comment completions as they are, the overwhelming majority of completions are false positives since most words I'm typing are not identifiers from the code.

@dandua98
Copy link

If manually invoked, the user definitely wants completions. That's why they invoked them in comments, right?

@muirdm
Copy link
Author

muirdm commented Sep 30, 2020

If manually invoked, the user definitely wants completions.

Good point.

For me, having unwanted completions pop up as I'm composing comments greatly outweighs having false negative completions. Anyway, I can always disable it in my editor if I don't like it.

@dandua98
Copy link

Anyway, I can always disable it in my editor if I don't like it.

Was curious what you meant by this. Do you mean you can configure your editor to disable 'always on' completions in comments?

@muirdm
Copy link
Author

muirdm commented Sep 30, 2020

Do you mean you can configure your editor to disable 'always on' completions in comments?

I meant I would disable all comment completions.

@dandua98
Copy link

dandua98 commented Oct 2, 2020

Closing this since the prefix issue was resolved by https://go-review.googlesource.com/c/tools/+/258322. The other issue mentioned here depends on if microsoft/language-server-protocol#1101 is addressed at some point in the future.

@dandua98 dandua98 closed this as completed Oct 2, 2020
@golang golang locked and limited conversation to collaborators Oct 2, 2021
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.
Projects
None yet
Development

No branches or pull requests

4 participants