gopls/internal/lsp/cache: move Option management to the Server

In order to move toward tracking options by Folder, not view, move them
into the Server. This will also help us fix bugs related to
configuration lifecycle events.

For golang/go#57979
Updates golang/go#42814

Change-Id: Id281cad20697756138a7bdc67f718a7468a04d4a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/526417
LUCI-TryBot-Result: Go LUCI <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/cache.go

@@ -11,7 +11,6 @@ import (
 	"sync/atomic"
 	"time"
 
-	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/gocommand"
 	"golang.org/x/tools/internal/memoize"
@@ -56,17 +55,12 @@ type Cache struct {
 // The provided optionsOverrides may be nil.
 //
 // TODO(rfindley): move this to session.go.
-func NewSession(ctx context.Context, c *Cache, optionsOverrides func(*source.Options)) *Session {
+func NewSession(ctx context.Context, c *Cache) *Session {
 	index := atomic.AddInt64(&sessionIndex, 1)
-	options := source.DefaultOptions().Clone()
-	if optionsOverrides != nil {
-		optionsOverrides(options)
-	}
 	s := &Session{
 		id:          strconv.FormatInt(index, 10),
 		cache:       c,
 		gocmdRunner: &gocommand.Runner{},
-		options:     options,
 		overlayFS:   newOverlayFS(c),
 		parseCache:  newParseCache(1 * time.Minute), // keep recently parsed files for a minute, to optimize typing CPU
 	}

gopls/internal/lsp/cache/session.go

@@ -33,12 +33,9 @@ type Session struct {
 	cache       *Cache            // shared cache
 	gocmdRunner *gocommand.Runner // limits go command concurrency
 
-	optionsMu sync.Mutex
-	options   *source.Options
-
 	viewMu  sync.Mutex
 	views   []*View
-	viewMap map[span.URI]*View // map of URI->best view
+	viewMap map[span.URI]*View // file->best view
 
 	parseCache *parseCache
 
@@ -54,20 +51,6 @@ func (s *Session) GoCommandRunner() *gocommand.Runner {
 	return s.gocmdRunner
 }
 
-// Options returns a copy of the SessionOptions for this session.
-func (s *Session) Options() *source.Options {
-	s.optionsMu.Lock()
-	defer s.optionsMu.Unlock()
-	return s.options
-}
-
-// SetOptions sets the options of this session to new values.
-func (s *Session) SetOptions(options *source.Options) {
-	s.optionsMu.Lock()
-	defer s.optionsMu.Unlock()
-	s.options = options
-}
-
 // Shutdown the session and all views it has created.
 func (s *Session) Shutdown(ctx context.Context) {
 	var views []*View
@@ -293,6 +276,7 @@ func bestViewForURI(uri span.URI, views []*View) *View {
 func (s *Session) RemoveView(view *View) {
 	s.viewMu.Lock()
 	defer s.viewMu.Unlock()
+
 	i := s.dropView(view)
 	if i == -1 { // error reported elsewhere
 		return
@@ -302,18 +286,11 @@ func (s *Session) RemoveView(view *View) {
 	s.views = removeElement(s.views, i)
 }
 
-// updateView recreates the view with the given options.
+// updateViewLocked recreates the view with the given options.
 //
 // If the resulting error is non-nil, the view may or may not have already been
 // dropped from the session.
-func (s *Session) updateView(ctx context.Context, view *View, options *source.Options) (*View, error) {
-	s.viewMu.Lock()
-	defer s.viewMu.Unlock()
-
-	return s.updateViewLocked(ctx, view, options)
-}
-
-func (s *Session) updateViewLocked(ctx context.Context, view *View, options *source.Options) (*View, error) {
+func (s *Session) updateViewLocked(ctx context.Context, view *View, options *source.Options) error {
 	// Preserve the snapshot ID if we are recreating the view.
 	view.snapshotMu.Lock()
 	if view.snapshot == nil {
@@ -325,7 +302,7 @@ func (s *Session) updateViewLocked(ctx context.Context, view *View, options *sou
 
 	i := s.dropView(view)
 	if i == -1 {
-		return nil, fmt.Errorf("view %q not found", view.id)
+		return fmt.Errorf("view %q not found", view.id)
 	}
 
 	v, snapshot, release, err := s.createView(ctx, view.name, view.folder, options, seqID)
@@ -334,7 +311,7 @@ func (s *Session) updateViewLocked(ctx context.Context, view *View, options *sou
 		// this should not happen and is very bad, but we still need to clean
 		// up the view array if it happens
 		s.views = removeElement(s.views, i)
-		return nil, err
+		return err
 	}
 	defer release()
 
@@ -350,7 +327,7 @@ func (s *Session) updateViewLocked(ctx context.Context, view *View, options *sou
 
 	// substitute the new view into the array where the old view was
 	s.views[i] = v
-	return v, nil
+	return nil
 }
 
 // removeElement removes the ith element from the slice replacing it with the last element.
@@ -457,8 +434,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
 			}
 
 			if info != view.workspaceInformation {
-				_, err := s.updateViewLocked(ctx, view, view.Options())
-				if err != nil {
+				if err := s.updateViewLocked(ctx, view, view.Options()); err != nil {
 					// More catastrophic failure. The view may or may not still exist.
 					// The best we can do is log and move on.
 					event.Error(ctx, "recreating view", err)

gopls/internal/lsp/cache/view.go

@@ -482,21 +482,35 @@ func minorOptionsChange(a, b *source.Options) bool {
 	return reflect.DeepEqual(aBuildFlags, bBuildFlags)
 }
 
-// SetViewOptions sets the options of the given view to new values. Calling
-// this may cause the view to be invalidated and a replacement view added to
-// the session. If so the new view will be returned, otherwise the original one
-// will be returned.
-func (s *Session) SetViewOptions(ctx context.Context, v *View, options *source.Options) (*View, error) {
+// SetFolderOptions updates the options of each View associated with the folder
+// of the given URI.
+//
+// Calling this may cause each related view to be invalidated and a replacement
+// view added to the session.
+func (s *Session) SetFolderOptions(ctx context.Context, uri span.URI, options *source.Options) error {
+	s.viewMu.Lock()
+	defer s.viewMu.Unlock()
+
+	for _, v := range s.views {
+		if v.folder == uri {
+			if err := s.setViewOptions(ctx, v, options); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+func (s *Session) setViewOptions(ctx context.Context, v *View, options *source.Options) error {
 	// no need to rebuild the view if the options were not materially changed
 	v.optionsMu.Lock()
 	if minorOptionsChange(v.options, options) {
 		v.options = options
 		v.optionsMu.Unlock()
-		return v, nil
+		return nil
 	}
 	v.optionsMu.Unlock()
-	newView, err := s.updateView(ctx, v, options)
-	return newView, err
+	return s.updateViewLocked(ctx, v, options)
 }
 
 // viewEnv returns a string describing the environment of a newly created view.

gopls/internal/lsp/cmd/capabilities_test.go

@@ -49,7 +49,7 @@ func TestCapabilities(t *testing.T) {
 	// Send an initialize request to the server.
 	ctx := context.Background()
 	client := newClient(app, nil)
-	server := lsp.NewServer(cache.NewSession(ctx, cache.New(nil), app.options), client)
+	server := lsp.NewServer(cache.NewSession(ctx, cache.New(nil)), client, app.options)
 	result, err := server.Initialize(ctx, params)
 	if err != nil {
 		t.Fatal(err)

gopls/internal/lsp/cmd/cmd.go

@@ -316,7 +316,7 @@ func (app *Application) connect(ctx context.Context, onProgress func(*protocol.P
 	switch {
 	case app.Remote == "":
 		client := newClient(app, onProgress)
-		server := lsp.NewServer(cache.NewSession(ctx, cache.New(nil), app.options), client)
+		server := lsp.NewServer(cache.NewSession(ctx, cache.New(nil)), client, app.options)
 		conn := newConnection(server, client)
 		if err := conn.initialize(protocol.WithClient(ctx, client), app.options); err != nil {
 			return nil, err

gopls/internal/lsp/code_action.go

@@ -299,7 +299,7 @@ func (s *Server) findMatchingDiagnostics(uri span.URI, pd protocol.Diagnostic) [
 
 func (s *Server) getSupportedCodeActions() []protocol.CodeActionKind {
 	allCodeActionKinds := make(map[protocol.CodeActionKind]struct{})
-	for _, kinds := range s.session.Options().SupportedCodeActions {
+	for _, kinds := range s.Options().SupportedCodeActions {
 		for kind := range kinds {
 			allCodeActionKinds[kind] = struct{}{}
 		}

gopls/internal/lsp/command.go

@@ -43,7 +43,7 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom
 	defer done()
 
 	var found bool
-	for _, name := range s.session.Options().SupportedCommands {
+	for _, name := range s.Options().SupportedCommands {
 		if name == params.Command {
 			found = true
 			break

gopls/internal/lsp/completion_test.go

@@ -144,20 +144,8 @@ func expected(t *testing.T, test tests.Completion, items tests.CompletionItems)
 
 func (r *runner) callCompletion(t *testing.T, src span.Span, options func(*source.Options)) []protocol.CompletionItem {
 	t.Helper()
-
-	view, err := r.server.session.ViewOf(src.URI())
-	if err != nil {
-		t.Fatal(err)
-	}
-	original := view.Options()
-	modified := view.Options().Clone()
-	options(modified)
-	view, err = r.server.session.SetViewOptions(r.ctx, view, modified)
-	if err != nil {
-		t.Error(err)
-		return nil
-	}
-	defer r.server.session.SetViewOptions(r.ctx, view, original)
+	cleanup := r.toggleOptions(t, src.URI(), options)
+	defer cleanup()
 
 	list, err := r.server.Completion(r.ctx, &protocol.CompletionParams{
 		TextDocumentPositionParams: protocol.TextDocumentPositionParams{
@@ -175,3 +163,21 @@ func (r *runner) callCompletion(t *testing.T, src span.Span, options func(*sourc
 	}
 	return list.Items
 }
+
+func (r *runner) toggleOptions(t *testing.T, uri span.URI, options func(*source.Options)) (reset func()) {
+	view, err := r.server.session.ViewOf(uri)
+	if err != nil {
+		t.Fatal(err)
+	}
+	folder := view.Folder()
+
+	original := view.Options()
+	modified := view.Options().Clone()
+	options(modified)
+	if err = r.server.session.SetFolderOptions(r.ctx, folder, modified); err != nil {
+		t.Fatal(err)
+	}
+	return func() {
+		r.server.session.SetFolderOptions(r.ctx, folder, original)
+	}
+}

gopls/internal/lsp/debug/serve.go

@@ -701,9 +701,10 @@ Unknown page
 		}
 		return s
 	},
-	"options": func(s *cache.Session) []sessionOption {
-		return showOptions(s.Options())
-	},
+	// TODO(rfindley): re-enable option inspection.
+	// "options": func(s *cache.Session) []sessionOption {
+	// 	return showOptions(s.Options())
+	// },
 })
 
 var MainTmpl = template.Must(template.Must(BaseTemplate.Clone()).Parse(`
@@ -837,12 +838,6 @@ From: <b>{{template "cachelink" .Cache.ID}}</b><br>
 <li>
 <a href="/file/{{$session.ID}}/{{.FileIdentity.Hash}}">{{.FileIdentity.URI}}</a>
 </li>{{end}}</ul>
-<h2>Options</h2>
-{{range options .}}
-<p><b>{{.Name}}</b> {{.Type}}</p>
-<p><i>default:</i> {{.Default}}</p>
-{{if ne .Default .Current}}<p><i>current:</i> {{.Current}}</p>{{end}}
-{{end}}
 {{end}}
 `))
 

gopls/internal/lsp/general.go

@@ -57,8 +57,10 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitializ
 	}
 	s.progress.SetSupportsWorkDoneProgress(params.Capabilities.Window.WorkDoneProgress)
 
-	options := s.session.Options()
-	defer func() { s.session.SetOptions(options) }()
+	options := s.Options().Clone()
+	// TODO(rfindley): remove the error return from handleOptionResults, and
+	// eliminate this defer.
+	defer func() { s.SetOptions(options) }()
 
 	if err := s.handleOptionResults(ctx, source.SetOptions(options, params.InitializationOptions)); err != nil {
 		return nil, err
@@ -170,8 +172,8 @@ See https://github.com/golang/go/issues/45732 for more information.`,
 				Range: &protocol.Or_SemanticTokensOptions_range{Value: true},
 				Full:  &protocol.Or_SemanticTokensOptions_full{Value: true},
 				Legend: protocol.SemanticTokensLegend{
-					TokenTypes:     nonNilSliceString(s.session.Options().SemanticTypes),
-					TokenModifiers: nonNilSliceString(s.session.Options().SemanticMods),
+					TokenTypes:     nonNilSliceString(s.Options().SemanticTypes),
+					TokenModifiers: nonNilSliceString(s.Options().SemanticMods),
 				},
 			},
 			SignatureHelpProvider: &protocol.SignatureHelpOptions{
@@ -215,9 +217,7 @@ func (s *Server) initialized(ctx context.Context, params *protocol.InitializedPa
 	}
 	s.notifications = nil
 
-	options := s.session.Options()
-	defer func() { s.session.SetOptions(options) }()
-
+	options := s.Options()
 	if err := s.addFolders(ctx, s.pendingFolders); err != nil {
 		return err
 	}
@@ -348,7 +348,7 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
 	viewErrors := make(map[span.URI]error)
 
 	var ndiagnose sync.WaitGroup // number of unfinished diagnose calls
-	if s.session.Options().VerboseWorkDoneProgress {
+	if s.Options().VerboseWorkDoneProgress {
 		work := s.progress.Start(ctx, DiagnosticWorkTitle(FromInitialWorkspaceLoad), "Calculating diagnostics for initial workspace load...", nil, nil)
 		defer func() {
 			go func() {
@@ -475,7 +475,7 @@ func equalURISet(m1, m2 map[string]struct{}) bool {
 // registrations to the client and updates s.watchedDirectories.
 // The caller must not subsequently mutate patterns.
 func (s *Server) registerWatchedDirectoriesLocked(ctx context.Context, patterns map[string]struct{}) error {
-	if !s.session.Options().DynamicWatchedFilesSupported {
+	if !s.Options().DynamicWatchedFilesSupported {
 		return nil
 	}
 	s.watchedGlobPatterns = patterns
@@ -503,9 +503,27 @@ func (s *Server) registerWatchedDirectoriesLocked(ctx context.Context, patterns
 	return nil
 }
 
-func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI, o *source.Options) error {
-	if !s.session.Options().ConfigurationSupported {
-		return nil
+// Options returns the current server options.
+//
+// The caller must not modify the result.
+func (s *Server) Options() *source.Options {
+	s.optionsMu.Lock()
+	defer s.optionsMu.Unlock()
+	return s.options
+}
+
+// SetOptions sets the current server options.
+//
+// The caller must not subsequently modify the options.
+func (s *Server) SetOptions(opts *source.Options) {
+	s.optionsMu.Lock()
+	defer s.optionsMu.Unlock()
+	s.options = opts
+}
+
+func (s *Server) fetchFolderOptions(ctx context.Context, folder span.URI) (*source.Options, error) {
+	if opts := s.Options(); !opts.ConfigurationSupported {
+		return opts, nil
 	}
 	configs, err := s.client.Configuration(ctx, &protocol.ParamConfiguration{
 		Items: []protocol.ConfigurationItem{{
@@ -515,14 +533,16 @@ func (s *Server) fetchConfig(ctx context.Context, name string, folder span.URI,
 	},
 	)
 	if err != nil {
-		return fmt.Errorf("failed to get workspace configuration from client (%s): %v", folder, err)
+		return nil, fmt.Errorf("failed to get workspace configuration from client (%s): %v", folder, err)
 	}
+
+	folderOpts := s.Options().Clone()
 	for _, config := range configs {
-		if err := s.handleOptionResults(ctx, source.SetOptions(o, config)); err != nil {
-			return err
+		if err := s.handleOptionResults(ctx, source.SetOptions(folderOpts, config)); err != nil {
+			return nil, err
 		}
 	}
-	return nil
+	return folderOpts, nil
 }
 
 func (s *Server) eventuallyShowMessage(ctx context.Context, msg *protocol.ShowMessageParams) error {