internal/lsp/cache: fix load race, refactor

As far as I can tell, the code I removed in from load did roughly
nothing -- returning nil metadata didn't suppress type checking as I
think was intended. Throwing away the metadata also created the race in

Pull the check for missing import changes up to PackageHandles, where it
is non-racy and can cause type checking to be skipped. Simplify and
refactor.

Fixes golang/go#35951.

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

internal/lsp/cache/load.go

@@ -71,48 +71,7 @@ func (s *snapshot) load(ctx context.Context, scope source.Scope) ([]*metadata, e
 	if err != nil {
 		return nil, err
 	}
-	m, prevMissingImports, err := s.updateMetadata(ctx, scope, pkgs, cfg)
-	if err != nil {
-		return nil, err
-	}
-	meta, err := validateMetadata(ctx, m, prevMissingImports)
-	if err != nil {
-		return nil, err
-	}
-	return meta, nil
-}
-
-func validateMetadata(ctx context.Context, metadata []*metadata, prevMissingImports map[packageID]map[packagePath]struct{}) ([]*metadata, error) {
-	// If we saw incorrect metadata for this package previously, don't bother rechecking it.
-	for _, m := range metadata {
-		if len(m.missingDeps) > 0 {
-			prev, ok := prevMissingImports[m.id]
-			// There are missing imports that we previously hadn't seen before.
-			if !ok {
-				return metadata, nil
-			}
-			// The set of missing imports has changed.
-			if !sameSet(prev, m.missingDeps) {
-				return metadata, nil
-			}
-		} else {
-			// There are no missing imports.
-			return metadata, nil
-		}
-	}
-	return nil, nil
-}
-
-func sameSet(x, y map[packagePath]struct{}) bool {
-	if len(x) != len(y) {
-		return false
-	}
-	for k := range x {
-		if _, ok := y[k]; !ok {
-			return false
-		}
-	}
-	return true
+	return s.updateMetadata(ctx, scope, pkgs, cfg)
 }
 
 // shouldLoad reparses a file's package and import declarations to
@@ -150,7 +109,7 @@ func (c *cache) shouldLoad(ctx context.Context, s *snapshot, originalFH, current
 	return false
 }
 
-func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, map[packageID]map[packagePath]struct{}, error) {
+func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []*packages.Package, cfg *packages.Config) ([]*metadata, error) {
 	// Clear metadata since we are re-running go/packages.
 	var m []*metadata
 	switch uri.(type) {
@@ -165,12 +124,6 @@ func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []
 	default:
 		panic(fmt.Errorf("unsupported Scope type %T", uri))
 	}
-	prevMissingImports := make(map[packageID]map[packagePath]struct{})
-	for _, m := range m {
-		if len(m.missingDeps) > 0 {
-			prevMissingImports[m.id] = m.missingDeps
-		}
-	}
 
 	var results []*metadata
 	for _, pkg := range pkgs {
@@ -178,7 +131,7 @@ func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []
 
 		// Set the metadata for this package.
 		if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg, map[packageID]struct{}{}); err != nil {
-			return nil, nil, err
+			return nil, err
 		}
 		m := s.getMetadata(packageID(pkg.ID))
 		if m != nil {
@@ -190,9 +143,9 @@ func (s *snapshot) updateMetadata(ctx context.Context, uri source.Scope, pkgs []
 	s.clearAndRebuildImportGraph()
 
 	if len(results) == 0 {
-		return nil, nil, errors.Errorf("no metadata for %s", uri)
+		return nil, errors.Errorf("no metadata for %s", uri)
 	}
-	return results, prevMissingImports, nil
+	return results, nil
 }
 
 func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *packages.Package, cfg *packages.Config, seen map[packageID]struct{}) error {

internal/lsp/cache/snapshot.go

@@ -63,28 +63,28 @@ func (s *snapshot) View() source.View {
 
 func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]source.PackageHandle, error) {
 	ctx = telemetry.File.With(ctx, fh.Identity().URI)
-
-	metadata := s.getMetadataForURI(fh.Identity().URI)
-
+	meta := s.getMetadataForURI(fh.Identity().URI)
 	// Determine if we need to type-check the package.
-	phs, load, check := s.shouldCheck(metadata)
+	phs, load, check := s.shouldCheck(meta)
 
 	// We may need to re-load package metadata.
 	// We only need to this if it has been invalidated, and is therefore unvailable.
 	if load {
-		var err error
-		m, err := s.load(ctx, source.FileURI(fh.Identity().URI))
+		newMeta, err := s.load(ctx, source.FileURI(fh.Identity().URI))
 		if err != nil {
 			return nil, err
 		}
-		// If metadata was returned, from the load call, use it.
-		if m != nil {
-			metadata = m
+		newMissing := missingImports(newMeta)
+		if len(newMissing) != 0 {
+			// Type checking a package with the same missing imports over and over
+			// is futile. Don't re-check unless something has changed.
+			check = check && !sameSet(missingImports(meta), newMissing)
 		}
+		meta = newMeta
 	}
 	if check {
 		var results []source.PackageHandle
-		for _, m := range metadata {
+		for _, m := range meta {
 			ph, err := s.packageHandle(ctx, m.id, source.ParseFull)
 			if err != nil {
 				return nil, err
@@ -96,9 +96,32 @@ func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]
 	if len(phs) == 0 {
 		return nil, errors.Errorf("no CheckPackageHandles for %s", fh.Identity().URI)
 	}
+
 	return phs, nil
 }
 
+func missingImports(metadata []*metadata) map[packagePath]struct{} {
+	result := map[packagePath]struct{}{}
+	for _, m := range metadata {
+		for path := range m.missingDeps {
+			result[path] = struct{}{}
+		}
+	}
+	return result
+}
+
+func sameSet(x, y map[packagePath]struct{}) bool {
+	if len(x) != len(y) {
+		return false
+	}
+	for k := range x {
+		if _, ok := y[k]; !ok {
+			return false
+		}
+	}
+	return true
+}
+
 func (s *snapshot) PackageHandle(ctx context.Context, id string) (source.PackageHandle, error) {
 	ctx = telemetry.Package.With(ctx, id)