internal/lsp: recover from a view initialization failure

If an orphaned file is used to recover a workspace package, we should
remove the initialization error and treat the view as correctly
initialized.

Also, stop caching metadata for packages with no files. We have no way
to invalidate it, and it's useless, so just re-load those files as
needed.

Fixes golang/go#36795.
Fixes golang/go#36671.
Fixes golang/go#36772.

Change-Id: I0aee5a43401517b6073d27136cca533160effef2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216637
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

Author: stamblerre

internal/lsp/cache/load.go

@@ -149,14 +149,15 @@ func (s *snapshot) updateMetadata(ctx context.Context, scopes []interface{}, pkg
 			}
 		}
 		if !containsDir || s.view.Options().VerboseOutput {
-			log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
+			log.Print(ctx, "go/packages.Load", tag.Of("snapshot", s.ID()), tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
 		}
-		// golang/go#36292: Ignore packages with no sources and no errors.
-		if len(pkg.GoFiles) == 0 && len(pkg.CompiledGoFiles) == 0 && len(pkg.Errors) == 0 {
+		// Ignore packages with no sources, since we will never be able to
+		// correctly invalidate that metadata.
+		if len(pkg.GoFiles) == 0 && len(pkg.CompiledGoFiles) == 0 {
 			continue
 		}
 		// Skip test main packages.
-		if s.view.isTestMain(ctx, pkg) {
+		if isTestMain(ctx, pkg, s.view.gocache) {
 			continue
 		}
 		// Set the metadata for this package.
@@ -234,7 +235,7 @@ func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *
 	return nil
 }
 
-func (v *view) isTestMain(ctx context.Context, pkg *packages.Package) bool {
+func isTestMain(ctx context.Context, pkg *packages.Package, gocache string) bool {
 	// Test mains must have an import path that ends with ".test".
 	if !strings.HasSuffix(pkg.PkgPath, ".test") {
 		return false
@@ -247,7 +248,7 @@ func (v *view) isTestMain(ctx context.Context, pkg *packages.Package) bool {
 	if len(pkg.GoFiles) > 1 {
 		return false
 	}
-	if !strings.HasPrefix(pkg.GoFiles[0], v.gocache) {
+	if !strings.HasPrefix(pkg.GoFiles[0], gocache) {
 		return false
 	}
 	return true

internal/lsp/cache/snapshot.go

@@ -359,9 +359,7 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.PackageHandle, e
 func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Package, error) {
 	// Don't reload workspace package metadata.
 	// This function is meant to only return currently cached information.
-	if err := s.view.awaitInitialized(ctx); err != nil {
-		return nil, err
-	}
+	s.view.awaitInitialized(ctx)
 
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -537,9 +535,8 @@ func (s *snapshot) findFileHandle(f *fileBase) source.FileHandle {
 
 func (s *snapshot) awaitLoaded(ctx context.Context) error {
 	// Do not return results until the snapshot's view has been initialized.
-	if err := s.view.awaitInitialized(ctx); err != nil {
-		return err
-	}
+	s.view.awaitInitialized(ctx)
+
 	m, err := s.reloadWorkspace(ctx)
 	if err != nil {
 		return err

internal/lsp/cache/view.go

@@ -90,9 +90,8 @@ type view struct {
 	// 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 don't re-try to avoid too many go/packages calls.
-	initializeOnce      sync.Once
-	initialized         chan struct{}
-	initializationError error
+	initializeOnce sync.Once
+	initialized    chan struct{}
 
 	// builtin pins the AST and package for builtin.go in memory.
 	builtin *builtinPackageHandle
@@ -205,9 +204,8 @@ func (v *view) Rebuild(ctx context.Context) (source.Snapshot, error) {
 }
 
 func (v *view) LookupBuiltin(ctx context.Context, name string) (*ast.Object, error) {
-	if err := v.awaitInitialized(ctx); err != nil {
-		return nil, err
-	}
+	v.awaitInitialized(ctx)
+
 	data := v.builtin.handle.Get(ctx)
 	if ctx.Err() != nil {
 		return nil, ctx.Err()
@@ -553,7 +551,7 @@ func (v *view) initialize(ctx context.Context, s *snapshot) {
 	v.initializeOnce.Do(func() {
 		defer close(v.initialized)
 
-		v.initializationError = func() error {
+		err := func() error {
 			// Do not cancel the call to go/packages.Load for the entire workspace.
 			meta, err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin"))
 			if err != nil {
@@ -576,21 +574,17 @@ func (v *view) initialize(ctx context.Context, s *snapshot) {
 			}
 			return nil
 		}()
+		if err != nil {
+			log.Error(ctx, "initial workspace load failed", err)
+		}
 	})
 }
 
-func (v *view) Initialized(ctx context.Context) bool {
-	err := v.awaitInitialized(ctx)
-	return err == nil
-}
-
-func (v *view) awaitInitialized(ctx context.Context) error {
+func (v *view) awaitInitialized(ctx context.Context) {
 	select {
 	case <-ctx.Done():
-		return ctx.Err()
 	case <-v.initialized:
 	}
-	return v.initializationError
 }
 
 // invalidateContent invalidates the content of a Go file,
@@ -605,7 +599,7 @@ func (v *view) invalidateContent(ctx context.Context, uris []span.URI) source.Sn
 	v.cancelBackground()
 
 	// Do not clone a snapshot until its view has finished initializing.
-	_ = v.awaitInitialized(ctx)
+	v.awaitInitialized(ctx)
 
 	// This should be the only time we hold the view's snapshot lock for any period of time.
 	v.snapshotMu.Lock()

internal/lsp/diagnostics.go

@@ -36,16 +36,14 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot) {
 	ctx, done := trace.StartSpan(ctx, "lsp:background-worker")
 	defer done()
 
-	ctx = telemetry.Snapshot.With(ctx, snapshot.ID())
-
 	// Diagnose all of the packages in the workspace.
 	go func() {
 		wsPackages, err := snapshot.WorkspacePackages(ctx)
 		if ctx.Err() != nil {
 			return
 		}
 		if err != nil {
-			log.Error(ctx, "diagnose: no workspace packages", err, telemetry.Directory.Of(snapshot.View().Folder))
+			log.Error(ctx, "diagnose: no workspace packages", err, telemetry.Snapshot.Of(snapshot.ID()), telemetry.Directory.Of(snapshot.View().Folder))
 			return
 		}
 		for _, ph := range wsPackages {
@@ -70,7 +68,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot) {
 					return
 				}
 				if err != nil {
-					log.Error(ctx, "diagnose: could not generate diagnostics for package", err, telemetry.Package.Of(ph.ID()))
+					log.Error(ctx, "diagnose: could not generate diagnostics for package", err, telemetry.Snapshot.Of(snapshot.ID()), telemetry.Package.Of(ph.ID()))
 					return
 				}
 				s.publishReports(ctx, snapshot, reports, withAnalyses)

internal/lsp/source/view.go

@@ -128,9 +128,6 @@ type View interface {
 	// Snapshot returns the current snapshot for the view.
 	Snapshot() Snapshot
 
-	// Initialized returns true if the view has been initialized without errors.
-	Initialized(ctx context.Context) bool
-
 	// Rebuild rebuilds the current view, replacing the original view in its session.
 	Rebuild(ctx context.Context) (Snapshot, error)