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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

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,

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.

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")

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
 	})
 }
 

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

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"
 	}

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