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 keywords #34009

Open
stamblerre opened this issue Sep 1, 2019 · 10 comments

Comments

@stamblerre
Copy link
Contributor

commented Sep 1, 2019

They probably should be ranked even lower than builtins.

@gopherbot gopherbot added this to the Unreleased milestone Sep 1, 2019

@sebsebmc

This comment has been minimized.

Copy link

commented Sep 4, 2019

Hi, I am interested in helping with this.

@stamblerre stamblerre added the Tools label Sep 9, 2019

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@sebsebmc: Awesome! A good place to start looking is https://github.com/golang/tools/blob/master/internal/lsp/source/completion.go. Feel free to reach out to me here or on the Gophers Slack if you need help.

Some things to consider:

  1. How should keywords be ranked? My thinking is that they should probably be prioritized lower than lexical completions, but higher than deep completions (what do you think, @muirrn?).

  2. We should only suggest keywords if they are reasonable in the context of the code. For example, we shouldn't suggest continue outside of a loop.

@muirrn

This comment has been minimized.

Copy link

commented Sep 10, 2019

I think if we do a good job with 2), we can probably just start with the "stdScore" (assuming they are naturally ordered after lexical completions). My guess is typing "cont<>" and getting some weird fuzzy match instead of "continue" will be a more common annoyance than typing "cont<>" and getting "continue" instead of a lexical candidate.

One other thought: most keywords are statements, so we should also limit those to beginning of statements. For example:

for {
  breakDance := 123
  actuate(brea<>) // don't offer "break" completion because it isn't start of statement
}
@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I actually just ran into the continue case a few minutes ago (VSCode offers keyword completions on its own, so they don't work at all with our rankings), but that's exactly why I mentioned the placement between standard and deep completions. I agree, there are enough cases where we can exclude keywords that the ranking system should be fine.

@sebsebmc

This comment has been minimized.

Copy link

commented Sep 10, 2019

Hi, I've been looking at completions.go already, I'll be sure to reach out with any questions.

@sebsebmc

This comment has been minimized.

Copy link

commented Sep 12, 2019

Would it be alright to submit my in-progress changes and then mark it as work-in-progress (is that the purpose of DNR) somehow?

@sebsebmc

This comment has been minimized.

Copy link

commented Sep 17, 2019

So currently I have reduced the keywords into roughly 2 categories: those that the parser thinks might be an identifier, and those that it doesn't. I've been focusing on the *ast.Ident case and trying to return keywords based on the context and erring on the side of not recommending a keyword.

Currently, the scoring is very rudimentary and there still needs to be some more work on restricting the keywords to beginnings of statements. This is partially being done right now by bailing on keyword suggestion if the path contains certain nodes like: FieldSet, ValueSpec, TypeSwitchExpr... These node types mean we are inside a statement where a keyword is unlikely to be valid.

For nodes that aren't being marked as an identifier, the parser is typically marking them as a *ast.BadDecl because it only wants valid declaration keywords at the file level scope. A partially typed keyword here is invalid and the token needs to be retrieved from the file to be able to prefix match on it.

I plan on submitting a patch for the first case once I have it flushed out a bit more and then based on that we can think about how to handle the second case. (Or if there's a different approach altogether that would be better)

@muirrn

This comment has been minimized.

Copy link

commented Sep 17, 2019

there still needs to be some more work on restricting the keywords to beginnings of statements.

Not tested, but to detect if we are at the start of a statement I would start by checking if path[0] or path[1] is an *ast.BlockStmt. Maybe you already tried something like that.

the parser is typically marking them as a *ast.BadDecl because it only wants valid declaration keywords at the file level scope

I assume you are talking about cases like this:

package foo
co<> // want to complete to "const".

I had a failed CL that might have helped with this case. The CL would replace the leaf BadDecl in path with an *ast.Ident to facilitate completion. If that helps you then maybe there is a case to reopen it. On the other hand, maybe not offering completions for file level keywords is fine.

@sebsebmc

This comment has been minimized.

Copy link

commented Sep 18, 2019

I'm not sure that the BadDecl needs to be replaced, it would likely be sufficient to be able to get the co in your example and match that against the keywords used in declarations (like var, const, etc.)

checking if path[0] or path[1] is an *ast.BlockStmt

The problem are nodes like CaseClause that don't have BlockStmt as bodies.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 20, 2019

Change https://golang.org/cl/196664 mentions this issue: internal/lsp: add some keyword completions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.