From 0a9721c3dd4248e8ead3c3e0912c175480975dac Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 6 Sep 2023 13:49:15 -0400 Subject: [PATCH] gopls/internal/lsp: move options into the snapshot Snapshots should be idempotent, and the fact that configuration changes do not cause snapshots to increment has been a long-standing source of bugs. After a fair bit of setup, this CL finally moves options onto the snapshot. The only remaining use of the options stored on the View is for "minorOptionsChange" detection. This is of questionable value, but I opted not to delete it in this CL (Chesterton's fence). There is still more cleanup to do (and tests to update), but that is deferred to later CLs. Fixes golang/go#61325 Fixes golang/go#42814 Change-Id: If82dc199af13ddaa3464b2a67a8bab2013161f26 Reviewed-on: https://go-review.googlesource.com/c/tools/+/526159 LUCI-TryBot-Result: Go LUCI gopls-CI: kokoro Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/analysis.go | 4 +- gopls/internal/lsp/cache/check.go | 4 +- gopls/internal/lsp/cache/imports.go | 14 ++- gopls/internal/lsp/cache/load.go | 12 +-- gopls/internal/lsp/cache/session.go | 17 +++- gopls/internal/lsp/cache/snapshot.go | 73 +++++++++----- gopls/internal/lsp/cache/view.go | 101 ++++++++------------ gopls/internal/lsp/cmd/capabilities_test.go | 4 +- gopls/internal/lsp/cmd/cmd.go | 13 +-- gopls/internal/lsp/cmd/info.go | 3 +- gopls/internal/lsp/completion_test.go | 5 +- gopls/internal/lsp/debug/serve.go | 2 - gopls/internal/lsp/general.go | 2 +- gopls/internal/lsp/lsp_test.go | 5 +- gopls/internal/lsp/lsprpc/lsprpc.go | 6 +- gopls/internal/lsp/server.go | 6 +- gopls/internal/lsp/source/options.go | 10 +- 17 files changed, 145 insertions(+), 136 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 5676a7814a2..dfb09951494 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/gopls/internal/lsp/cache/analysis.go @@ -192,7 +192,7 @@ func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit, toSrc := make(map[*analysis.Analyzer]*source.Analyzer) var enabled []*analysis.Analyzer // enabled subset + transitive requirements for _, a := range analyzers { - if a.IsEnabled(snapshot.view.Options()) { + if a.IsEnabled(snapshot.options) { toSrc[a.Analyzer] = a enabled = append(enabled, a.Analyzer) } @@ -309,7 +309,7 @@ func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit, // Now that we have read all files, // we no longer need the snapshot. // (but options are needed for progress reporting) - options := snapshot.view.Options() + options := snapshot.options snapshot = nil // Progress reporting. If supported, gopls reports progress on analysis diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index b7267e983ec..438da1604e6 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -1345,8 +1345,8 @@ func (s *snapshot) typeCheckInputs(ctx context.Context, m *source.Metadata) (typ depsByImpPath: m.DepsByImpPath, goVersion: goVersion, - relatedInformation: s.view.Options().RelatedInformationSupported, - linkTarget: s.view.Options().LinkTarget, + relatedInformation: s.options.RelatedInformationSupported, + linkTarget: s.options.LinkTarget, moduleMode: s.view.moduleMode(), }, nil } diff --git a/gopls/internal/lsp/cache/imports.go b/gopls/internal/lsp/cache/imports.go index f22d42c21a8..028607608cc 100644 --- a/gopls/internal/lsp/cache/imports.go +++ b/gopls/internal/lsp/cache/imports.go @@ -54,15 +54,13 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot // view.goEnv is immutable -- changes make a new view. Options can change. // We can't compare build flags directly because we may add -modfile. - snapshot.view.optionsMu.Lock() - localPrefix := snapshot.view.options.Local - currentBuildFlags := snapshot.view.options.BuildFlags - currentDirectoryFilters := snapshot.view.options.DirectoryFilters + localPrefix := snapshot.options.Local + currentBuildFlags := snapshot.options.BuildFlags + currentDirectoryFilters := snapshot.options.DirectoryFilters changed := !reflect.DeepEqual(currentBuildFlags, s.cachedBuildFlags) || - snapshot.view.options.VerboseOutput != (s.processEnv.Logf != nil) || + snapshot.options.VerboseOutput != (s.processEnv.Logf != nil) || modFileHash != s.cachedModFileHash || - !reflect.DeepEqual(snapshot.view.options.DirectoryFilters, s.cachedDirectoryFilters) - snapshot.view.optionsMu.Unlock() + !reflect.DeepEqual(snapshot.options.DirectoryFilters, s.cachedDirectoryFilters) // If anything relevant to imports has changed, clear caches and // update the processEnv. Clearing caches blocks on any background @@ -120,7 +118,7 @@ func populateProcessEnvFromSnapshot(ctx context.Context, pe *imports.ProcessEnv, ctx, done := event.Start(ctx, "cache.populateProcessEnvFromSnapshot") defer done() - if snapshot.view.Options().VerboseOutput { + if snapshot.options.VerboseOutput { pe.Logf = func(format string, args ...interface{}) { event.Log(ctx, fmt.Sprintf(format, args...)) } diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index df68ba7429d..331e0e7ebc7 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -75,7 +75,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc if err != nil { continue } - if isStandaloneFile(contents, s.view.Options().StandaloneTags) { + if isStandaloneFile(contents, s.options.StandaloneTags) { standalone = true query = append(query, uri.Filename()) } else { @@ -160,7 +160,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc } moduleErrs := make(map[string][]packages.Error) // module path -> errors - filterFunc := s.view.filterFunc() + filterFunc := s.filterFunc() newMetadata := make(map[PackageID]*source.Metadata) for _, pkg := range pkgs { // The Go command returns synthetic list results for module queries that @@ -178,7 +178,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc continue } - if !containsDir || s.view.Options().VerboseOutput { + if !containsDir || s.options.VerboseOutput { event.Log(ctx, eventName, append( source.SnapshotLabels(s), tag.Package.Of(pkg.ID), @@ -359,7 +359,7 @@ func (s *snapshot) applyCriticalErrorToFiles(ctx context.Context, msg string, fi for _, fh := range files { // Place the diagnostics on the package or module declarations. var rng protocol.Range - switch s.view.FileKind(fh) { + switch s.FileKind(fh) { case source.Go: if pgf, err := s.ParseGo(ctx, fh, source.ParseHeader); err == nil { // Check that we have a valid `package foo` range to use for positioning the error. @@ -616,7 +616,7 @@ func containsPackageLocked(s *snapshot, m *source.Metadata) bool { uris[uri] = struct{}{} } - filterFunc := s.view.filterFunc() + filterFunc := s.filterFunc() for uri := range uris { // Don't use view.contains here. go.work files may include modules // outside of the workspace folder. @@ -671,7 +671,7 @@ func containsFileInWorkspaceLocked(s *snapshot, m *source.Metadata) bool { // The package's files are in this view. It may be a workspace package. // Vendored packages are not likely to be interesting to the user. - if !strings.Contains(string(uri), "/vendor/") && s.view.contains(uri) { + if !strings.Contains(string(uri), "/vendor/") && s.contains(uri) { return true } } diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index edfb78ec979..e2869dd6e29 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -117,9 +117,9 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, v := &View{ id: strconv.FormatInt(index, 10), gocmdRunner: s.gocmdRunner, + lastOptions: options, initialWorkspaceLoad: make(chan struct{}), initializationSema: make(chan struct{}, 1), - options: options, baseCtx: baseCtx, name: name, moduleUpgrades: map[span.URI]map[string]string{}, @@ -167,6 +167,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, workspaceModFiles: wsModFiles, workspaceModFilesErr: wsModFilesErr, pkgIndex: typerefs.NewPackageIndex(), + options: options, } // Save one reference in the view. v.releaseSnapshot = v.snapshot.Acquire() @@ -255,9 +256,15 @@ func bestViewForURI(uri span.URI, views []*View) *View { } // TODO(rfindley): this should consider the workspace layout (i.e. // go.work). - if view.contains(uri) { + snapshot, release, err := view.getSnapshot() + if err != nil { + // view is shutdown + continue + } + if snapshot.contains(uri) { longest = view } + release() } if longest != nil { return longest @@ -420,7 +427,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif // synchronously to change processing? Can we assume that the env did not // change, and derive go.work using a combination of the configured // GOWORK value and filesystem? - info, err := s.getWorkspaceInformation(ctx, view.folder, view.Options()) + info, err := s.getWorkspaceInformation(ctx, view.folder, view.lastOptions) if err != nil { // Catastrophic failure, equivalent to a failure of session // initialization and therefore should almost never happen. One @@ -434,7 +441,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif } if info != view.workspaceInformation { - if err := s.updateViewLocked(ctx, view, view.Options()); err != nil { + if err := s.updateViewLocked(ctx, view, view.lastOptions); 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) @@ -492,7 +499,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif var releases []func() viewToSnapshot := map[*View]*snapshot{} for view, changed := range views { - snapshot, release := view.invalidateContent(ctx, changed, forceReloadMetadata) + snapshot, release := view.invalidateContent(ctx, changed, nil, forceReloadMetadata) releases = append(releases, release) viewToSnapshot[view] = snapshot } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 4010c985747..9d659f046dc 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -187,6 +187,10 @@ type snapshot struct { // active packages. Running type checking batches in parallel after an // invalidation can cause redundant calculation of this shared state. typeCheckMu sync.Mutex + + // options holds the user configuration at the time this snapshot was + // created. + options *source.Options } var globalSnapshotID uint64 @@ -275,12 +279,39 @@ func (s *snapshot) View() source.View { return s.view } -func (s *snapshot) FileKind(h source.FileHandle) source.FileKind { - return s.view.FileKind(h) +func (s *snapshot) FileKind(fh source.FileHandle) source.FileKind { + // The kind of an unsaved buffer comes from the + // TextDocumentItem.LanguageID field in the didChange event, + // not from the file name. They may differ. + if o, ok := fh.(*Overlay); ok { + if o.kind != source.UnknownKind { + return o.kind + } + } + + fext := filepath.Ext(fh.URI().Filename()) + switch fext { + case ".go": + return source.Go + case ".mod": + return source.Mod + case ".sum": + return source.Sum + case ".work": + return source.Work + } + exts := s.options.TemplateExtensions + for _, ext := range exts { + if fext == ext || fext == "."+ext { + return source.Tmpl + } + } + // and now what? This should never happen, but it does for cgo before go1.15 + return source.Go } func (s *snapshot) Options() *source.Options { - return s.view.Options() // temporarily return view options. + return s.options // temporarily return view options. } func (s *snapshot) BackgroundContext() context.Context { @@ -306,7 +337,7 @@ func (s *snapshot) Templates() map[span.URI]source.FileHandle { tmpls := map[span.URI]source.FileHandle{} s.files.Range(func(k span.URI, fh source.FileHandle) { - if s.view.FileKind(fh) == source.Tmpl { + if s.FileKind(fh) == source.Tmpl { tmpls[k] = fh } }) @@ -354,8 +385,7 @@ func (s *snapshot) workspaceMode() workspaceMode { return mode } mode |= moduleMode - options := s.view.Options() - if options.TempModfile { + if s.options.TempModfile { mode |= tempModfile } return mode @@ -368,9 +398,6 @@ func (s *snapshot) workspaceMode() workspaceMode { // multiple modules in on config, so buildOverlay needs to filter overlays by // module. func (s *snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packages.Config { - s.view.optionsMu.Lock() - verboseOutput := s.view.options.VerboseOutput - s.view.optionsMu.Unlock() cfg := &packages.Config{ Context: ctx, @@ -393,7 +420,7 @@ func (s *snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packa panic("go/packages must not be used to parse files") }, Logf: func(format string, args ...interface{}) { - if verboseOutput { + if s.options.VerboseOutput { event.Log(ctx, fmt.Sprintf(format, args...)) } }, @@ -475,18 +502,16 @@ func (s *snapshot) RunGoCommands(ctx context.Context, allowNetwork bool, wd stri // it used only after call to tempModFile. Clarify that it is only // non-nil on success. func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.InvocationFlags, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) { - s.view.optionsMu.Lock() - allowModfileModificationOption := s.view.options.AllowModfileModifications - allowNetworkOption := s.view.options.AllowImplicitNetworkAccess + allowModfileModificationOption := s.options.AllowModfileModifications + allowNetworkOption := s.options.AllowImplicitNetworkAccess // TODO(rfindley): this is very hard to follow, and may not even be doing the // right thing: should inv.Env really trample view.options? Do we ever invoke // this with a non-empty inv.Env? // // We should refactor to make it clearer that the correct env is being used. - inv.Env = append(append(append(os.Environ(), s.view.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.GO111MODULE()) - inv.BuildFlags = append([]string{}, s.view.options.BuildFlags...) - s.view.optionsMu.Unlock() + inv.Env = append(append(append(os.Environ(), s.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.GO111MODULE()) + inv.BuildFlags = append([]string{}, s.options.BuildFlags...) cleanup = func() {} // fallback // All logic below is for module mode. @@ -993,8 +1018,7 @@ func (s *snapshot) workspaceDirs(ctx context.Context) []string { // Code) that do not send notifications for individual files in a directory // when the entire directory is deleted. func (s *snapshot) watchSubdirs() bool { - opts := s.view.Options() - switch p := opts.SubdirWatchPatterns; p { + switch p := s.options.SubdirWatchPatterns; p { case source.SubdirWatchPatternsOn: return true case source.SubdirWatchPatternsOff: @@ -1007,7 +1031,7 @@ func (s *snapshot) watchSubdirs() bool { // requirements that client names do not change. We should update the VS // Code extension to set a default value of "subdirWatchPatterns" to "on", // so that this workaround is only temporary. - if opts.ClientInfo != nil && opts.ClientInfo.Name == "Visual Studio Code" { + if s.options.ClientInfo != nil && s.options.ClientInfo.Name == "Visual Studio Code" { return true } return false @@ -1505,7 +1529,7 @@ func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error { var files []*Overlay for _, o := range open { uri := o.URI() - if s.IsBuiltin(uri) || s.view.FileKind(o) != source.Go { + if s.IsBuiltin(uri) || s.FileKind(o) != source.Go { continue } if len(meta.ids[uri]) == 0 { @@ -1606,7 +1630,7 @@ func (s *snapshot) OrphanedFileDiagnostics(ctx context.Context) (map[span.URI]*s searchOverlays: for _, o := range s.overlays() { uri := o.URI() - if s.IsBuiltin(uri) || s.view.FileKind(o) != source.Go { + if s.IsBuiltin(uri) || s.FileKind(o) != source.Go { continue } md, err := s.MetadataForFile(ctx, uri) @@ -1805,7 +1829,7 @@ func inVendor(uri span.URI) bool { return found && strings.Contains(after, "/") } -func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source.FileHandle, forceReloadMetadata bool) (*snapshot, func()) { +func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source.FileHandle, newOptions *source.Options, forceReloadMetadata bool) (*snapshot, func()) { ctx, done := event.Start(ctx, "cache.snapshot.clone") defer done() @@ -1838,6 +1862,11 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source workspaceModFilesErr: s.workspaceModFilesErr, importGraph: s.importGraph, pkgIndex: s.pkgIndex, + options: s.options, + } + + if newOptions != nil { + result.options = newOptions } // Create a lease on the new snapshot. diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index a71fc1d6acd..7c2eda1743b 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -46,8 +46,11 @@ type View struct { // name is the user-specified name of this view. name string - optionsMu sync.Mutex - options *source.Options + // lastOptions holds the most recent options on this view, used for detecting + // major changes. + // + // Guarded by Session.viewMu. + lastOptions *source.Options // Workspace information. The fields below are immutable, and together with // options define the build list. Any change to these fields results in a new @@ -412,48 +415,18 @@ func (v *View) Folder() span.URI { return v.folder } -func (v *View) Options() *source.Options { - v.optionsMu.Lock() - defer v.optionsMu.Unlock() - return v.options -} - -func (v *View) FileKind(fh source.FileHandle) source.FileKind { - // The kind of an unsaved buffer comes from the - // TextDocumentItem.LanguageID field in the didChange event, - // not from the file name. They may differ. - if o, ok := fh.(*Overlay); ok { - if o.kind != source.UnknownKind { - return o.kind - } - } - - fext := filepath.Ext(fh.URI().Filename()) - switch fext { - case ".go": - return source.Go - case ".mod": - return source.Mod - case ".sum": - return source.Sum - case ".work": - return source.Work - } - exts := v.Options().TemplateExtensions - for _, ext := range exts { - if fext == ext || fext == "."+ext { - return source.Tmpl - } - } - // and now what? This should never happen, but it does for cgo before go1.15 - return source.Go -} - func minorOptionsChange(a, b *source.Options) bool { // TODO(rfindley): this function detects whether a view should be recreated, // but this is also checked by the getWorkspaceInformation logic. // // We should eliminate this redundancy. + // + // Additionally, this function has existed for a long time, but git history + // suggests that it was added arbitrarily, not due to an actual performance + // problem. + // + // Especially now that we have optimized reinitialization of the session, we + // should consider just always creating a new view on any options change. // Check if any of the settings that modify our understanding of files have // been changed. @@ -503,13 +476,12 @@ func (s *Session) SetFolderOptions(ctx context.Context, uri span.URI, options *s 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() + if minorOptionsChange(v.lastOptions, options) { + _, release := v.invalidateContent(ctx, nil, options, false) + release() + v.lastOptions = options return nil } - v.optionsMu.Unlock() return s.updateViewLocked(ctx, v, options) } @@ -517,8 +489,8 @@ func (s *Session) setViewOptions(ctx context.Context, v *View, options *source.O // // It must not be called concurrently with any other view methods. func viewEnv(v *View) string { - env := v.options.EnvSlice() - buildFlags := append([]string{}, v.options.BuildFlags...) + env := v.snapshot.options.EnvSlice() + buildFlags := append([]string{}, v.snapshot.options.BuildFlags...) var buf bytes.Buffer fmt.Fprintf(&buf, `go info for %v @@ -567,13 +539,13 @@ func fileHasExtension(path string, suffixes []string) bool { // locateTemplateFiles ensures that the snapshot has mapped template files // within the workspace folder. func (s *snapshot) locateTemplateFiles(ctx context.Context) { - if len(s.view.Options().TemplateExtensions) == 0 { + if len(s.options.TemplateExtensions) == 0 { return } - suffixes := s.view.Options().TemplateExtensions + suffixes := s.options.TemplateExtensions searched := 0 - filterFunc := s.view.filterFunc() + filterFunc := s.filterFunc() err := filepath.WalkDir(s.view.folder.Filename(), func(path string, entry os.DirEntry, err error) error { if err != nil { return err @@ -607,33 +579,33 @@ func (s *snapshot) locateTemplateFiles(ctx context.Context) { } } -func (v *View) contains(uri span.URI) bool { +func (s *snapshot) contains(uri span.URI) bool { // If we've expanded the go dir to a parent directory, consider if the // expanded dir contains the uri. // TODO(rfindley): should we ignore the root here? It is not provided by the // user. It would be better to explicitly consider the set of active modules // wherever relevant. inGoDir := false - if source.InDir(v.goCommandDir.Filename(), v.folder.Filename()) { - inGoDir = source.InDir(v.goCommandDir.Filename(), uri.Filename()) + if source.InDir(s.view.goCommandDir.Filename(), s.view.folder.Filename()) { + inGoDir = source.InDir(s.view.goCommandDir.Filename(), uri.Filename()) } - inFolder := source.InDir(v.folder.Filename(), uri.Filename()) + inFolder := source.InDir(s.view.folder.Filename(), uri.Filename()) if !inGoDir && !inFolder { return false } - return !v.filterFunc()(uri) + return !s.filterFunc()(uri) } // filterFunc returns a func that reports whether uri is filtered by the currently configured // directoryFilters. -func (v *View) filterFunc() func(span.URI) bool { - filterer := buildFilterer(v.folder.Filename(), v.gomodcache, v.Options()) +func (s *snapshot) filterFunc() func(span.URI) bool { + filterer := buildFilterer(s.view.folder.Filename(), s.view.gomodcache, s.options) return func(uri span.URI) bool { // Only filter relative to the configured root directory. - if source.InDir(v.folder.Filename(), uri.Filename()) { - return pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), v.folder.Filename()), filterer) + if source.InDir(s.view.folder.Filename(), uri.Filename()) { + return pathExcludedByFilter(strings.TrimPrefix(uri.Filename(), s.view.folder.Filename()), filterer) } return false } @@ -660,7 +632,12 @@ func (v *View) relevantChange(c source.FileModification) bool { // had neither test nor associated issue, and cited only emacs behavior, this // logic was deleted. - return v.contains(c.URI) + snapshot, release, err := v.getSnapshot() + if err != nil { + return false // view was shut down + } + defer release() + return snapshot.contains(c.URI) } func (v *View) markKnown(uri span.URI) { @@ -938,7 +915,9 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr // // invalidateContent returns a non-nil snapshot for the new content, along with // a callback which the caller must invoke to release that snapshot. -func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]source.FileHandle, forceReloadMetadata bool) (*snapshot, func()) { +// +// newOptions may be nil, in which case options remain unchanged. +func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]source.FileHandle, newOptions *source.Options, forceReloadMetadata bool) (*snapshot, func()) { // Detach the context so that content invalidation cannot be canceled. ctx = xcontext.Detach(ctx) @@ -960,7 +939,7 @@ func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]sourc prevSnapshot.AwaitInitialized(ctx) // Save one lease of the cloned snapshot in the view. - v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, v.baseCtx, changes, forceReloadMetadata) + v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, v.baseCtx, changes, newOptions, forceReloadMetadata) prevReleaseSnapshot() v.destroy(prevSnapshot, "View.invalidateContent") diff --git a/gopls/internal/lsp/cmd/capabilities_test.go b/gopls/internal/lsp/cmd/capabilities_test.go index ddf56851937..02ff0d950f6 100644 --- a/gopls/internal/lsp/cmd/capabilities_test.go +++ b/gopls/internal/lsp/cmd/capabilities_test.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/gopls/internal/lsp" "golang.org/x/tools/gopls/internal/lsp/cache" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/internal/testenv" ) @@ -49,7 +50,8 @@ 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)), client, app.options) + options := source.DefaultOptions(app.options) + server := lsp.NewServer(cache.NewSession(ctx, cache.New(nil)), client, options) result, err := server.Initialize(ctx, params) if err != nil { t.Fatal(err) diff --git a/gopls/internal/lsp/cmd/cmd.go b/gopls/internal/lsp/cmd/cmd.go index 2e1e6229edd..9a9d627880a 100644 --- a/gopls/internal/lsp/cmd/cmd.go +++ b/gopls/internal/lsp/cmd/cmd.go @@ -316,7 +316,8 @@ 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)), client, app.options) + options := source.DefaultOptions(app.options) + server := lsp.NewServer(cache.NewSession(ctx, cache.New(nil)), client, options) conn := newConnection(server, client) if err := conn.initialize(protocol.WithClient(ctx, client), app.options); err != nil { return nil, err @@ -326,10 +327,7 @@ func (app *Application) connect(ctx context.Context, onProgress func(*protocol.P case strings.HasPrefix(app.Remote, "internal@"): internalMu.Lock() defer internalMu.Unlock() - opts := source.DefaultOptions().Clone() - if app.options != nil { - app.options(opts) - } + opts := source.DefaultOptions(app.options) key := fmt.Sprintf("%s %v %v %v", app.wd, opts.PreferredContentFormat, opts.HierarchicalDocumentSymbolSupport, opts.SymbolMatcher) if c := internalConnections[key]; c != nil { return c, nil @@ -376,10 +374,7 @@ func (c *connection) initialize(ctx context.Context, options func(*source.Option params.Capabilities.Workspace.Configuration = true // Make sure to respect configured options when sending initialize request. - opts := source.DefaultOptions().Clone() - if options != nil { - options(opts) - } + opts := source.DefaultOptions(options) // If you add an additional option here, you must update the map key in connect. params.Capabilities.TextDocument.Hover = &protocol.HoverClientCapabilities{ ContentFormat: []protocol.MarkupKind{opts.PreferredContentFormat}, diff --git a/gopls/internal/lsp/cmd/info.go b/gopls/internal/lsp/cmd/info.go index d63e24b6bfc..b0f08bbef67 100644 --- a/gopls/internal/lsp/cmd/info.go +++ b/gopls/internal/lsp/cmd/info.go @@ -299,8 +299,7 @@ gopls also includes software made available under these licenses: ` func (l *licenses) Run(ctx context.Context, args ...string) error { - opts := source.DefaultOptions() - l.app.options(opts) + opts := source.DefaultOptions(l.app.options) txt := licensePreamble if opts.LicensesText == "" { txt += "(development gopls, license information not available)" diff --git a/gopls/internal/lsp/completion_test.go b/gopls/internal/lsp/completion_test.go index 1da7ade563d..48ec9ea094e 100644 --- a/gopls/internal/lsp/completion_test.go +++ b/gopls/internal/lsp/completion_test.go @@ -171,13 +171,12 @@ func (r *runner) toggleOptions(t *testing.T, uri span.URI, options func(*source. } folder := view.Folder() - original := view.Options() - modified := view.Options().Clone() + modified := r.server.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) + r.server.session.SetFolderOptions(r.ctx, folder, r.server.Options()) } } diff --git a/gopls/internal/lsp/debug/serve.go b/gopls/internal/lsp/debug/serve.go index c0bfd9e7b50..8aa2938c228 100644 --- a/gopls/internal/lsp/debug/serve.go +++ b/gopls/internal/lsp/debug/serve.go @@ -846,8 +846,6 @@ var ViewTmpl = template.Must(template.Must(BaseTemplate.Clone()).Parse(` {{define "body"}} Name: {{.Name}}
Folder: {{.Folder}}
-

Environment

-
    {{range .Options.Env}}
  • {{.}}
    • {{end}}
{{end}} `)) diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go index e2f8ddbbf67..61c16c0531a 100644 --- a/gopls/internal/lsp/general.go +++ b/gopls/internal/lsp/general.go @@ -623,7 +623,7 @@ func (s *Server) beginFileRequest(ctx context.Context, pURI protocol.DocumentURI release() return nil, nil, false, func() {}, err } - if expectKind != source.UnknownKind && view.FileKind(fh) != expectKind { + if expectKind != source.UnknownKind && snapshot.FileKind(fh) != expectKind { // Wrong kind of file. Nothing to do. release() return nil, nil, false, func() {}, nil diff --git a/gopls/internal/lsp/lsp_test.go b/gopls/internal/lsp/lsp_test.go index 285f78a7f40..88f6a3940f1 100644 --- a/gopls/internal/lsp/lsp_test.go +++ b/gopls/internal/lsp/lsp_test.go @@ -52,8 +52,7 @@ func testLSP(t *testing.T, datum *tests.Data) { ctx = debug.WithInstance(ctx, "", "off") session := cache.NewSession(ctx, cache.New(nil)) - options := source.DefaultOptions().Clone() - tests.DefaultOptions(options) + options := source.DefaultOptions(tests.DefaultOptions) options.SetEnvSlice(datum.Config.Env) view, snapshot, release, err := session.NewView(ctx, datum.Config.Dir, span.URIFromPath(datum.Config.Dir), options) if err != nil { @@ -112,7 +111,7 @@ func testLSP(t *testing.T, datum *tests.Data) { editRecv: make(chan map[span.URI][]byte, 1), } - r.server = NewServer(session, testClient{runner: r}, nil) + r.server = NewServer(session, testClient{runner: r}, options) tests.Run(t, r, datum) } diff --git a/gopls/internal/lsp/lsprpc/lsprpc.go b/gopls/internal/lsp/lsprpc/lsprpc.go index ef2c3ed0c54..7dc709291fb 100644 --- a/gopls/internal/lsp/lsprpc/lsprpc.go +++ b/gopls/internal/lsp/lsprpc/lsprpc.go @@ -59,7 +59,8 @@ func (s *StreamServer) Binder() *ServerBinder { session := cache.NewSession(ctx, s.cache) server := s.serverForTest if server == nil { - server = lsp.NewServer(session, client, s.optionsOverrides) + options := source.DefaultOptions(s.optionsOverrides) + server = lsp.NewServer(session, client, options) if instance := debug.GetInstance(ctx); instance != nil { instance.AddService(server, session) } @@ -76,7 +77,8 @@ func (s *StreamServer) ServeStream(ctx context.Context, conn jsonrpc2.Conn) erro session := cache.NewSession(ctx, s.cache) server := s.serverForTest if server == nil { - server = lsp.NewServer(session, client, s.optionsOverrides) + options := source.DefaultOptions(s.optionsOverrides) + server = lsp.NewServer(session, client, options) if instance := debug.GetInstance(ctx); instance != nil { instance.AddService(server, session) } diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go index bddf86458ea..a236779962f 100644 --- a/gopls/internal/lsp/server.go +++ b/gopls/internal/lsp/server.go @@ -26,11 +26,7 @@ const concurrentAnalyses = 1 // NewServer creates an LSP server and binds it to handle incoming client // messages on the supplied stream. -func NewServer(session *cache.Session, client protocol.ClientCloser, optionsOverrides func(*source.Options)) *Server { - options := source.DefaultOptions().Clone() - if optionsOverrides != nil { - optionsOverrides(options) - } +func NewServer(session *cache.Session, client protocol.ClientCloser, options *source.Options) *Server { return &Server{ diagnostics: map[span.URI]*fileReports{}, gcOptimizationDetails: make(map[source.PackageID]struct{}), diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go index 2b91f834d6a..a0802361bf2 100644 --- a/gopls/internal/lsp/source/options.go +++ b/gopls/internal/lsp/source/options.go @@ -81,7 +81,7 @@ var ( // DefaultOptions is the options that are used for Gopls execution independent // of any externally provided configuration (LSP initialization, command // invocation, etc.). -func DefaultOptions() *Options { +func DefaultOptions(overrides ...func(*Options)) *Options { optionsOnce.Do(func() { var commands []string for _, c := range command.Commands { @@ -189,7 +189,13 @@ func DefaultOptions() *Options { }, } }) - return defaultOptions + options := defaultOptions.Clone() + for _, override := range overrides { + if override != nil { + override(options) + } + } + return options } // Options holds various configuration that affects Gopls execution, organized