From ef1313dc6d0aa5fb2b1427480d0ca8fabd120917 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Thu, 5 Mar 2020 21:46:49 -0800 Subject: [PATCH] internal/lsp/source: prefer unexported and non-func candidates A common annoying mis-completion is as follows: type foo struct { field int } func (f foo) Field() int { return f.field } func (f foo) logic() { if f.f<> } Now at <> we prefer "field" over "Field()". Similarly: type foo struct { } func (f foo) DoSomething() { } func (f foo) doSomething() { } func (f foo) logic() { f.d<> } Now at <> we prefer "doSomething()" over "DoSomething()". All else being equally, you normally want private objects over public objects when the private objects are available. The same logic is applied to deep completions so we prefer "c.foo.bar" over "c.Foo().bar". Change-Id: Ic91cba7721ddb1f2a30338037693ddcce8c621f7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/223877 Run-TryBot: Muir Manders Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/source/completion.go | 15 +++++++++++-- internal/lsp/source/deep_completion.go | 22 +++++++++++++++++++ .../lsp/testdata/lsp/primarymod/bad/bad1.go | 8 +++---- .../lsp/primarymod/badstmt/badstmt.go.in | 2 +- .../lsp/primarymod/badstmt/badstmt_2.go.in | 2 +- .../lsp/primarymod/badstmt/badstmt_3.go.in | 2 +- .../lsp/primarymod/badstmt/badstmt_4.go.in | 2 +- .../lsp/testdata/lsp/primarymod/bar/bar.go.in | 10 ++++----- .../lsp/primarymod/complit/complit.go.in | 16 +++++++------- .../lsp/testdata/lsp/primarymod/deep/deep.go | 22 +++++++++++++++---- .../lsp/primarymod/func_rank/func_rank.go.in | 17 ++++++++++++++ .../lsp/primarymod/nodisk/nodisk.overlay.go | 4 ++-- .../lsp/primarymod/selector/selector.go.in | 16 +++++++------- .../primarymod/unimported/unimported.go.in | 3 ++- internal/lsp/testdata/lsp/summary.txt.golden | 2 +- 15 files changed, 104 insertions(+), 39 deletions(-) diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 41830f08fa..995fd45de7 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -345,8 +345,19 @@ func (c *completer) found(cand candidate) { } } - // Favor shallow matches by lowering weight according to depth. - cand.score -= cand.score * float64(len(c.deepState.chain)) / 10 + // Lower score of function calls so we prefer fields and vars over calls. + if cand.expandFuncCall { + cand.score *= 0.9 + } + + // Prefer private objects over public ones. + if !obj.Exported() && obj.Parent() != types.Universe { + cand.score *= 1.1 + } + + // Favor shallow matches by lowering score according to depth. + cand.score -= cand.score * c.deepState.scorePenalty() + if cand.score < 0 { cand.score = 0 } diff --git a/internal/lsp/source/deep_completion.go b/internal/lsp/source/deep_completion.go index 674ce49335..6a04937c34 100644 --- a/internal/lsp/source/deep_completion.go +++ b/internal/lsp/source/deep_completion.go @@ -93,6 +93,28 @@ func (s *deepCompletionState) isHighScore(score float64) bool { return false } +// scorePenalty computes a deep candidate score penalty. A candidate +// is penalized based on depth to favor shallower candidates. We also +// give a slight bonus to unexported objects and a slight additional +// penalty to function objects. +func (s *deepCompletionState) scorePenalty() float64 { + var deepPenalty float64 + for _, dc := range s.chain { + deepPenalty += 1 + + if !dc.Exported() { + deepPenalty -= 0.1 + } + + if _, isSig := dc.Type().Underlying().(*types.Signature); isSig { + deepPenalty += 0.1 + } + } + + // Normalize penalty to a max depth of 10. + return deepPenalty / 10 +} + func (c *completer) inDeepCompletion() bool { return len(c.deepState.chain) > 0 } diff --git a/internal/lsp/testdata/lsp/primarymod/bad/bad1.go b/internal/lsp/testdata/lsp/primarymod/bad/bad1.go index 512f2d9869..eb547f7932 100644 --- a/internal/lsp/testdata/lsp/primarymod/bad/bad1.go +++ b/internal/lsp/testdata/lsp/primarymod/bad/bad1.go @@ -8,7 +8,7 @@ type stateFunc func() stateFunc //@item(stateFunc, "stateFunc", "func() stateFun var a unknown //@item(global_a, "a", "unknown", "var"),diag("unknown", "compiler", "undeclared name: unknown", "error") func random() int { //@item(random, "random", "func() int", "func") - //@complete("", global_a, bob, random, random2, random3, stateFunc, stuff) + //@complete("", global_a, bob, stateFunc, random, random2, random3, stuff) return 0 } @@ -16,18 +16,18 @@ func random2(y int) int { //@item(random2, "random2", "func(y int) int", "func") x := 6 //@item(x, "x", "int", "var"),diag("x", "compiler", "x declared but not used", "error") var q blah //@item(q, "q", "blah", "var"),diag("q", "compiler", "q declared but not used", "error"),diag("blah", "compiler", "undeclared name: blah", "error") var t **blob //@item(t, "t", "**blob", "var"),diag("t", "compiler", "t declared but not used", "error"),diag("blob", "compiler", "undeclared name: blob", "error") - //@complete("", q, t, x, bad_y_param, global_a, bob, random, random2, random3, stateFunc, stuff) + //@complete("", q, t, x, bad_y_param, global_a, bob, stateFunc, random, random2, random3, stuff) return y } func random3(y ...int) { //@item(random3, "random3", "func(y ...int)", "func"),item(y_variadic_param, "y", "[]int", "var") - //@complete("", y_variadic_param, global_a, bob, random, random2, random3, stateFunc, stuff) + //@complete("", y_variadic_param, global_a, bob, stateFunc, random, random2, random3, stuff) var ch chan (favType1) //@item(ch, "ch", "chan (favType1)", "var"),diag("ch", "compiler", "ch declared but not used", "error"),diag("favType1", "compiler", "undeclared name: favType1", "error") var m map[keyType]int //@item(m, "m", "map[keyType]int", "var"),diag("m", "compiler", "m declared but not used", "error"),diag("keyType", "compiler", "undeclared name: keyType", "error") var arr []favType2 //@item(arr, "arr", "[]favType2", "var"),diag("arr", "compiler", "arr declared but not used", "error"),diag("favType2", "compiler", "undeclared name: favType2", "error") var fn1 func() badResult //@item(fn1, "fn1", "func() badResult", "var"),diag("fn1", "compiler", "fn1 declared but not used", "error"),diag("badResult", "compiler", "undeclared name: badResult", "error") var fn2 func(badParam) //@item(fn2, "fn2", "func(badParam)", "var"),diag("fn2", "compiler", "fn2 declared but not used", "error"),diag("badParam", "compiler", "undeclared name: badParam", "error") - //@complete("", arr, ch, fn1, fn2, m, y_variadic_param, global_a, bob, random, random2, random3, stateFunc, stuff) + //@complete("", arr, ch, fn1, fn2, m, y_variadic_param, global_a, bob, stateFunc, random, random2, random3, stuff) } diff --git a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt.go.in b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt.go.in index 35cfa542f2..d9b65a3b00 100644 --- a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt.go.in +++ b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt.go.in @@ -20,6 +20,6 @@ func _() { func _() { defer func() { foo.F //@complete(" //", Foo),snippet(" //", Foo, "Foo()", "Foo()") - foo. //@complete(" //", Foo, IntFoo, StructFoo) + foo. //@complete(" //", IntFoo, StructFoo, Foo) } } diff --git a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_2.go.in b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_2.go.in index 294701d963..cceb6902d2 100644 --- a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_2.go.in +++ b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_2.go.in @@ -5,5 +5,5 @@ import ( ) func _() { - defer func() { foo. } //@complete(" }", Foo, IntFoo, StructFoo) + defer func() { foo. } //@complete(" }", IntFoo, StructFoo, Foo) } diff --git a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_3.go.in b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_3.go.in index 656ad76738..a44732399f 100644 --- a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_3.go.in +++ b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_3.go.in @@ -5,5 +5,5 @@ import ( ) func _() { - go foo. //@complete(" //", Foo, IntFoo, StructFoo) + go foo. //@complete(" //", IntFoo, StructFoo, Foo) } diff --git a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_4.go.in b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_4.go.in index 87673061e2..a81e2f28a9 100644 --- a/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_4.go.in +++ b/internal/lsp/testdata/lsp/primarymod/badstmt/badstmt_4.go.in @@ -6,6 +6,6 @@ import ( func _() { go func() { - defer foo. //@complete(" //", Foo, IntFoo, StructFoo) + defer foo. //@complete(" //", IntFoo, StructFoo, Foo) } } diff --git a/internal/lsp/testdata/lsp/primarymod/bar/bar.go.in b/internal/lsp/testdata/lsp/primarymod/bar/bar.go.in index 70b69b8f47..dc8e41b72e 100644 --- a/internal/lsp/testdata/lsp/primarymod/bar/bar.go.in +++ b/internal/lsp/testdata/lsp/primarymod/bar/bar.go.in @@ -15,9 +15,9 @@ func _() { // Bar is a function. func Bar() { //@item(Bar, "Bar", "func()", "func", "Bar is a function.") - foo.Foo() //@complete("F", Foo, IntFoo, StructFoo) + foo.Foo() //@complete("F", IntFoo, StructFoo, Foo) var _ foo.IntFoo //@complete("I", IntFoo, StructFoo) - foo.() //@complete("(", Foo, IntFoo, StructFoo) + foo.() //@complete("(", IntFoo, StructFoo, Foo) } func _() { @@ -33,15 +33,15 @@ func _() { Value: 5, //@complete("a", Value) } _ = foo.StructFoo{ - //@complete("", Value, Valentine, foo, Bar, helper) + //@complete("", Value, Valentine, foo, helper, Bar) } _ = foo.StructFoo{ Value: Valen //@complete("le", Valentine) } _ = foo.StructFoo{ - Value: //@complete(" //", Valentine, foo, Bar, helper) + Value: //@complete(" //", Valentine, foo, helper, Bar) } _ = foo.StructFoo{ - Value: //@complete(" ", Valentine, foo, Bar, helper) + Value: //@complete(" ", Valentine, foo, helper, Bar) } } diff --git a/internal/lsp/testdata/lsp/primarymod/complit/complit.go.in b/internal/lsp/testdata/lsp/primarymod/complit/complit.go.in index 6a8fc5988c..83ccb2951b 100644 --- a/internal/lsp/testdata/lsp/primarymod/complit/complit.go.in +++ b/internal/lsp/testdata/lsp/primarymod/complit/complit.go.in @@ -9,7 +9,7 @@ type position struct { //@item(structPosition, "position", "struct{...}", "struc func _() { _ = position{ - //@complete("", fieldX, fieldY, cVar, structPosition) + //@complete("", fieldX, fieldY, structPosition, cVar) } _ = position{ X: 1, @@ -21,7 +21,7 @@ func _() { } _ = []*position{ { - //@complete("", fieldX, fieldY, cVar, structPosition) + //@complete("", fieldX, fieldY, structPosition, cVar) }, } } @@ -37,7 +37,7 @@ func _() { } _ = map[int]int{ - //@complete("", abVar, aaVar, cVar, structPosition) + //@complete("", abVar, aaVar, structPosition, cVar) } _ = []string{a: ""} //@complete(":", abVar, aaVar) @@ -45,10 +45,10 @@ func _() { _ = position{X: a} //@complete("}", abVar, aaVar) _ = position{a} //@complete("}", abVar, aaVar) - _ = position{a, } //@complete("}", abVar, aaVar, cVar, structPosition) + _ = position{a, } //@complete("}", abVar, aaVar, structPosition, cVar) - _ = []int{a} //@complete("a", abVar, aaVar, cVar, structPosition) - _ = [1]int{a} //@complete("a", abVar, aaVar, cVar, structPosition) + _ = []int{a} //@complete("a", abVar, aaVar, structPosition, cVar) + _ = [1]int{a} //@complete("a", abVar, aaVar, structPosition, cVar) type myStruct struct { AA int //@item(fieldAA, "AA", "int", "field") @@ -73,7 +73,7 @@ func _() { func _() { _ := position{ - X: 1, //@complete("X", fieldX),complete(" 1", cVar, structPosition) - Y: , //@complete(":", fieldY),complete(" ,", cVar, structPosition) + X: 1, //@complete("X", fieldX),complete(" 1", structPosition, cVar) + Y: , //@complete(":", fieldY),complete(" ,", structPosition, cVar) } } diff --git a/internal/lsp/testdata/lsp/primarymod/deep/deep.go b/internal/lsp/testdata/lsp/primarymod/deep/deep.go index 0a1dfd5c49..09dd1b8544 100644 --- a/internal/lsp/testdata/lsp/primarymod/deep/deep.go +++ b/internal/lsp/testdata/lsp/primarymod/deep/deep.go @@ -113,9 +113,23 @@ func _() { f foo ) - f.b.ptrReceiver() //@item(deepBPtr, "f.b.ptrReceiver", "func() int", "method") - f.bar().valueReceiver //@item(deepBarValue, "f.bar().valueReceiver", "func() int", "method") - f.barPtr().ptrReceiver //@item(deepBarPtrPtr, "f.barPtr().ptrReceiver", "func() int", "method") + f.bar().valueReceiver //@item(deepBarValue, "f.bar().valueReceiver", "func() int", "method") + f.barPtr().ptrReceiver //@item(deepBarPtrPtr, "f.barPtr().ptrReceiver", "func() int", "method") + f.barPtr().valueReceiver //@item(deepBarPtrValue, "f.barPtr().valueReceiver", "func() int", "method") - i = fb //@fuzzy(" //", deepBPtr, deepBarValue, deepBarPtrPtr) + i = fbar //@fuzzy(" //", deepBarValue, deepBarPtrPtr, deepBarPtrValue) +} + +func (b baz) Thing() struct{ val int } { + return b.thing +} + +type baz struct { + thing struct{ val int } +} + +func (b baz) _() { + b.Thing().val //@item(deepBazMethVal, "b.Thing().val", "int", "field") + b.thing.val //@item(deepBazFieldVal, "b.thing.val", "int", "field") + var _ int = b //@rank(" //", deepBazFieldVal, deepBazMethVal) } diff --git a/internal/lsp/testdata/lsp/primarymod/func_rank/func_rank.go.in b/internal/lsp/testdata/lsp/primarymod/func_rank/func_rank.go.in index f9cc6a1d34..58b939f2c1 100644 --- a/internal/lsp/testdata/lsp/primarymod/func_rank/func_rank.go.in +++ b/internal/lsp/testdata/lsp/primarymod/func_rank/func_rank.go.in @@ -42,3 +42,20 @@ func _() { return s.A //@complete(" //", rankAB, rankAA, rankAC) } } + +type foo struct { + fooPrivateField int //@item(rankFooPrivField, "fooPrivateField", "int", "field") + FooPublicField int //@item(rankFooPubField, "FooPublicField", "int", "field") +} + +func (foo) fooPrivateMethod() int { //@item(rankFooPrivMeth, "fooPrivateMethod", "func() int", "method") + return 0 +} + +func (foo) FooPublicMethod() int { //@item(rankFooPubMeth, "FooPublicMethod", "func() int", "method") + return 0 +} + +func _() { + var _ int = foo{}. //@rank(" //", rankFooPrivField, rankFooPubField),rank(" //", rankFooPrivMeth, rankFooPubMeth),rank(" //", rankFooPrivField, rankFooPrivMeth) +} diff --git a/internal/lsp/testdata/lsp/primarymod/nodisk/nodisk.overlay.go b/internal/lsp/testdata/lsp/primarymod/nodisk/nodisk.overlay.go index 0831f0eae3..fa2a6414b1 100644 --- a/internal/lsp/testdata/lsp/primarymod/nodisk/nodisk.overlay.go +++ b/internal/lsp/testdata/lsp/primarymod/nodisk/nodisk.overlay.go @@ -5,5 +5,5 @@ import ( ) func _() { - foo.Foo() //@complete("F", Foo, IntFoo, StructFoo) -} \ No newline at end of file + foo.Foo() //@complete("F", IntFoo, StructFoo, Foo) +} diff --git a/internal/lsp/testdata/lsp/primarymod/selector/selector.go.in b/internal/lsp/testdata/lsp/primarymod/selector/selector.go.in index 6c6e1fd4fd..277f98bde7 100644 --- a/internal/lsp/testdata/lsp/primarymod/selector/selector.go.in +++ b/internal/lsp/testdata/lsp/primarymod/selector/selector.go.in @@ -27,7 +27,7 @@ func _() { b := &bob{} y := b.george(). jack(); - y.; //@complete(";", jill, c) + y.; //@complete(";", c, jill) } func _() { @@ -35,10 +35,10 @@ func _() { x := 5 var b *bob - b. //@complete(" /", george, a) + b. //@complete(" /", a, george) y, z := 5, 6 - b. //@complete(" /", george, a) + b. //@complete(" /", a, george) y, z, a, b, c := 5, 6 } @@ -52,15 +52,15 @@ func _() { func _() { var b *bob - if y != b. //@complete(" /", george, a) + if y != b. //@complete(" /", a, george) z := 5 - if z + y + 1 + b. //@complete(" /", george, a) + if z + y + 1 + b. //@complete(" /", a, george) r, s, t := 4, 5 - if y != b. //@complete(" /", george, a) + if y != b. //@complete(" /", a, george) z = 5 - if z + y + 1 + b. //@complete(" /", george, a) + if z + y + 1 + b. //@complete(" /", a, george) r = 4 -} \ No newline at end of file +} diff --git a/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in b/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in index d9f109c6ff..3c5ab11cd9 100644 --- a/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in +++ b/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in @@ -6,7 +6,8 @@ func _() { // container/ring is extremely unlikely to be imported by anything, so shouldn't have type information. ring.Ring //@unimported("Ring", ringring) signature.Foo //@unimported("Foo", signaturefoo) - context.Bac //@unimported("Bac", contextBackground, contextBackgroundErr) + + context.Bac //@unimported(" //", contextBackground, contextBackgroundErr) } // Create markers for unimported std lib packages. Only for use by this test. diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 09247fac35..c4e7d68d37 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/internal/lsp/testdata/lsp/summary.txt.golden @@ -5,7 +5,7 @@ CompletionSnippetCount = 75 UnimportedCompletionsCount = 11 DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 -RankedCompletionsCount = 116 +RankedCompletionsCount = 120 CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 39 FoldingRangesCount = 2