Revert "internal/lsp: move initialization entirely into the snapshot"

This reverts commit deb1282f04.

Reason for revert: Merge conflicts for Rob's CL stack

Change-Id: I166b3bd60ec2b727a79a090049f0764864656d47
Reviewed-on: https://go-review.googlesource.com/c/tools/+/266699
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>

internal/lsp/cache/load.go

@@ -91,6 +91,8 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error {
 	ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query))
 	defer done()
 
+	cleanup := func() {}
+
 	_, inv, cleanup, err := s.goCommandInvocation(ctx, source.ForTypeChecking, &gocommand.Invocation{
 		WorkingDir: s.view.rootURI.Filename(),
 	})

internal/lsp/cache/session.go

@@ -186,6 +186,9 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 
 	v := &View{
 		session:              s,
+		initialized:          make(chan struct{}),
+		initializationSema:   make(chan struct{}, 1),
+		initializeOnce:       &sync.Once{},
 		id:                   strconv.FormatInt(index, 10),
 		options:              options,
 		baseCtx:              baseCtx,
@@ -206,26 +209,23 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 		},
 	}
 	v.snapshot = &snapshot{
-		id:                 snapshotID,
-		view:               v,
-		initialized:        make(chan struct{}),
-		initializationSema: make(chan struct{}, 1),
-		initializeOnce:     &sync.Once{},
-		generation:         s.cache.store.Generation(generationName(v, 0)),
-		packages:           make(map[packageKey]*packageHandle),
-		ids:                make(map[span.URI][]packageID),
-		metadata:           make(map[packageID]*metadata),
-		files:              make(map[span.URI]source.VersionedFileHandle),
-		goFiles:            make(map[parseKey]*parseGoHandle),
-		importedBy:         make(map[packageID][]packageID),
-		actions:            make(map[actionKey]*actionHandle),
-		workspacePackages:  make(map[packageID]packagePath),
-		unloadableFiles:    make(map[span.URI]struct{}),
-		parseModHandles:    make(map[span.URI]*parseModHandle),
-		modTidyHandles:     make(map[span.URI]*modTidyHandle),
-		modUpgradeHandles:  make(map[span.URI]*modUpgradeHandle),
-		modWhyHandles:      make(map[span.URI]*modWhyHandle),
-		modules:            modules,
+		id:                snapshotID,
+		view:              v,
+		generation:        s.cache.store.Generation(generationName(v, 0)),
+		packages:          make(map[packageKey]*packageHandle),
+		ids:               make(map[span.URI][]packageID),
+		metadata:          make(map[packageID]*metadata),
+		files:             make(map[span.URI]source.VersionedFileHandle),
+		goFiles:           make(map[parseKey]*parseGoHandle),
+		importedBy:        make(map[packageID][]packageID),
+		actions:           make(map[actionKey]*actionHandle),
+		workspacePackages: make(map[packageID]packagePath),
+		unloadableFiles:   make(map[span.URI]struct{}),
+		parseModHandles:   make(map[span.URI]*parseModHandle),
+		modTidyHandles:    make(map[span.URI]*modTidyHandle),
+		modUpgradeHandles: make(map[span.URI]*modUpgradeHandle),
+		modWhyHandles:     make(map[span.URI]*modWhyHandle),
+		modules:           modules,
 	}
 
 	v.snapshot.workspaceDirectories = v.snapshot.findWorkspaceDirectories(ctx)

internal/lsp/cache/snapshot.go

