gopls/internal/lsp/source: add the "symbolScope" option

Add a new "symbolScope" option that controls whether matches are
restricted to workspace packages only. This is the new default behavior,
though the old behavior can be enabled by setting "symbolScope": "all".

Fixes golang/go#37236

Change-Id: Ic614b57005488e61ea0c018a68076785e667db16
Reviewed-on: https://go-review.googlesource.com/c/tools/+/490935
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>

Author: findleyr

gopls/doc/settings.md

@@ -456,6 +456,22 @@ just "Foo.Field".
 
 Default: `"Dynamic"`.
 
+##### **symbolScope** *enum*
+
+symbolScope controls which packages are searched for workspace/symbol
+requests. The default value, "workspace", searches only workspace
+packages. The legacy behavior, "all", causes all loaded packages to be
+searched, including dependencies; this is more expensive and may return
+unwanted results.
+
+Must be one of:
+
+* `"all"` matches symbols in any loaded package, including
+dependencies.
+* `"workspace"` matches symbols in workspace packages only.
+
+Default: `"workspace"`.
+
 #### **verboseOutput** *bool*
 
 **This setting is for debugging purposes only.**

gopls/internal/lsp/cache/snapshot.go

@@ -1113,18 +1113,26 @@ func (s *snapshot) WorkspaceMetadata(ctx context.Context) ([]*source.Metadata, e
 // a loaded package. It awaits snapshot loading.
 //
 // TODO(rfindley): move this to the top of cache/symbols.go
-func (s *snapshot) Symbols(ctx context.Context) (map[span.URI][]source.Symbol, error) {
+func (s *snapshot) Symbols(ctx context.Context, workspaceOnly bool) (map[span.URI][]source.Symbol, error) {
 	if err := s.awaitLoaded(ctx); err != nil {
 		return nil, err
 	}
 
-	// Build symbols for all loaded Go files.
-	s.mu.Lock()
-	meta := s.meta
-	s.mu.Unlock()
+	var (
+		meta []*source.Metadata
+		err  error
+	)
+	if workspaceOnly {
+		meta, err = s.WorkspaceMetadata(ctx)
+	} else {
+		meta, err = s.AllMetadata(ctx)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("loading metadata: %v", err)
+	}
 
 	goFiles := make(map[span.URI]struct{})
-	for _, m := range meta.metadata {
+	for _, m := range meta {
 		for _, uri := range m.GoFiles {
 			goFiles[uri] = struct{}{}
 		}

gopls/internal/lsp/regtest/marker.go

@@ -882,14 +882,21 @@ func (run *markerTestRun) fmtPos(pos token.Pos) string {
 // archive-relative paths for files and including the line number in the full
 // archive file.
 func (run *markerTestRun) fmtLoc(loc protocol.Location) string {
-	return run.fmtLocDetails(loc, true)
+	formatted := run.fmtLocDetails(loc, true)
+	if formatted == "" {
+		run.env.T.Errorf("unable to find %s in test archive", loc)
+		return "<invalid location>"
+	}
+	return formatted
 }
 
 // See fmtLoc. If includeTxtPos is not set, the position in the full archive
 // file is omitted.
+//
+// If the location cannot be found within the archive, fmtLocDetails returns "".
 func (run *markerTestRun) fmtLocDetails(loc protocol.Location, includeTxtPos bool) string {
 	if loc == (protocol.Location{}) {
-		return "<missing location>"
+		return ""
 	}
 	lines := bytes.Count(run.test.archive.Comment, []byte("\n"))
 	var name string
@@ -903,8 +910,7 @@ func (run *markerTestRun) fmtLocDetails(loc protocol.Location, includeTxtPos boo
 		lines += bytes.Count(f.Data, []byte("\n"))
 	}
 	if name == "" {
-		run.env.T.Errorf("unable to find %s in test archive", loc)
-		return "<invalid location>"
+		return ""
 	}
 	m, err := run.env.Editor.Mapper(name)
 	if err != nil {
@@ -1675,6 +1681,11 @@ func workspaceSymbolMarker(mark marker, query string, golden *Golden) {
 		// Omit the txtar position of the symbol location; otherwise edits to the
 		// txtar archive lead to unexpected failures.
 		loc := mark.run.fmtLocDetails(s.Location, false)
+		// TODO(rfindley): can we do better here, by detecting if the location is
+		// relative to GOROOT?
+		if loc == "" {
+			loc = "<unknown>"
+		}
 		fmt.Fprintf(&got, "%s %s %s\n", loc, s.Name, s.Kind)
 	}
 

gopls/internal/lsp/source/api_json.go

@@ -143,6 +143,7 @@ func DefaultOptions() *Options {
 						ImportShortcut: BothShortcuts,
 						SymbolMatcher:  SymbolFastFuzzy,
 						SymbolStyle:    DynamicSymbols,
+						SymbolScope:    WorkspaceSymbolScope,
 					},
 					CompletionOptions: CompletionOptions{
 						Matcher:                        Fuzzy,
@@ -454,6 +455,13 @@ type NavigationOptions struct {
 	// }
 	// ```
 	SymbolStyle SymbolStyle `status:"advanced"`
+
+	// SymbolScope controls which packages are searched for workspace/symbol
+	// requests. The default value, "workspace", searches only workspace
+	// packages. The legacy behavior, "all", causes all loaded packages to be
+	// searched, including dependencies; this is more expensive and may return
+	// unwanted results.
+	SymbolScope SymbolScope
 }
 
 // UserOptions holds custom Gopls configuration (not part of the LSP) that is
@@ -617,6 +625,8 @@ const (
 	CaseSensitive   Matcher = "CaseSensitive"
 )
 
+// A SymbolMatcher controls the matching of symbols for workspace/symbol
+// requests.
 type SymbolMatcher string
 
 const (
@@ -626,6 +636,7 @@ const (
 	SymbolCaseSensitive   SymbolMatcher = "CaseSensitive"
 )
 
+// A SymbolStyle controls the formatting of symbols in workspace/symbol results.
 type SymbolStyle string
 
 const (
@@ -642,6 +653,17 @@ const (
 	DynamicSymbols SymbolStyle = "Dynamic"
 )
 
+// A SymbolScope controls the search scope for workspace/symbol requests.
+type SymbolScope string
+
+const (
+	// WorkspaceSymbolScope matches symbols in workspace packages only.
+	WorkspaceSymbolScope SymbolScope = "workspace"
+	// AllSymbolScope matches symbols in any loaded package, including
+	// dependencies.
+	AllSymbolScope SymbolScope = "all"
+)
+
 type HoverKind string
 
 const (
@@ -969,6 +991,14 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{})
 			o.SymbolStyle = SymbolStyle(s)
 		}
 
+	case "symbolScope":
+		if s, ok := result.asOneOf(
+			string(WorkspaceSymbolScope),
+			string(AllSymbolScope),
+		); ok {
+			o.SymbolScope = SymbolScope(s)
+		}
+
 	case "hoverKind":
 		if s, ok := result.asOneOf(
 			string(NoDocumentation),

gopls/internal/lsp/source/options.go

@@ -159,7 +159,10 @@ type Snapshot interface {
 	CriticalError(ctx context.Context) *CriticalError
 
 	// Symbols returns all symbols in the snapshot.
-	Symbols(ctx context.Context) (map[span.URI][]Symbol, error)
+	//
+	// If workspaceOnly is set, this only includes symbols from files in a
+	// workspace package. Otherwise, it returns symbols from all loaded packages.
+	Symbols(ctx context.Context, workspaceOnly bool) (map[span.URI][]Symbol, error)
 
 	// -- package metadata --
 

gopls/internal/lsp/source/view.go

@@ -317,10 +317,16 @@ func collectSymbols(ctx context.Context, views []View, matcherType SymbolMatcher
 		filters := v.Options().DirectoryFilters
 		filterer := NewFilterer(filters)
 		folder := filepath.ToSlash(v.Folder().Filename())
-		symbols, err := snapshot.Symbols(ctx)
+
+		workspaceOnly := true
+		if v.Options().SymbolScope == AllSymbolScope {
+			workspaceOnly = false
+		}
+		symbols, err := snapshot.Symbols(ctx, workspaceOnly)
 		if err != nil {
 			return nil, err
 		}
+
 		for uri, syms := range symbols {
 			norm := filepath.ToSlash(uri.Filename())
 			nm := strings.TrimPrefix(norm, folder)
@@ -496,6 +502,8 @@ func matchFile(store *symbolStore, symbolizer symbolizer, matcher matcherFunc, r
 			}
 		}
 
+		// TODO(rfindley): use metadata to determine if the file is in a workspace
+		// package, rather than this heuristic.
 		inWorkspace := false
 		for _, root := range roots {
 			if strings.HasPrefix(string(i.uri), root) {

gopls/internal/lsp/source/workspace_symbol.go

@@ -0,0 +1,30 @@
+This test verifies behavior when "symbolScope" is set to "all".
+
+-- settings.json --
+{
+	"symbolStyle": "full",
+	"symbolMatcher": "casesensitive",
+	"symbolScope": "all"
+}
+
+-- go.mod --
+module mod.test/symbols
+
+go 1.18
+
+-- query.go --
+package symbols
+
+//@workspacesymbol("fmt.Println", println)
+
+-- fmt/fmt.go --
+package fmt
+
+import "fmt"
+
+func Println(s string) {
+	fmt.Println(s)
+}
+-- @println --
+fmt/fmt.go:5:6-13 mod.test/symbols/fmt.Println Function
+<unknown> fmt.Println Function

gopls/internal/regtest/marker/testdata/workspacesymbol/allscope.txt

@@ -0,0 +1,29 @@
+This test verifies behavior when "symbolScope" is set to the default value
+("workspace").
+
+-- settings.json --
+{
+	"symbolStyle": "full",
+	"symbolMatcher": "casesensitive"
+}
+
+-- go.mod --
+module mod.test/symbols
+
+go 1.18
+
+-- query.go --
+package symbols
+
+//@workspacesymbol("fmt.Println", println)
+
+-- fmt/fmt.go --
+package fmt
+
+import "fmt"
+
+func Println(s string) {
+	fmt.Println(s)
+}
+-- @println --
+fmt/fmt.go:5:6-13 mod.test/symbols/fmt.Println Function

gopls/internal/regtest/marker/testdata/workspacesymbol/wsscope.txt

@@ -73,7 +73,6 @@ const (
 			"Fooex",  // shorter than Fooest, FooBar, lexically before Fooey
 			"Fooey",  // shorter than Fooest, Foobar
 			"Fooest",
-			"unsafe.Offsetof", // a very fuzzy match
 		)
 	})
 }