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>

internal/lsp/cache/parse.go

@@ -231,21 +231,161 @@ func fix(ctx context.Context, n ast.Node, tok *token.File, src []byte) error {
 			// 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.

internal/lsp/source/completion.go

@@ -786,7 +786,7 @@ func importPath(s *ast.ImportSpec) string {
 }
 
 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 {

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)
+}

internal/lsp/testdata/summary.txt.golden

@@ -1,5 +1,5 @@
 -- summary --
-CompletionsCount = 216
+CompletionsCount = 219
 CompletionSnippetCount = 51
 UnimportedCompletionsCount = 3
 DeepCompletionsCount = 5