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 <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>
TryBot-Result: Go Bot <[email protected]>

Author: findleyr

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 {

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)
-			}
-		})
-	}
-}