From 2ef0387721d2ff1d47cf2dd9dbbb280c3efb5abe Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Thu, 6 Feb 2020 21:22:05 -0800 Subject: [PATCH] internal/lsp/source: fix unimported member completion ranking When completing members on a type checked, unimported package, you get fully typed members. That means you get deep completions. Because we downrank the initial unimported package members so much, any deep completions were dominating the rankings. For example context.Back<> yielded "context.Background().Err" ranked above "context.Background". Fix by scoring context.Background in this example as stdScore+tinyRelevanceScore instead of just tinyRelevanceScore. I also changed untyped candidate scores in the same way so they stay competitive when you have both imported and unimported candidates. The other option was to propagate the score penalty into deep candidates, but that wasn't easy. In general I think you are better off avoiding big score penalties because they complicate the interplay between different kinds of candidates. Scoring needs an overhaul, but at least we are building up our test suite in the meantime. Change-Id: Ia5d32c057b04174229686cec6ac0542c30e186e2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/218378 Run-TryBot: Muir Manders TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/source/completion.go | 4 ++-- .../testdata/lsp/primarymod/unimported/unimported.go.in | 8 ++++++-- .../lsp/primarymod/unimported/unimported_cand_type.go | 4 +++- internal/lsp/testdata/lsp/summary.txt.golden | 2 +- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 82324a25bd..76fda99d27 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -721,7 +721,7 @@ func (c *completer) unimportedMembers(id *ast.Ident) error { if imports.ImportPathToAssumedName(path) != pkg.GetTypes().Name() { imp.name = pkg.GetTypes().Name() } - c.packageMembers(pkg.GetTypes(), .01*float64(relevances[path]), imp) + c.packageMembers(pkg.GetTypes(), stdScore+.01*float64(relevances[path]), imp) if len(c.items) >= unimportedTarget { return nil } @@ -740,7 +740,7 @@ func (c *completer) unimportedMembers(id *ast.Ident) error { // Continue with untyped proposals. pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName) for _, export := range pkgExport.Exports { - score := 0.01 * float64(pkgExport.Fix.Relevance) + score := stdScore + 0.01*float64(pkgExport.Fix.Relevance) c.found(candidate{ obj: types.NewVar(0, pkg, export, nil), score: score, diff --git a/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in b/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in index d4ce8b3d25..ef5dc4caec 100644 --- a/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in +++ b/internal/lsp/testdata/lsp/primarymod/unimported/unimported.go.in @@ -4,8 +4,9 @@ func _() { //@unimported("", hashslashadler32, goslashast, encodingslashbase64, bytes) pkg //@unimported("g", externalpackage) // container/ring is extremely unlikely to be imported by anything, so shouldn't have type information. - ring.Ring //@unimported("Ring", ringring) + ring.Ring //@unimported("Ring", ringring) signature.Foo //@unimported("Foo", signaturefoo) + context.Bac //@unimported("Bac", contextBackground, contextBackgroundErr) } // Create markers for unimported std lib packages. Only for use by this test. @@ -17,4 +18,7 @@ func _() { /* ring.Ring */ //@item(ringring, "Ring", "(from \"container/ring\")", "var") -/* signature.Foo */ //@item(signaturefoo, "Foo", "func(a string, b int) (c bool) (from \"golang.org/x/tools/internal/lsp/signature\")", "func") \ No newline at end of file +/* signature.Foo */ //@item(signaturefoo, "Foo", "func(a string, b int) (c bool) (from \"golang.org/x/tools/internal/lsp/signature\")", "func") + +/* context.Background */ //@item(contextBackground, "Background", "func() context.Context (from \"context\")", "func") +/* context.Background().Err */ //@item(contextBackgroundErr, "Background().Err", "func() error (from \"context\")", "method") diff --git a/internal/lsp/testdata/lsp/primarymod/unimported/unimported_cand_type.go b/internal/lsp/testdata/lsp/primarymod/unimported/unimported_cand_type.go index 2690c1a4a2..531aa2d180 100644 --- a/internal/lsp/testdata/lsp/primarymod/unimported/unimported_cand_type.go +++ b/internal/lsp/testdata/lsp/primarymod/unimported/unimported_cand_type.go @@ -1,8 +1,10 @@ package unimported import ( + _ "context" + "golang.org/x/tools/internal/lsp/baz" - "golang.org/x/tools/internal/lsp/signature" // provide type information for unimported completions in the other file + _ "golang.org/x/tools/internal/lsp/signature" // provide type information for unimported completions in the other file ) func _() { diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 519a643566..d6d706caca 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/internal/lsp/testdata/lsp/summary.txt.golden @@ -1,7 +1,7 @@ -- summary -- CompletionsCount = 227 CompletionSnippetCount = 66 -UnimportedCompletionsCount = 9 +UnimportedCompletionsCount = 11 DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 RankedCompletionsCount = 84