x/tools/gopls: autocomplete keywords #34009

stamblerre · 2019-09-01T20:44:25Z

They probably should be ranked even lower than builtins.

sebsebmc · 2019-09-04T20:45:30Z

Hi, I am interested in helping with this.

stamblerre · 2019-09-10T18:58:25Z

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

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

muirdm · 2019-09-10T19:13:56Z

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 · 2019-09-10T19:18:39Z

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 · 2019-09-10T19:38:20Z

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

sebsebmc · 2019-09-12T18:08:17Z

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 · 2019-09-17T19:53:56Z

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)

muirdm · 2019-09-17T20:44:21Z

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 · 2019-09-18T15:05:05Z

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 · 2019-09-20T20:08:42Z

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

sebsebmc · 2019-10-21T20:05:03Z

Additional work is needed to add support for keywords used in declarations at the file level. Currently any non-comment text results in *ast.BadDecl and so there is no text to match against to provide completions.

The keywords that we want to complete here are: package, import, const, var, type, and func. We can special case things like suggesting a package snippet if the document doesn't yet have an *ast.Package node, or we can fix the AST like suggested in #34332 (comment)

Note: I don't believe testing package statements is possible right now because of packagetest not running tests that don't contain a valid package statement.

Another issue is that inside of switch/select statements when there is no case or default statement the the parser expects a right brace so when typing the first case/default keyword users will not get completions.

For *ast.Ident completion requests, this checks the parent node to see if the token begins a statement and then based on the path adds possible keyword completion candidates. The test lists some cases where this approach cannot provide completion candidates. The biggest thing missing is keywords for file level declarations Updates golang/go#34009 Change-Id: I9d9c0c1eb88e362613feca66d0eea6b88705b9b0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/196664 Run-TryBot: Rebecca Stambler <rstambler@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>

stamblerre · 2019-10-30T21:55:22Z

It seems that keyword completions are not being proposed in some of the expected cases. It will require a bit more investigation to see what's going on, but I just wanted to note that here.

gopherbot · 2020-02-03T05:49:45Z

Change https://golang.org/cl/217500 mentions this issue: internal/lsp/source: improve completions at file scope

Add support for var/func/const/type/import keywords at the file scope. I left out "package" because, currently, if you are completing something that means you must already have a package declaration. The main hurdle was that anything other than a decl keyword shows up as an *ast.BadDecl at the file scope. To properly detect the prefix we manually scan for the token containing the position. I also made a couple small drive-by improvements: - Also suggest "goto" and "type" keywords in functions. - Allow completing directly before a comment, e.g. "foo<>//". I needed this for a test that would have been annoying to write otherwise. Updates golang/go#34009. Change-Id: I290e7bdda9e66a16f996cdc291985a54bf375231 Reviewed-on: https://go-review.googlesource.com/c/tools/+/217500 Run-TryBot: Rebecca Stambler <rstambler@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Robert Findley <rfindley@google.com>

gopherbot · 2020-02-22T05:08:20Z

Change https://golang.org/cl/220503 mentions this issue: internal/lsp/source: complete keywords as types

gopherbot · 2020-02-22T05:31:22Z

Change https://golang.org/cl/220504 mentions this issue: internal/lsp/source: support completing "range" keyword

gopherbot · 2020-02-23T03:00:13Z

Change https://golang.org/cl/220580 mentions this issue: internal/lsp/source: suppress keyword completions in "case" statement

gopherbot · 2020-02-23T03:00:14Z

Change https://golang.org/cl/220579 mentions this issue: internal/lsp/source: fix completion in empty switch statements

Offer "struct", "interface", "map", "chan", and "func" keywords when we expect a type. For example "var foo i<>" will offer "interface". Because "struct" and "interface" are more often used when declaring named types, they get a higher score in type declarations. Otherwise, "map", "chan" and "func" get a higher score. I also got rid of the special keyword scoring. Now keywords just use stdScore and highScore. This makes the interplay with other types of candidates more predictable. Keywords are offered in pretty limited contexts, so I don't think they will be annoying. Finally, keyword candidate score is now to be scaled properly based on how well they match the prefix. Previously they weren't penalized for not matching well, so there were probably some situations where keywords were ranked too high. Updates golang/go#34009. Change-Id: I0b659c00a8503cd72da28853dfe54fcb67f734ae Reviewed-on: https://go-review.googlesource.com/c/tools/+/220503 Run-TryBot: Muir Manders <muir@mnd.rs> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>

We now offer "range" keyword in the for loop init statement: for i := r<> // offer "range" completion candidate Updates golang/go#34009. Change-Id: I2e4c1db11c37127406c78191681c39b9dd3439f7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/220504 Run-TryBot: Muir Manders <muir@mnd.rs> Reviewed-by: Rebecca Stambler <rstambler@golang.org>

