From 49ba83114ef5af9f95e7efcd8a1892acf0e68069 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Tue, 23 Apr 2019 22:00:40 +0000 Subject: [PATCH] internal/lsp: improve composite literal completion When the value of a composite literal key/value pair was unparsable, you were getting completions for the composite literal keys instead of values. For example "struct { foo int }{foo: []<>" was completing to the field name "foo". This was because the leaf ast.Node at the cursor was the composite literal itself, and our go-back-one-character logic was not happening because the preceding character's node was *ast.BadExpr, not *ast.Ident. Fix by always generating the ast path for the character before the cursor's position. I couldn't find any cases where this broke completion. I also added expected type detection for the following composite literal cases: - array/slice literals - struct literals (both implicit and explicit field names) - map keys and values Fixes golang/go#29153 Change-Id: If8cf678cbd743a970f52893fcf4a9b83ea06d7e9 GitHub-Last-Rev: f385705cc05eb98132e20561451dbb8c39b68519 GitHub-Pull-Request: golang/tools#86 Reviewed-on: https://go-review.googlesource.com/c/tools/+/173099 Run-TryBot: Rebecca Stambler Reviewed-by: Rebecca Stambler --- internal/lsp/lsp_test.go | 20 ++- internal/lsp/source/completion.go | 124 ++++++++++++++++-- internal/lsp/testdata/complit/complit.go.in | 34 ++++- .../nested_complit/nested_complit.go.in | 13 ++ internal/lsp/testdata/signature/signature.go | 11 +- internal/lsp/tests/tests.go | 2 +- 6 files changed, 181 insertions(+), 23 deletions(-) create mode 100644 internal/lsp/testdata/nested_complit/nested_complit.go.in diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index f45290de..2f6d8f1f 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -162,9 +162,27 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, items tests.Co t.Fatalf("completion failed for %v: %v", src, err) } if diff := diffCompletionItems(t, src, want, got); diff != "" { - t.Errorf(diff) + t.Errorf("%s: %s", src, diff) } } + + // Make sure we don't crash completing the first position in file set. + firstFile := r.data.Config.Fset.Position(1).Filename + + _, err := r.server.Completion(context.Background(), &protocol.CompletionParams{ + TextDocumentPositionParams: protocol.TextDocumentPositionParams{ + TextDocument: protocol.TextDocumentIdentifier{ + URI: protocol.NewURI(span.FileURI(firstFile)), + }, + Position: protocol.Position{ + Line: 0, + Character: 0, + }, + }, + }) + if err != nil { + t.Fatal(err) + } } func isBuiltin(item protocol.CompletionItem) bool { diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 38f1df76..3f70f850 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -52,23 +52,15 @@ func Completion(ctx context.Context, f File, pos token.Pos) (items []CompletionI if pkg.IsIllTyped() { return nil, "", fmt.Errorf("package for %s is ill typed", f.URI()) } - path, _ := astutil.PathEnclosingInterval(file, pos, pos) + + // Completion is based on what precedes the cursor. + // To understand what we are completing, find the path to the + // position before pos. + path, _ := astutil.PathEnclosingInterval(file, pos-1, pos-1) if path == nil { return nil, "", fmt.Errorf("cannot find node enclosing position") } - // If the position is not an identifier but immediately follows - // an identifier or selector period (as is common when - // requesting a completion), use the path to the preceding node. - if _, ok := path[0].(*ast.Ident); !ok { - if p, _ := astutil.PathEnclosingInterval(file, pos-1, pos-1); p != nil { - switch p[0].(type) { - case *ast.Ident, *ast.SelectorExpr: - path = p // use preceding ident/selector - } - } - } - // Skip completion inside comment blocks or string literals. switch lit := path[0].(type) { case *ast.File, *ast.BlockStmt: @@ -411,6 +403,43 @@ func complit(path []ast.Node, pos token.Pos, pkg *types.Package, info *types.Inf return items, prefix, false } +// enclosingCompLit returns the composite literal and key value expression, if +// any, enclosing the given position. +func enclosingCompLit(pos token.Pos, path []ast.Node) (*ast.CompositeLit, *ast.KeyValueExpr) { + var keyVal *ast.KeyValueExpr + + for _, n := range path { + switch n := n.(type) { + case *ast.CompositeLit: + // pos isn't part of the composite literal unless it falls within the curly + // braces (e.g. "foo.Foo<>Struct{}"). + if n.Lbrace <= pos && pos <= n.Rbrace { + if keyVal == nil { + if i := exprAtPos(pos, n.Elts); i < len(n.Elts) { + keyVal, _ = n.Elts[i].(*ast.KeyValueExpr) + } + } + + return n, keyVal + } + + return nil, nil + case *ast.KeyValueExpr: + keyVal = n + case *ast.FuncType, *ast.CallExpr, *ast.TypeAssertExpr: + // These node types break the type link between the leaf node and + // the composite literal. The type of the leaf node becomes unrelated + // to the type of the composite literal, so we return nil to avoid + // inappropriate completions. For example, "Foo{Bar: x.Baz(<>)}" + // should complete as a function argument to Baz, not part of the Foo + // composite literal. + return nil, nil + } + } + + return nil, nil +} + // formatCompletion creates a completion item for a given types.Object. func formatCompletion(obj types.Object, qualifier types.Qualifier, score float64, isParam func(*types.Var) bool) CompletionItem { label := obj.Name() @@ -578,8 +607,77 @@ func enclosingFunction(path []ast.Node, pos token.Pos, info *types.Info) *types. return nil } +func expectedCompLitType(cl *ast.CompositeLit, kv *ast.KeyValueExpr, pos token.Pos, info *types.Info) types.Type { + // Get the type of the *ast.CompositeLit we belong to. + clType, ok := info.Types[cl] + if !ok { + return nil + } + + switch t := clType.Type.Underlying().(type) { + case *types.Slice: + return t.Elem() + case *types.Array: + return t.Elem() + case *types.Map: + // If pos isn't in a key/value expression or it is on the left side + // of a key/value colon, a key must be entered next. + if kv == nil || pos <= kv.Colon { + return t.Key() + } + + return t.Elem() + case *types.Struct: + // pos is in a key/value expression + if kv != nil { + // If pos is to left of the colon, it is a struct field name, + // so there is no expected type. + if pos <= kv.Colon { + return nil + } + + if keyIdent, ok := kv.Key.(*ast.Ident); ok { + // Find the type of the struct field whose name matches the key. + for i := 0; i < t.NumFields(); i++ { + if field := t.Field(i); field.Name() == keyIdent.Name { + return field.Type() + } + } + } + + return nil + } + + hasKeys := false // true if the composite literal has any key/value pairs + for _, el := range cl.Elts { + if _, ok := el.(*ast.KeyValueExpr); ok { + hasKeys = true + break + } + } + + // The struct literal is using field names, but pos is not in a key/value + // pair. A field name must be entered next, so there is no expected type. + if hasKeys { + return nil + } + + // The order of the literal fields must match the order in the struct definition. + // Find the element pos falls in and use the corresponding field's type. + if i := exprAtPos(pos, cl.Elts); i < t.NumFields() { + return t.Field(i).Type() + } + } + + return nil +} + // expectedType returns the expected type for an expression at the query position. func expectedType(path []ast.Node, pos token.Pos, info *types.Info) types.Type { + if compLit, keyVal := enclosingCompLit(pos, path); compLit != nil { + return expectedCompLitType(compLit, keyVal, pos, info) + } + for i, node := range path { if i == 2 { break diff --git a/internal/lsp/testdata/complit/complit.go.in b/internal/lsp/testdata/complit/complit.go.in index 9c29c52c..fec5aadd 100644 --- a/internal/lsp/testdata/complit/complit.go.in +++ b/internal/lsp/testdata/complit/complit.go.in @@ -5,19 +5,47 @@ type position struct { //@item(structPosition, "position", "struct{...}", "struc } func _() { - _ := position{ + _ = position{ //@complete("", fieldX, fieldY, structPosition) } - _ := position{ + _ = position{ X: 1, //@complete("", fieldY) } - _ := position{ + _ = position{ //@complete("", fieldX) Y: 1, } } +func _() { + var ( + aa string //@item(aaVar, "aa", "string", "var") + ab int //@item(abVar, "ab", "int", "var") + ) + + _ = map[int]int{ + a: a, //@complete(":", abVar, aaVar),complete(",", abVar, aaVar) + } + + _ = map[int]int{ + //@complete("", abVar, aaVar, structPosition) + } + + _ = position{X: a} //@complete("}", abVar, aaVar) + _ = position{a} //@complete("}", abVar, aaVar) + + _ = []int{a} //@complete("a", abVar, aaVar, structPosition) + _ = [1]int{a} //@complete("a", abVar, aaVar, structPosition) + + var s struct { + AA int //@item(fieldAA, "AA", "int", "field") + AB string //@item(fieldAB, "AB", "string", "field") + } + _ = map[int]string{1: "" + s.A} //@complete("}", fieldAB, fieldAA) + _ = map[int]string{1: (func(i int) string { return "" })(s.A)} //@complete(")}", fieldAA, fieldAB) +} + func _() { _ := position{ X: 1, //@complete("X", fieldX),complete(" 1", structPosition) diff --git a/internal/lsp/testdata/nested_complit/nested_complit.go.in b/internal/lsp/testdata/nested_complit/nested_complit.go.in new file mode 100644 index 00000000..3c3c88f0 --- /dev/null +++ b/internal/lsp/testdata/nested_complit/nested_complit.go.in @@ -0,0 +1,13 @@ +package nested_complit + +type ncFoo struct {} //@item(structNCFoo, "ncFoo", "struct{...}", "struct") + +type ncBar struct { //@item(structNCBar, "ncBar", "struct{...}", "struct") + baz []ncFoo +} + +func _() { + _ := ncBar{ + baz: [] //@complete(" //", structNCBar, structNCFoo) + } +} diff --git a/internal/lsp/testdata/signature/signature.go b/internal/lsp/testdata/signature/signature.go index 72351c48..7e826a04 100644 --- a/internal/lsp/testdata/signature/signature.go +++ b/internal/lsp/testdata/signature/signature.go @@ -28,12 +28,12 @@ func Qux() { Foo("foo", 123) //@signature(" 1", "Foo(a string, b int) (c bool)", 1) Foo("foo", 123) //@signature(")", "Foo(a string, b int) (c bool)", 1) - Bar(13.37, 0x1337) //@signature("13.37", "Bar(float64, ...byte)", 0) - Bar(13.37, 0x1337) //@signature("0x1337", "Bar(float64, ...byte)", 1) + Bar(13.37, 0x13) //@signature("13.37", "Bar(float64, ...byte)", 0) + Bar(13.37, 0x37) //@signature("0x37", "Bar(float64, ...byte)", 1) Bar(13.37, 1, 2, 3, 4) //@signature("4", "Bar(float64, ...byte)", 1) fn := func(hi, there string) func(i int) rune { - return 0 + return func(int) rune { return 0 } } fn("hi", "there") //@signature("hi", "fn(hi string, there string) func(i int) rune", 0) @@ -53,9 +53,10 @@ func Qux() { var ms myStruct ms.foo(nil) //@signature("nil", "foo(e *json.Decoder) (*big.Int, error)", 0) - panic("oops!") //@signature("oops", "panic(interface{})", 0) - make([]int, 1, 2) //@signature("2", "make([]int, int, int) []int", 2) + _ = make([]int, 1, 2) //@signature("2", "make([]int, int, int) []int", 2) Foo(myFunc(123), 456) //@signature("myFunc", "Foo(a string, b int) (c bool)", 0) Foo(myFunc(123), 456) //@signature("123", "myFunc(foo int) string", 0) + + panic("oops!") //@signature("oops", "panic(interface{})", 0) } diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 196087d3..bd36b82b 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -27,7 +27,7 @@ import ( // We hardcode the expected number of test cases to ensure that all tests // are being executed. If a test is added, this number must be changed. const ( - ExpectedCompletionsCount = 65 + ExpectedCompletionsCount = 75 ExpectedDiagnosticsCount = 16 ExpectedFormatCount = 4 ExpectedDefinitionsCount = 21