@@ -46,33 +46,6 @@ type snapshot struct {
 	// builtin pins the AST and package for builtin.go in memory.
 	builtin *builtinPackageHandle
 
-	// The snapshot's initialization state is controlled by the fields below.
-	// These fields are propagated across snapshots to avoid multiple
-	// concurrent initializations. They may be invalidated during cloning.
-	//
-	// initialized is closed when the snapshot has been fully initialized. On
-	// initialization, the snapshot's workspace packages are loaded. All of the
-	// fields below are set as part of initialization. If we failed to load, we
-	// only retry if the go.mod file changes, to avoid too many go/packages
-	// calls.
-	//
-	// When the view is created, its snapshot's initializeOnce is non-nil,
-	// initialized is open. Once initialization completes, initializedErr may
-	// be set and initializeOnce becomes nil. If initializedErr is non-nil,
-	// initialization may be retried (depending on how files are changed). To
-	// indicate that initialization should be retried, initializeOnce will be
-	// set. The next time a caller requests workspace packages, the
-	// initialization will retry.
-	initialized chan struct{}
-
-	// initializationSema is used as a mutex to guard initializeOnce and
-	// initializedErr, which will be updated after each attempt to initialize
-	// the snapshot. We use a channel instead of a mutex to avoid blocking when
-	// a context is canceled.
-	initializationSema chan struct{}
-	initializeOnce     *sync.Once
-	initializedErr     error
-
 	// mu guards all of the maps in the snapshot.
 	mu sync.Mutex
 
@@ -893,7 +866,7 @@ func (s *snapshot) awaitLoaded(ctx context.Context) error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	if len(s.metadata) == 0 {
-		return s.initializedErr
+		return s.view.initializedErr
 	}
 	return nil
 }
@@ -902,7 +875,7 @@ func (s *snapshot) AwaitInitialized(ctx context.Context) {
 	select {
 	case <-ctx.Done():
 		return
-	case <-s.initialized:
+	case <-s.view.initialized:
 	}
 	// We typically prefer to run something as intensive as the IWL without
 	// blocking. I'm not sure if there is a way to do that here.
@@ -1019,7 +992,7 @@ func generationName(v *View, snapshotID uint64) string {
 	return fmt.Sprintf("v%v/%v", v.id, snapshotID)
 }
 
-func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.VersionedFileHandle, forceReloadMetadata bool) *snapshot {
+func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.VersionedFileHandle, forceReloadMetadata bool) (*snapshot, reinitializeView) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -1029,10 +1002,6 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve
 		generation:            newGen,
 		view:                  s.view,
 		builtin:               s.builtin,
-		initialized:           s.initialized,
-		initializationSema:    s.initializationSema,
-		initializeOnce:        s.initializeOnce,
-		initializedErr:        s.initializedErr,
 		ids:                   make(map[span.URI][]packageID),
 		importedBy:            make(map[packageID][]packageID),
 		metadata:              make(map[packageID]*metadata),
@@ -1329,17 +1298,20 @@ copyIDs:
 	// Don't bother copying the importedBy graph,
 	// as it changes each time we update metadata.
 
+	var reinitialize reinitializeView
+	if modulesChanged {
+		reinitialize = maybeReinit
+	}
+	if shouldReinitializeView {
+		reinitialize = definitelyReinit
+	}
+
 	// If the snapshot's workspace mode has changed, the packages loaded using
 	// the previous mode are no longer relevant, so clear them out.
 	if s.workspaceMode() != result.workspaceMode() {
 		result.workspacePackages = map[packageID]packagePath{}
 	}
-
-	// The snapshot may need to be reinitialized.
-	if modulesChanged || shouldReinitializeView {
-		result.reinitialize(shouldReinitializeView)
-	}
-	return result
+	return result, reinitialize
 }
 
 // guessPackagesForURI returns all packages related to uri. If we haven't seen this
@@ -1392,6 +1364,14 @@ func guessPackagesForURI(uri span.URI, known map[span.URI][]packageID) []package
 	return found
 }
 
+type reinitializeView int
+
+const (
+	doNotReinit = reinitializeView(iota)
+	maybeReinit
+	definitelyReinit
+)
+
 // fileWasSaved reports whether the FileHandle passed in has been saved. It
 // accomplishes this by checking to see if the original and current FileHandles
 // are both overlays, and if the current FileHandle is saved while the original

internal/lsp/cache/view.go

