internal/lsp: fix bad *ast.ArrayTypes for completion
Currently array and slice literals don't work very well for
completion. When go/parser is not expecting a type, it often turns
array types (e.g. "[]int") into *ast.BadExpr, which messes up
completion because we can't figure out the prefix from *ast.BadExpr,
and *ast.BadExprs don't get type checked.
This change addresses the first problem of not being able to figure
out the prefix. If we see an *ast.BadExpr, we now blindly try to
reparse it as a composite literal by adding on "{}". If we end up with
an *ast.CompositeLit with an *ast.ArrayType "Type", we swap
the *ast.BadExpr for the *ast.ArrayType. This approach is dumb but
simple, and fixes lexical completions in array types.
Change-Id: Ifa42e646bcbf2a30170d73e6dd11982384d40b43
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197437
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index 37b7818..83eb48a 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -199,45 +199,101 @@
func fix(ctx context.Context, n ast.Node, tok *token.File, src []byte) error {
var (
ancestors []ast.Node
+ parent ast.Node
err error
)
ast.Inspect(n, func(n ast.Node) bool {
if n == nil {
if len(ancestors) > 0 {
ancestors = ancestors[:len(ancestors)-1]
+ if len(ancestors) > 0 {
+ parent = ancestors[len(ancestors)-1]
+ }
}
return false
}
+
switch n := n.(type) {
case *ast.BadStmt:
- var parent ast.Node
- if len(ancestors) > 0 {
- parent = ancestors[len(ancestors)-1]
- }
- err = parseDeferOrGoStmt(n, parent, tok, src) // don't shadow err
+ err = fixDeferOrGoStmt(n, parent, tok, src) // don't shadow err
if err == nil {
- // Recursively fix any BadStmts in our fixed node.
+ // Recursively fix in our fixed node.
err = fix(ctx, parent, tok, src)
} else {
err = errors.Errorf("unable to parse defer or go from *ast.BadStmt: %v", err)
}
return false
+ case *ast.BadExpr:
+ // 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 {
+ // Recursively fix in our fixed node.
+ err = fix(ctx, parent, tok, src)
+ }
+ return false
default:
ancestors = append(ancestors, n)
+ parent = n
return true
}
})
return err
}
-// parseDeferOrGoStmt tries to parse an *ast.BadStmt into a defer or a go statement.
+// 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.
+func fixArrayType(bad *ast.BadExpr, parent ast.Node, tok *token.File, src []byte) error {
+ // Our expected input is a bad expression that looks like "[]someExpr".
+
+ from := bad.Pos()
+ to := bad.End()
+
+ if !from.IsValid() || !to.IsValid() {
+ return errors.Errorf("invalid BadExpr from/to: %d/%d", from, to)
+ }
+
+ exprBytes := make([]byte, 0, int(to-from)+2)
+ // Avoid doing tok.Offset(to) since that panics if badExpr ends at EOF.
+ exprBytes = append(exprBytes, src[tok.Offset(from):tok.Offset(to-1)+1]...)
+
+ // Add "{}" to turn our ArrayType into a CompositeLit. This is to
+ // handle the case of "[...]int" where we must make it a composite
+ // literal to be parseable.
+ exprBytes = append(exprBytes, '{', '}')
+
+ expr, err := parseExpr(from, exprBytes)
+ if err != nil {
+ return err
+ }
+
+ cl, _ := expr.(*ast.CompositeLit)
+ if cl == nil {
+ return errors.Errorf("expr not compLit (%T)", expr)
+ }
+
+ at, _ := cl.Type.(*ast.ArrayType)
+ if at == nil {
+ return errors.Errorf("compLit type not array (%T)", cl.Type)
+ }
+
+ if !replaceNode(parent, bad, at) {
+ return errors.Errorf("couldn't replace array type")
+ }
+
+ return nil
+}
+
+// fixDeferOrGoStmt tries to parse an *ast.BadStmt into a defer or a go statement.
//
// go/parser packages a statement of the form "defer x." as an *ast.BadStmt because
// it does not include a call expression. This means that go/types skips type-checking
// this statement entirely, and we can't use the type information when completing.
// Here, we try to generate a fake *ast.DeferStmt or *ast.GoStmt to put into the AST,
// instead of the *ast.BadStmt.
-func parseDeferOrGoStmt(bad *ast.BadStmt, parent ast.Node, tok *token.File, src []byte) error {
+func fixDeferOrGoStmt(bad *ast.BadStmt, parent ast.Node, tok *token.File, src []byte) error {
// Check if we have a bad statement containing either a "go" or "defer".
s := &scanner.Scanner{}
s.Init(tok, src, nil, 0)
@@ -357,38 +413,11 @@
exprBytes = append(exprBytes, '_')
}
- // Wrap our expression to make it a valid Go file we can pass to ParseFile.
- fileSrc := bytes.Join([][]byte{
- []byte("package fake;func _(){"),
- exprBytes,
- []byte("}"),
- }, nil)
-
- // Use ParseFile instead of ParseExpr because ParseFile has
- // best-effort behavior, whereas ParseExpr fails hard on any error.
- fakeFile, err := parser.ParseFile(token.NewFileSet(), "", fileSrc, 0)
- if fakeFile == nil {
- return errors.Errorf("error reading fake file source: %v", err)
+ expr, err := parseExpr(from, exprBytes)
+ if err != nil {
+ return err
}
- // Extract our expression node from inside the fake file.
- if len(fakeFile.Decls) == 0 {
- return errors.Errorf("error parsing fake file: %v", err)
- }
- fakeDecl, _ := fakeFile.Decls[0].(*ast.FuncDecl)
- if fakeDecl == nil || len(fakeDecl.Body.List) == 0 {
- return errors.Errorf("no statement in %s: %v", exprBytes, err)
- }
- exprStmt, ok := fakeDecl.Body.List[0].(*ast.ExprStmt)
- if !ok {
- return errors.Errorf("no expr in %s: %v", exprBytes, err)
- }
- expr := exprStmt.X
-
- // parser.ParseExpr returns undefined positions.
- // Adjust them for the current file.
- offsetPositions(expr, from-1-(expr.Pos()-1))
-
// Package the expression into a fake *ast.CallExpr and re-insert
// into the function.
call := &ast.CallExpr{
@@ -396,6 +425,7 @@
Lparen: to,
Rparen: to,
}
+
switch stmt := stmt.(type) {
case *ast.DeferStmt:
stmt.Call = call
@@ -404,12 +434,53 @@
}
if !replaceNode(parent, bad, stmt) {
- return errors.Errorf("couldn't replace %T in %T", stmt, parent)
+ return errors.Errorf("couldn't replace CallExpr")
}
return nil
}
+// parseExpr parses the expression in src and updates its position to
+// start at pos.
+func parseExpr(pos token.Pos, src []byte) (ast.Expr, error) {
+ // Wrap our expression to make it a valid Go file we can pass to ParseFile.
+ fileSrc := bytes.Join([][]byte{
+ []byte("package fake;func _(){"),
+ src,
+ []byte("}"),
+ }, nil)
+
+ // Use ParseFile instead of ParseExpr because ParseFile has
+ // best-effort behavior, whereas ParseExpr fails hard on any error.
+ fakeFile, err := parser.ParseFile(token.NewFileSet(), "", fileSrc, 0)
+ if fakeFile == nil {
+ return nil, errors.Errorf("error reading fake file source: %v", err)
+ }
+
+ // Extract our expression node from inside the fake file.
+ if len(fakeFile.Decls) == 0 {
+ return nil, errors.Errorf("error parsing fake file: %v", err)
+ }
+
+ fakeDecl, _ := fakeFile.Decls[0].(*ast.FuncDecl)
+ if fakeDecl == nil || len(fakeDecl.Body.List) == 0 {
+ return nil, errors.Errorf("no statement in %s: %v", src, err)
+ }
+
+ exprStmt, ok := fakeDecl.Body.List[0].(*ast.ExprStmt)
+ if !ok {
+ return nil, errors.Errorf("no expr in %s: %v", src, err)
+ }
+
+ expr := exprStmt.X
+
+ // parser.ParseExpr returns undefined positions.
+ // Adjust them for the current file.
+ offsetPositions(expr, pos-1-(expr.Pos()-1))
+
+ return expr, nil
+}
+
var tokenPosType = reflect.TypeOf(token.NoPos)
// offsetPositions applies an offset to the positions in an ast.Node.
@@ -433,7 +504,7 @@
continue
}
- f.SetInt(int64(f.Interface().(token.Pos) + offset))
+ f.SetInt(f.Int() + int64(offset))
}
}
diff --git a/internal/lsp/testdata/arraytype/array_type.go.in b/internal/lsp/testdata/arraytype/array_type.go.in
new file mode 100644
index 0000000..2f1dbaf
--- /dev/null
+++ b/internal/lsp/testdata/arraytype/array_type.go.in
@@ -0,0 +1,25 @@
+package arraytype
+
+import (
+ "golang.org/x/tools/internal/lsp/foo"
+)
+
+func _() {
+ var (
+ val string //@item(atVal, "val", "string", "var")
+ )
+
+ [] //@complete(" //", atVal, PackageFoo)
+
+ []val //@complete(" //", atVal)
+
+ []foo.StructFoo //@complete(" //", StructFoo)
+
+ []*foo.StructFoo //@complete(" //", StructFoo)
+
+ [...]foo.StructFoo //@complete(" //", StructFoo)
+
+ [2][][4]foo.StructFoo //@complete(" //", StructFoo)
+
+ []struct { f []foo.StructFoo } //@complete(" }", StructFoo)
+}
diff --git a/internal/lsp/testdata/foo/foo.go b/internal/lsp/testdata/foo/foo.go
index 444f07f..277ac53 100644
--- a/internal/lsp/testdata/foo/foo.go
+++ b/internal/lsp/testdata/foo/foo.go
@@ -1,4 +1,4 @@
-package foo //@mark(PackageFoo, "foo")
+package foo //@mark(PackageFoo, "foo"),item(PackageFoo, "foo", "\"golang.org/x/tools/internal/lsp/foo\"", "package")
type StructFoo struct { //@item(StructFoo, "StructFoo", "struct{...}", "struct")
Value int //@item(Value, "Value", "int", "field")
diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden
index 042115d..75f2e36 100644
--- a/internal/lsp/testdata/summary.txt.golden
+++ b/internal/lsp/testdata/summary.txt.golden
@@ -1,5 +1,5 @@
-- summary --
-CompletionsCount = 178
+CompletionsCount = 185
CompletionSnippetCount = 39
UnimportedCompletionsCount = 1
DeepCompletionsCount = 5
