internal/lsp: improve completion after accidental keywords
Sometimes the prefix of the thing you want to complete is a keyword.
For example:
variance := 123
fmt.Println(var<>)
In this case the parser produces an *ast.BadExpr which breaks
completion. 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"
We also fix accidental keywords in selectors, such as:
foo.var<>
The parser produces a phantom "_" in place of the keyword, so we swap
it back for an *ast.Ident named "var".
In general, though, accidental keywords wreak havoc on the AST so we
can only do so much. There are still many cases where a keyword prefix
breaks completion. Perhaps in the future the parser can be
cursor/in-progress-edit aware and turn accidental keywords into
identifiers.
Fixes golang/go#34332.
PS I tweaked nodeContains() to include n.End() to fix a test failure
against tip related to a change to go/parser. When a syntax error is
present, an *ast.BlockStmt's End() is now set to the block's final
statement's End() (earlier than what it used to be). In order for the
cursor pos to test "inside" the block in this case I had to relax the
End() comparison.
Change-Id: Ib45952cf086cc974f1578298df3dd12829344faa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209438
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index 6dff423..3ea1d97 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -231,21 +231,161 @@
// Don't propagate this error since *ast.BadExpr is very common
// and it is only sometimes due to array types. Errors from here
// are expected and not actionable in general.
- fixArrayErr := fixArrayType(n, parent, tok, src)
- if fixArrayErr == nil {
+ if fixArrayType(n, parent, tok, src) == nil {
// Recursively fix in our fixed node.
err = fix(ctx, parent, tok, src)
+ return false
}
+
+ // Fix cases where the parser expects an expression but finds a keyword, e.g.:
+ //
+ // someFunc(var<>) // want to complete to "variance"
+ //
+ fixAccidentalKeyword(n, parent, tok, src)
+
return false
+ case *ast.DeclStmt:
+ // Fix cases where the completion prefix looks like a decl, e.g.:
+ //
+ // func typeName(obj interface{}) string {}
+ // type<> // want to call "typeName()" but looks like a "type" decl
+ //
+ fixAccidentalDecl(n, parent, tok, src)
+ return false
+ case *ast.SelectorExpr:
+ // Fix cases where a keyword prefix results in a phantom "_" selector, e.g.:
+ //
+ // foo.var<> // want to complete to "foo.variance"
+ //
+ fixPhantomSelector(n, tok, src)
+ return true
default:
ancestors = append(ancestors, n)
parent = n
return true
}
})
+
return err
}
+// fixAccidentalDecl tries to fix "accidental" declarations. For example:
+//
+// func typeOf() {}
+// type<> // want to call typeOf(), not declare a type
+//
+// If we find an *ast.DeclStmt with only a single phantom "_" spec, we
+// replace the decl statement with an expression statement containing
+// only the keyword. This allows completion to work to some degree.
+func fixAccidentalDecl(decl *ast.DeclStmt, parent ast.Node, tok *token.File, src []byte) {
+ genDecl, _ := decl.Decl.(*ast.GenDecl)
+ if genDecl == nil || len(genDecl.Specs) != 1 {
+ return
+ }
+
+ switch spec := genDecl.Specs[0].(type) {
+ case *ast.TypeSpec:
+ // If the name isn't a phantom "_" identifier inserted by the
+ // parser then the decl is likely legitimate and we shouldn't mess
+ // with it.
+ if !isPhantomUnderscore(spec.Name, tok, src) {
+ return
+ }
+ case *ast.ValueSpec:
+ if len(spec.Names) != 1 || !isPhantomUnderscore(spec.Names[0], tok, src) {
+ return
+ }
+ }
+
+ replaceNode(parent, decl, &ast.ExprStmt{
+ X: &ast.Ident{
+ Name: genDecl.Tok.String(),
+ NamePos: decl.Pos(),
+ },
+ })
+}
+
+// fixPhantomSelector tries to fix selector expressions with phantom
+// "_" selectors. In particular, we check if the selector is a
+// keyword, and if so we swap in an *ast.Ident with the keyword text. For example:
+//
+// foo.var
+//
+// yields a "_" selector instead of "var" since "var" is a keyword.
+func fixPhantomSelector(sel *ast.SelectorExpr, tok *token.File, src []byte) {
+ if !isPhantomUnderscore(sel.Sel, tok, src) {
+ return
+ }
+
+ maybeKeyword := readKeyword(sel.Sel.Pos(), tok, src)
+ if maybeKeyword == "" {
+ return
+ }
+
+ replaceNode(sel, sel.Sel, &ast.Ident{
+ Name: maybeKeyword,
+ NamePos: sel.Sel.Pos(),
+ })
+}
+
+// isPhantomUnderscore reports whether the given ident is a phantom
+// underscore. The parser sometimes inserts phantom underscores when
+// it encounters otherwise unparseable situations.
+func isPhantomUnderscore(id *ast.Ident, tok *token.File, src []byte) bool {
+ if id == nil || id.Name != "_" {
+ return false
+ }
+
+ // Phantom underscore means the underscore is not actually in the
+ // program text.
+ offset := tok.Offset(id.Pos())
+ return len(src) <= offset || src[offset] != '_'
+}
+
+// fixAccidentalKeyword tries to fix "accidental" keyword expressions. For example:
+//
+// variance := 123
+// doMath(var<>)
+//
+// If we find an *ast.BadExpr that begins with a keyword, we replace
+// the BadExpr with an *ast.Ident containing the text of the keyword.
+// This allows completion to work to some degree.
+func fixAccidentalKeyword(bad *ast.BadExpr, parent ast.Node, tok *token.File, src []byte) {
+ if !bad.Pos().IsValid() {
+ return
+ }
+
+ maybeKeyword := readKeyword(bad.Pos(), tok, src)
+ if maybeKeyword == "" {
+ return
+ }
+
+ replaceNode(parent, bad, &ast.Ident{Name: maybeKeyword, NamePos: bad.Pos()})
+}
+
+// readKeyword reads the keyword starting at pos, if any.
+func readKeyword(pos token.Pos, tok *token.File, src []byte) string {
+ var kwBytes []byte
+ for i := tok.Offset(pos); i < len(src); i++ {
+ // Use a simplified identifier check since keywords are always lowercase ASCII.
+ if src[i] < 'a' || src[i] > 'z' {
+ break
+ }
+ kwBytes = append(kwBytes, src[i])
+
+ // Stop search at arbitrarily chosen too-long-for-a-keyword length.
+ if len(kwBytes) > 15 {
+ return ""
+ }
+ }
+
+ if kw := string(kwBytes); token.Lookup(kw).IsKeyword() {
+ return kw
+ }
+
+ return ""
+}
+
// fixArrayType tries to parse an *ast.BadExpr into an *ast.ArrayType.
// go/parser often turns lone array types like "[]int" into BadExprs
// if it isn't expecting a type.
diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go
index d5bcf0f..b06c634 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -786,7 +786,7 @@
}
func nodeContains(n ast.Node, pos token.Pos) bool {
- return n != nil && n.Pos() <= pos && pos < n.End()
+ return n != nil && n.Pos() <= pos && pos <= n.End()
}
func (c *completer) inConstDecl() bool {
diff --git a/internal/lsp/testdata/keywords/accidental_keywords.go.in b/internal/lsp/testdata/keywords/accidental_keywords.go.in
new file mode 100644
index 0000000..1ae8ff3
--- /dev/null
+++ b/internal/lsp/testdata/keywords/accidental_keywords.go.in
@@ -0,0 +1,19 @@
+package keywords
+
+// non-matching candidate - shouldn't show up as completion
+var apple = "apple"
+
+func _() {
+ variance := 123 //@item(kwVariance, "variance", "int", "var")
+ println(var) //@complete(")", kwVariance)
+}
+
+func _() {
+ var s struct { variance int } //@item(kwVarianceField, "variance", "int", "field")
+ s.var //@complete(" //", kwVarianceField)
+}
+
+func _() {
+ var typeName string //@item(kwTypeName, "typeName", "string", "var")
+ type //@complete(" //", kwTypeName)
+}
diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden
index fdfde62..60d49aa 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -1,5 +1,5 @@
-- summary --
-CompletionsCount = 216
+CompletionsCount = 219
CompletionSnippetCount = 51
UnimportedCompletionsCount = 3
DeepCompletionsCount = 5