@@ -68,13 +68,34 @@ type View struct {
 	filesByURI  map[span.URI]*fileBase
 	filesByBase map[string][]*fileBase
 
-	// initCancelFirstAttempt can be used to terminate the view's first
-	// attempt at initialization.
-	initCancelFirstAttempt context.CancelFunc
-
 	snapshotMu sync.Mutex
 	snapshot   *snapshot
 
+	// initialized is closed when the view has been fully initialized. On
+	// initialization, the view's workspace packages are loaded. All of the
+	// fields below are set as part of initialization. If we failed to load, we
+	// only retry if the go.mod file changes, to avoid too many go/packages
+	// calls.
+	//
+	// When the view is created, initializeOnce is non-nil, initialized is
+	// open, and initCancelFirstAttempt can be used to terminate
+	// initialization. Once initialization completes, initializedErr may be set
+	// and initializeOnce becomes nil. If initializedErr is non-nil,
+	// initialization may be retried (depending on how files are changed). To
+	// indicate that initialization should be retried, initializeOnce will be
+	// set. The next time a caller requests workspace packages, the
+	// initialization will retry.
+	initialized            chan struct{}
+	initCancelFirstAttempt context.CancelFunc
+
+	// initializationSema is used as a mutex to guard initializeOnce and
+	// initializedErr, which will be updated after each attempt to initialize
+	// the view. We use a channel instead of a mutex to avoid blocking when a
+	// context is canceled.
+	initializationSema chan struct{}
+	initializeOnce     *sync.Once
+	initializedErr     error
+
 	// workspaceInformation tracks various details about this view's
 	// environment variables, go version, and use of modules.
 	workspaceInformation
@@ -474,21 +495,21 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
 	select {
 	case <-ctx.Done():
 		return
-	case s.initializationSema <- struct{}{}:
+	case s.view.initializationSema <- struct{}{}:
 	}
 
 	defer func() {
-		<-s.initializationSema
+		<-s.view.initializationSema
 	}()
 
-	if s.initializeOnce == nil {
+	if s.view.initializeOnce == nil {
 		return
 	}
-	s.initializeOnce.Do(func() {
+	s.view.initializeOnce.Do(func() {
 		defer func() {
-			s.initializeOnce = nil
+			s.view.initializeOnce = nil
 			if firstAttempt {
-				close(s.initialized)
+				close(s.view.initialized)
 			}
 		}()
 
@@ -536,9 +557,9 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
 		if err != nil {
 			event.Error(ctx, "initial workspace load failed", err)
 			if modErrors != nil {
-				s.initializedErr = errors.Errorf("errors loading modules: %v: %w", err, modErrors)
+				s.view.initializedErr = errors.Errorf("errors loading modules: %v: %w", err, modErrors)
 			} else {
-				s.initializedErr = err
+				s.view.initializedErr = err
 			}
 		}
 	})
@@ -563,9 +584,14 @@ func (v *View) invalidateContent(ctx context.Context, uris map[span.URI]source.V
 	defer v.snapshotMu.Unlock()
 
 	oldSnapshot := v.snapshot
-	v.snapshot = oldSnapshot.clone(ctx, uris, forceReloadMetadata)
+	var reinitialize reinitializeView
+	v.snapshot, reinitialize = oldSnapshot.clone(ctx, uris, forceReloadMetadata)
 	go oldSnapshot.generation.Destroy()
 
+	if reinitialize == maybeReinit || reinitialize == definitelyReinit {
+		v.reinitialize(reinitialize == definitelyReinit)
+	}
+
 	return v.snapshot, v.snapshot.generation.Acquire(ctx)
 }
 
@@ -580,17 +606,17 @@ func (v *View) cancelBackground() {
 	v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx)
 }
 
-func (s *snapshot) reinitialize(force bool) {
-	s.initializationSema <- struct{}{}
+func (v *View) reinitialize(force bool) {
+	v.initializationSema <- struct{}{}
 	defer func() {
-		<-s.initializationSema
+		<-v.initializationSema
 	}()
 
-	if !force && s.initializedErr == nil {
+	if !force && v.initializedErr == nil {
 		return
 	}
 	var once sync.Once
-	s.initializeOnce = &once
+	v.initializeOnce = &once
 }
 
 func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI, options *source.Options) (*workspaceInformation, error) {