From 7f68387a4a095a50642da43c02eb7df0702b6369 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 14 Jul 2021 12:29:14 -0400 Subject: [PATCH] internal/lsp/source: workspace symbol improvements for selectors This CL adds various improvements for matching nested fields and methods: - Limit the symbols we produce to not show unqualified fields/methods, and not show partial package paths. - Handle embedded selectors, by trimming the package path. - Improve the internal API used by symbolizers to operate on named chunks. Fixes golang/go#46997 Change-Id: I86cbe998adbb8e52549c937e330896134c375ed7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/334531 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro Reviewed-by: Rebecca Stambler TryBot-Result: Go Bot --- internal/lsp/source/workspace_symbol.go | 128 +++++++++---------- internal/lsp/source/workspace_symbol_test.go | 51 -------- 2 files changed, 64 insertions(+), 115 deletions(-) diff --git a/internal/lsp/source/workspace_symbol.go b/internal/lsp/source/workspace_symbol.go index c0aabf2afea..18583ae5dab 100644 --- a/internal/lsp/source/workspace_symbol.go +++ b/internal/lsp/source/workspace_symbol.go @@ -57,77 +57,75 @@ func WorkspaceSymbols(ctx context.Context, matcherType SymbolMatcher, style Symb // See the comment for symbolCollector for more information. type matcherFunc func(name string) float64 -// A symbolizer returns the best symbol match for name with pkg, according to -// some heuristic. +// A symbolizer returns the best symbol match for a name with pkg, according to +// some heuristic. The symbol name is passed as the slice nameParts of logical +// name pieces. For example, for myType.field the caller can pass either +// []string{"myType.field"} or []string{"myType.", "field"}. // // See the comment for symbolCollector for more information. -type symbolizer func(name string, pkg Package, m matcherFunc) (string, float64) +type symbolizer func(nameParts []string, pkg Package, m matcherFunc) (string, float64) -func fullyQualifiedSymbolMatch(name string, pkg Package, matcher matcherFunc) (string, float64) { - _, score := dynamicSymbolMatch(name, pkg, matcher) +func fullyQualifiedSymbolMatch(nameParts []string, pkg Package, matcher matcherFunc) (string, float64) { + _, score := dynamicSymbolMatch(nameParts, pkg, matcher) + path := append([]string{pkg.PkgPath() + "."}, nameParts...) if score > 0 { - return pkg.PkgPath() + "." + name, score + return strings.Join(path, ""), score } return "", 0 } -func dynamicSymbolMatch(name string, pkg Package, matcher matcherFunc) (string, float64) { - // Prefer any package-qualified match. - pkgQualified := pkg.Name() + "." + name - if match, score := bestMatch(pkgQualified, matcher); match != "" { - return match, score +func dynamicSymbolMatch(nameParts []string, pkg Package, matcher matcherFunc) (string, float64) { + var best string + fullName := strings.Join(nameParts, "") + var score float64 + var name string + + // Compute the match score by finding the highest scoring suffix. In these + // cases the matched symbol is still the full name: it is confusing to match + // an unqualified nested field or method. + if match := bestMatch("", nameParts, matcher); match > score { + best = fullName + score = match } - fullyQualified := pkg.PkgPath() + "." + name - if match, score := bestMatch(fullyQualified, matcher); match != "" { - return match, score + + // Next: try to match a package-qualified name. + name = pkg.Name() + "." + fullName + if match := matcher(name); match > score { + best = name + score = match } - return "", 0 -} -func packageSymbolMatch(name string, pkg Package, matcher matcherFunc) (string, float64) { - qualified := pkg.Name() + "." + name - if matcher(qualified) > 0 { - return qualified, 1 + // Finally: consider a fully qualified name. + prefix := pkg.PkgPath() + "." + fullyQualified := prefix + fullName + // As with field/method selectors, consider suffixes from right to left, but + // always return a fully-qualified symbol. + pathParts := strings.SplitAfter(prefix, "/") + if match := bestMatch(fullName, pathParts, matcher); match > score { + best = fullyQualified + score = match } - return "", 0 + return best, score } -// bestMatch returns the highest scoring symbol suffix of fullPath, starting -// from the right and splitting on selectors and path components. -// -// e.g. given a symbol path of the form 'host.com/dir/pkg.type.field', we -// check the match quality of the following: -// - field -// - type.field -// - pkg.type.field -// - dir/pkg.type.field -// - host.com/dir/pkg.type.field -// -// and return the best match, along with its score. -// -// This is used to implement the 'dynamic' symbol style. -func bestMatch(fullPath string, matcher matcherFunc) (string, float64) { - pathParts := strings.Split(fullPath, "/") - dottedParts := strings.Split(pathParts[len(pathParts)-1], ".") - - var best string +func bestMatch(name string, prefixParts []string, matcher matcherFunc) float64 { var score float64 - - for i := 0; i < len(dottedParts); i++ { - path := strings.Join(dottedParts[len(dottedParts)-1-i:], ".") - if match := matcher(path); match > score { - best = path + for i := len(prefixParts) - 1; i >= 0; i-- { + name = prefixParts[i] + name + if match := matcher(name); match > score { score = match } } - for i := 0; i < len(pathParts); i++ { - path := strings.Join(pathParts[len(pathParts)-1-i:], "/") - if match := matcher(path); match > score { - best = path - score = match - } + return score +} + +func packageSymbolMatch(components []string, pkg Package, matcher matcherFunc) (string, float64) { + path := append([]string{pkg.Name() + "."}, components...) + qualified := strings.Join(path, "") + if matcher(qualified) > 0 { + return qualified, 1 } - return best, score + return "", 0 } // symbolCollector holds context as we walk Packages, gathering symbols that @@ -420,7 +418,13 @@ func (sc *symbolCollector) walkType(typ ast.Expr, path ...*ast.Ident) { // or named. path is the path of nested identifiers containing the field. func (sc *symbolCollector) walkField(field *ast.Field, unnamedKind, namedKind protocol.SymbolKind, path ...*ast.Ident) { if len(field.Names) == 0 { - sc.match(types.ExprString(field.Type), unnamedKind, field, path...) + switch typ := field.Type.(type) { + case *ast.SelectorExpr: + // embedded qualified type + sc.match(typ.Sel.Name, unnamedKind, field, path...) + default: + sc.match(types.ExprString(field.Type), unnamedKind, field, path...) + } } for _, name := range field.Names { sc.match(name.Name, namedKind, name, path...) @@ -466,18 +470,14 @@ func (sc *symbolCollector) match(name string, kind protocol.SymbolKind, node ast } isExported := isExported(name) - if len(path) > 0 { - var nameBuilder strings.Builder - for _, ident := range path { - nameBuilder.WriteString(ident.Name) - nameBuilder.WriteString(".") - if !ident.IsExported() { - isExported = false - } + var names []string + for _, ident := range path { + names = append(names, ident.Name+".") + if !ident.IsExported() { + isExported = false } - nameBuilder.WriteString(name) - name = nameBuilder.String() } + names = append(names, name) // Factors to apply to the match score for the purpose of downranking // results. @@ -501,7 +501,7 @@ func (sc *symbolCollector) match(name string, kind protocol.SymbolKind, node ast // can be noisy. fieldFactor = 0.5 ) - symbol, score := sc.symbolizer(name, sc.current.pkg, sc.matcher) + symbol, score := sc.symbolizer(names, sc.current.pkg, sc.matcher) // Downrank symbols outside of the workspace. if !sc.current.isWorkspace { diff --git a/internal/lsp/source/workspace_symbol_test.go b/internal/lsp/source/workspace_symbol_test.go index f3d9dbb9d44..def73ce01de 100644 --- a/internal/lsp/source/workspace_symbol_test.go +++ b/internal/lsp/source/workspace_symbol_test.go @@ -5,7 +5,6 @@ package source import ( - "strings" "testing" ) @@ -45,53 +44,3 @@ func TestParseQuery(t *testing.T) { } } } - -func TestBestMatch(t *testing.T) { - tests := []struct { - desc string - symbol string - matcher matcherFunc - wantMatch string - wantScore float64 - }{ - { - desc: "shortest match", - symbol: "foo/bar/baz.quux", - matcher: func(string) float64 { return 1.0 }, - wantMatch: "quux", - wantScore: 1.0, - }, - { - desc: "partial match", - symbol: "foo/bar/baz.quux", - matcher: func(s string) float64 { - if strings.HasPrefix(s, "bar") { - return 1.0 - } - return 0.0 - }, - wantMatch: "bar/baz.quux", - wantScore: 1.0, - }, - { - desc: "longest match", - symbol: "foo/bar/baz.quux", - matcher: func(s string) float64 { - parts := strings.Split(s, "/") - return float64(len(parts)) - }, - wantMatch: "foo/bar/baz.quux", - wantScore: 3.0, - }, - } - - for _, test := range tests { - test := test - t.Run(test.desc, func(t *testing.T) { - gotMatch, gotScore := bestMatch(test.symbol, test.matcher) - if gotMatch != test.wantMatch || gotScore != test.wantScore { - t.Errorf("bestMatch(%q, matcher) = (%q, %.2g), want (%q, %.2g)", test.symbol, gotMatch, gotScore, test.wantMatch, test.wantScore) - } - }) - } -}