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

Comments

@muirdm
Copy link

@muirdm muirdm commented Sep 23, 2020

On master (463111b698783442f36fa0dfacbdff0bdd96cec3), 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 this to the Unreleased milestone Sep 23, 2020
@stamblerre stamblerre removed this from the Unreleased milestone Sep 23, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 23, 2020

/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

@dandua98 dandua98 commented Sep 29, 2020

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

@dandua98 dandua98 commented Sep 29, 2020

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

@dandua98 dandua98 commented Sep 30, 2020

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

@muirdm
Copy link
Author

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

@dandua98 dandua98 commented Sep 30, 2020

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 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 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 Oct 2, 2020
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
4 participants