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: completion breaks with keyword prefixes #34332

Closed
muirdm opened this issue Sep 16, 2019 · 8 comments
Closed

x/tools/gopls: completion breaks with keyword prefixes #34332

muirdm opened this issue Sep 16, 2019 · 8 comments
Assignees
Milestone

Comments

@muirdm
Copy link

@muirdm muirdm commented Sep 16, 2019

For example:

  variance := 123
  var<>
@gopherbot gopherbot added this to the Unreleased milestone Sep 16, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 16, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting section of the gopls Wiki page, and make sure that you have provided all of the relevant information here.

@sebsebmc

This comment has been minimized.

Copy link

@sebsebmc sebsebmc commented Sep 18, 2019

I think this is another case where the parser doesn't return an *ast.Ident and as a result the text to match against is "". It might be worthwhile to get the preceding token (work backwards until a non [a-zA-Z0-9_] character) as a fallback.

@sebsebmc

This comment has been minimized.

Copy link

@sebsebmc sebsebmc commented Oct 11, 2019

In the case of "var", "const", "type", and "import" we get a *ast.GenDecl that we can take the token out of and make an *ast.Ident to pass to setSurrounding. This at least gets us lexical completions on these keywords. If this seems like a reasonable fix I can submit this patch.

@muirdm

This comment has been minimized.

Copy link
Author

@muirdm muirdm commented Oct 17, 2019

I think the preferred approach is to fix the AST before type checking so type info is as complete as possible (see fix() in parse.go).

For accidental keywords I think there are two general cases:

  1. Keyword parses properly as in my above example. In this case it parses as an *ast.ValueSpec with a single phantom "_" name. We would fix by swapping the parent *ast.DeclStmt node with an *ast.Ident node.

  2. Keyword parses as an *ast.BadExpr. This will be much more common, e.g. someFunc(var). This is harder since the BadExpr can wreak arbitrary havoc (e.g. someFunc(var, foo) really messes things up). To fix this, I would try something like:

    1. If BadExpr starts with a keyword, alter keyword in source so it no longer is a keyword.
    2. Reparse the containing expression (maybe just reparsing entire file is easier?)
    3. Swap the value of the altered *ast.Ident back to the keyword string.

This is easier said than done, though. Probably lots of corner cases and gotchas.

muirdm added a commit to muirdm/tools that referenced this issue Nov 26, 2019
Sometimes the prefix of the thing you want to complete is a keyword.
For example:

variance := 123
fmt.Println(var<>)

The parser gives you back an *ast.BadExpr which prevents completion
form working. We now repair this BadExpr by replacing it with
an *ast.Ident named "var".

We also repair empty decls using a similar approach. This fixes cases
like:

var typeName string
type<> // want to complete to "typeName"

In general, though, we don't attempt repairs when there are no bad
statements/expressions. For example, we don't fix cases like:

selection := 123
select<>

Fixes golang/go#34332.

Change-Id: Ib45952cf086cc974f1578298df3dd12829344faa
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 30, 2019

Change https://golang.org/cl/209438 mentions this issue: internal/lsp: improve completion after accidental keywords

@atombender

This comment has been minimized.

Copy link

@atombender atombender commented Dec 5, 2019

I'm still seeing this with latest master:

gopls2

It is not consistent, but I found a way to consistently reproduce it locally. For me, I can reproduce the issue if I create a file like this:

package integration_test

func meh() {
	bad.foo()

	variance := 123
	var // <-- caret
}

For me, this triggers the problem in most cases. It appears I need a non-compiling statement in the same file for it to be triggered. (In some cases testing this, I don't get any completions at all, except var. I have no idea how to reproduce that, though.)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 5, 2019

Change https://golang.org/cl/209966 mentions this issue: internal/lsp: fix AST bookkeeping as we repair nodes

gopherbot pushed a commit to golang/tools that referenced this issue Dec 5, 2019
We weren't maintaining our ancestor node list correctly. This caused
us to fail to make AST repairs in certain cases. Now we are careful to
always append to the ancestors list when recursing.

Updates golang/go#34332.

Change-Id: I9b51ec70572170d9f592060d264c98b1f9720fb8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209966
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 3, 2020

Change https://golang.org/cl/216961 mentions this issue: internal/lsp/source: improve completion after accidental keywords

gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2020
Completion often fails when the completion prefix happens to be a
keyword. We previously tried to fix this with AST surgery, but
often the accidental keyword is not apparent looking at the AST.
For example:

    chan<>
    foo()

parses as CallExpr{Fun: ChanType{Value: Ident{"foo"}}} with very few
hints that something is wrong, and:

    default
    foo()

is completely omitted from the AST.

Rather than look in the AST, we now instead manually look for a
keyword token that contains the completion position. If we find one,
we treat that as our surrounding identifier.

Updates golang/go#34332.

Change-Id: I68ed0dd905848c0eae61f39ecb8b73adb1e72746
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216961
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
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
5 participants
You can’t perform that action at this time.