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")), + ) }) }