From 8b89cfa70c18ad01bf86c5f69e3e2f8f4ed9c98e Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 3 Nov 2023 14:31:35 -0400 Subject: [PATCH] gopls/internal/lsp/cache: remove forceReloadMetadata from clone We need to simplify Clone for zero config gopls. One source of complexity is the 'forceReloadMetadata' parameter, which reaches inside the clone internals to force invalidation of metadata, and must be threaded through the call stack, originating from the 'InvalidateMetadata' action on a file. Instead, just recreate the View, which will have the same side effect of invoking `go list`. As a nice side-effect, we can actually use the FromRegenerateCgo diagnostic source, which allows us to avoid the unbounded Await in TestRegenerateCgo. For golang/go#57979 Change-Id: I7fa01ed17a9407dfb98844eeec69b1fdbbd27aa7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/539658 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/session.go | 44 ++++++++++--------- gopls/internal/lsp/cache/snapshot.go | 4 +- gopls/internal/lsp/cache/view.go | 6 +-- gopls/internal/lsp/command.go | 36 ++++++++++++--- gopls/internal/lsp/diagnostics.go | 9 +++- gopls/internal/lsp/source/view.go | 3 -- .../regtest/codelens/codelens_test.go | 6 ++- 7 files changed, 71 insertions(+), 37 deletions(-) diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index 925bacd824a..47b69af8bc5 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -287,28 +287,34 @@ func (s *Session) RemoveView(view *View) { // // If the resulting error is non-nil, the view may or may not have already been // dropped from the session. -func (s *Session) updateViewLocked(ctx context.Context, view *View, info *workspaceInformation, folder *Folder) error { +func (s *Session) updateViewLocked(ctx context.Context, view *View, info *workspaceInformation, folder *Folder) (*View, error) { // Preserve the snapshot ID if we are recreating the view. view.snapshotMu.Lock() if view.snapshot == nil { view.snapshotMu.Unlock() panic("updateView called after View was already shut down") } + // TODO(rfindley): we should probably increment the sequence ID here. seqID := view.snapshot.sequenceID // Preserve sequence IDs when updating a view in place. view.snapshotMu.Unlock() i := s.dropView(view) if i == -1 { - return fmt.Errorf("view %q not found", view.id) + return nil, fmt.Errorf("view %q not found", view.id) } - v, snapshot, release, err := s.createView(ctx, info, folder, seqID) + var ( + snapshot *snapshot + release func() + err error + ) + view, snapshot, release, err = s.createView(ctx, info, folder, seqID) if err != nil { // we have dropped the old view, but could not create the new one // 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 err + return nil, err } defer release() @@ -318,13 +324,13 @@ func (s *Session) updateViewLocked(ctx context.Context, view *View, info *worksp // behavior when configuration is changed mid-session. // // Ensure the new snapshot observes all open files. - for _, o := range v.fs.Overlays() { + for _, o := range view.fs.Overlays() { _, _ = snapshot.ReadFile(ctx, o.URI()) } // substitute the new view into the array where the old view was - s.views[i] = v - return nil + s.views[i] = view + return view, nil } // removeElement removes the ith element from the slice replacing it with the last element. @@ -362,6 +368,14 @@ func (s *Session) ModifyFiles(ctx context.Context, changes []source.FileModifica return err } +// ResetView resets the best view for the given URI. +func (s *Session) ResetView(ctx context.Context, uri span.URI) (*View, error) { + s.viewMu.Lock() + defer s.viewMu.Unlock() + v := bestViewForURI(uri, s.views) + return s.updateViewLocked(ctx, v, v.workspaceInformation, v.folder) +} + // DidModifyFiles reports a file modification to the session. It returns // the new snapshots after the modifications have been applied, paired with // the affected file URIs for those snapshots. @@ -401,7 +415,6 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif // Change, InvalidateMetadata, and UnknownFileAction actions do not cause // us to re-evaluate views. redoViews := (c.Action != source.Change && - c.Action != source.InvalidateMetadata && c.Action != source.UnknownFileAction) if redoViews { @@ -429,7 +442,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif // could report a bug, but it's not really a bug. event.Error(ctx, "fetching workspace information", err) } else if *info != *view.workspaceInformation { - if err := s.updateViewLocked(ctx, view, info, view.folder); err != nil { + if _, err := s.updateViewLocked(ctx, view, info, view.folder); 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) @@ -441,13 +454,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif // Collect information about views affected by these changes. views := make(map[*View]map[span.URI]source.FileHandle) affectedViews := map[span.URI][]*View{} - // forceReloadMetadata records whether any change is the magic - // source.InvalidateMetadata action. - forceReloadMetadata := false for _, c := range changes { - if c.Action == source.InvalidateMetadata { - forceReloadMetadata = true - } // Build the list of affected views. var changedViews []*View for _, view := range s.views { @@ -487,7 +494,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) releases = append(releases, release) viewToSnapshot[view] = snapshot } @@ -574,11 +581,6 @@ func (fs *overlayFS) updateOverlays(ctx context.Context, changes []source.FileMo defer fs.mu.Unlock() for _, c := range changes { - // Don't update overlays for metadata invalidations. - if c.Action == source.InvalidateMetadata { - continue - } - o, ok := fs.overlays[c.URI] // If the file is not opened in an overlay and the change is on disk, diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index d7f51dd9d21..ba1322d2ea6 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1813,7 +1813,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) (*snapshot, func()) { ctx, done := event.Start(ctx, "cache.snapshot.clone") defer done() @@ -1975,7 +1975,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source result.unloadableFiles.Remove(uri) } - invalidateMetadata = invalidateMetadata || forceReloadMetadata || reinit + invalidateMetadata = invalidateMetadata || reinit anyImportDeleted = anyImportDeleted || importDeleted // Mark all of the package IDs containing the given file. diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index b0157ee8b6c..2565151a4df 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -435,7 +435,7 @@ func (s *Session) SetFolderOptions(ctx context.Context, uri span.URI, options *s if err != nil { return err } - if err := s.updateViewLocked(ctx, v, info, &folder2); err != nil { + if _, err := s.updateViewLocked(ctx, v, info, &folder2); err != nil { return err } } @@ -871,7 +871,7 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr // a callback which the caller must invoke to release that snapshot. // // newOptions may be nil, in which case options remain unchanged. -func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]source.FileHandle, forceReloadMetadata bool) (*snapshot, func()) { +func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]source.FileHandle) (*snapshot, func()) { // Detach the context so that content invalidation cannot be canceled. ctx = xcontext.Detach(ctx) @@ -893,7 +893,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) prevReleaseSnapshot() v.destroy(prevSnapshot, "View.invalidateContent") diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go index 8fe1ec82391..da9d44e0c21 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -17,6 +17,7 @@ import ( "runtime/pprof" "sort" "strings" + "sync" "golang.org/x/mod/modfile" "golang.org/x/tools/go/ast/astutil" @@ -214,12 +215,37 @@ func (c *commandHandler) ApplyFix(ctx context.Context, args command.ApplyFixArgs func (c *commandHandler) RegenerateCgo(ctx context.Context, args command.URIArg) error { return c.run(ctx, commandConfig{ progress: "Regenerating Cgo", - }, func(ctx context.Context, deps commandDeps) error { - mod := source.FileModification{ - URI: args.URI.SpanURI(), - Action: source.InvalidateMetadata, + }, func(ctx context.Context, _ commandDeps) error { + var wg sync.WaitGroup // tracks work done on behalf of this function, incl. diagnostics + wg.Add(1) + defer wg.Done() + + // Track progress on this operation for testing. + if c.s.Options().VerboseWorkDoneProgress { + work := c.s.progress.Start(ctx, DiagnosticWorkTitle(FromRegenerateCgo), "Calculating file diagnostics...", nil, nil) + go func() { + wg.Wait() + work.End(ctx, "Done.") + }() + } + + // Resetting the view causes cgo to be regenerated via `go list`. + v, err := c.s.session.ResetView(ctx, args.URI.SpanURI()) + if err != nil { + return err + } + + snapshot, release, err := v.Snapshot() + if err != nil { + return err } - return c.s.didModifyFiles(ctx, []source.FileModification{mod}, FromRegenerateCgo) + wg.Add(1) + go func() { + c.s.diagnoseSnapshot(snapshot, nil, true, 0) + release() + wg.Done() + }() + return nil }) } diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go index c19788fbc54..4ea02cff9bf 100644 --- a/gopls/internal/lsp/diagnostics.go +++ b/gopls/internal/lsp/diagnostics.go @@ -178,6 +178,9 @@ func (s *Server) diagnoseSnapshots(snapshots map[source.Snapshot][]span.URI, onD // If changedURIs is non-empty, it is a set of recently changed files that // should be diagnosed immediately, and onDisk reports whether these file // changes came from a change to on-disk files. +// +// TODO(rfindley): eliminate the onDisk parameter, which looks misplaced. If we +// don't want to diagnose changes on disk, filter out the changedURIs. func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.URI, onDisk bool, delay time.Duration) { ctx := snapshot.BackgroundContext() ctx, done := event.Start(ctx, "Server.diagnoseSnapshot", source.SnapshotLabels(snapshot)...) @@ -203,8 +206,10 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.U return } - s.diagnoseChangedFiles(ctx, snapshot, changedURIs, onDisk) - s.publishDiagnostics(ctx, false, snapshot) + if len(changedURIs) > 0 { + s.diagnoseChangedFiles(ctx, snapshot, changedURIs, onDisk) + s.publishDiagnostics(ctx, false, snapshot) + } if delay < minDelay { delay = 0 diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 7851d130a6c..46c2eeeb609 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -738,7 +738,6 @@ const ( Save Create Delete - InvalidateMetadata ) func (a FileAction) String() string { @@ -755,8 +754,6 @@ func (a FileAction) String() string { return "Create" case Delete: return "Delete" - case InvalidateMetadata: - return "InvalidateMetadata" default: return "Unknown" } diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index b72e598c913..107db1a2c29 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/gopls/internal/regtest/codelens/codelens_test.go @@ -10,6 +10,7 @@ import ( "golang.org/x/tools/gopls/internal/bug" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/gopls/internal/lsp" . "golang.org/x/tools/gopls/internal/lsp/regtest" "golang.org/x/tools/gopls/internal/lsp/tests/compare" @@ -343,6 +344,9 @@ func Foo() { // Regenerate cgo, fixing the diagnostic. env.ExecuteCodeLensCommand("cgo.go", command.RegenerateCgo, nil) - env.Await(NoDiagnostics(ForFile("cgo.go"))) + env.OnceMet( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromRegenerateCgo), 1, true), + NoDiagnostics(ForFile("cgo.go")), + ) }) }