Now we properly offer "case" and "default" keyword completion in cases like: switch { <> } First I had to add an AST fix for empty switch statements to move the closing brace down. For example, say the user is completing: switch { ca<> } This gets parsed as: switch { } Even if we manually recover the "ca" token, "<>" is not positioned inside the switch statement anymore, so we don't know to offer "case" and "default" candidates. To work around this, we move the closing brace down one line yielding: switch { } Second I had to add logic to manually extract the completion prefix inside empty switch statements, and finally some logic to offer (only) "case" and "default" candidates in empty switch statements. Updates golang/go#34009. Change-Id: I624f17da1c5e73faf91fe5f69e872d86f1cf5482 Reviewed-on: https://go-review.googlesource.com/c/tools/+/220579 Run-TryBot: Muir Manders <muir@mnd.rs> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>

Now we no longer offer keyword completions inside "case" statements: select { case fo<>: // don't offer "for" } Updates golang/go#34009. Change-Id: I908eda32af01fd1912fb587ce59efef1798b6741 Reviewed-on: https://go-review.googlesource.com/c/tools/+/220580 Run-TryBot: Muir Manders <muir@mnd.rs> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>

muirdm · 2020-03-11T14:41:03Z

@stamblerre do you have any other keyword TODOs or can this be closed? I'm ignoring "package" since that will probably be a can of worms.

stamblerre · 2020-03-11T16:55:45Z

One issue I have noticed is this (internal/lsp/cache/view.go:679):

gopherbot · 2020-03-12T02:44:34Z

Change https://golang.org/cl/223120 mentions this issue: internal/lsp/source: offer loop keyword completions in range stmt

stamblerre added help wanted NeedsFix The path to resolution is known, but the work has not been done. gopls Issues related to the Go language server, gopls. labels Sep 1, 2019

gopherbot added this to the Unreleased milestone Sep 1, 2019

stamblerre added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 9, 2019

stamblerre assigned muirdm Nov 7, 2019

stamblerre mentioned this issue Dec 3, 2019

code suggestion is not suggesting struct when using gopls microsoft/vscode-go#2916

Closed

stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 4, 2019

stamblerre modified the milestones: gopls v1.0, gopls/v0.3.0, gopls/v1.0.0 Jan 15, 2020

stamblerre modified the milestones: gopls/v1.0.0, gopls completion Jan 29, 2020

stamblerre removed the help wanted label Mar 11, 2020

gopherbot closed this as completed in golang/tools@11d5b4c Mar 12, 2020

golang locked and limited conversation to collaborators Mar 12, 2021

gopherbot added the FrozenDueToAge label Mar 12, 2021

rsc unassigned muirdm Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/tools/gopls: autocomplete keywords #34009

x/tools/gopls: autocomplete keywords #34009

stamblerre commented Sep 1, 2019

sebsebmc commented Sep 4, 2019

stamblerre commented Sep 10, 2019

muirdm commented Sep 10, 2019

stamblerre commented Sep 10, 2019

sebsebmc commented Sep 10, 2019

sebsebmc commented Sep 12, 2019

sebsebmc commented Sep 17, 2019

muirdm commented Sep 17, 2019

sebsebmc commented Sep 18, 2019

gopherbot commented Sep 20, 2019

sebsebmc commented Oct 21, 2019

stamblerre commented Oct 30, 2019

gopherbot commented Feb 3, 2020

gopherbot commented Feb 22, 2020

gopherbot commented Feb 22, 2020

gopherbot commented Feb 23, 2020

gopherbot commented Feb 23, 2020

muirdm commented Mar 11, 2020

stamblerre commented Mar 11, 2020

gopherbot commented Mar 12, 2020

x/tools/gopls: autocomplete keywords #34009

x/tools/gopls: autocomplete keywords #34009

Comments

stamblerre commented Sep 1, 2019

sebsebmc commented Sep 4, 2019

stamblerre commented Sep 10, 2019

muirdm commented Sep 10, 2019

stamblerre commented Sep 10, 2019

sebsebmc commented Sep 10, 2019

sebsebmc commented Sep 12, 2019

sebsebmc commented Sep 17, 2019

muirdm commented Sep 17, 2019

sebsebmc commented Sep 18, 2019

gopherbot commented Sep 20, 2019

sebsebmc commented Oct 21, 2019

stamblerre commented Oct 30, 2019

gopherbot commented Feb 3, 2020

gopherbot commented Feb 22, 2020

gopherbot commented Feb 22, 2020

gopherbot commented Feb 23, 2020

gopherbot commented Feb 23, 2020

muirdm commented Mar 11, 2020

stamblerre commented Mar 11, 2020

gopherbot commented Mar 12, 2020