gopls/internal/lsp/cache: use correct metadata in resolveImportGraph

Following CL 510095, we no longer await loading in resolveImportGraph.
However, we were still acquiring the snapshot metadata once, before
requesting MetadataForFile.

This means that the metadata used to build package handles had more
information than the metadata used to invalidate volatile package
handles, causing some deps not to be present in the handles map.

While at it, change cache.Package to hold only Metadata, not
packageHandle. Going forward, packageHandles should be scoped more
narrowly to the typecheckBatch logic.

Fixes golang/go#61512

Change-Id: I32fc248b3b1329e071caf0cb304f55a6a29f8c49
Reviewed-on: https://go-review.googlesource.com/c/tools/+/513095
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/check.go

@@ -169,7 +169,6 @@ func (s *snapshot) resolveImportGraph() (*importGraph, error) {
 	defer done()
 
 	s.mu.Lock()
-	meta := s.meta
 	lastImportGraph := s.importGraph
 	s.mu.Unlock()
 
@@ -211,28 +210,25 @@ func (s *snapshot) resolveImportGraph() (*importGraph, error) {
 	//
 	// TODO(rfindley): this logic could use a unit test.
 	volatileDeps := make(map[PackageID]bool)
-	var isVolatile func(PackageID) bool
-	isVolatile = func(id PackageID) (volatile bool) {
-		if v, ok := volatileDeps[id]; ok {
+	var isVolatile func(*packageHandle) bool
+	isVolatile = func(ph *packageHandle) (volatile bool) {
+		if v, ok := volatileDeps[ph.m.ID]; ok {
 			return v
 		}
 		defer func() {
-			volatileDeps[id] = volatile
+			volatileDeps[ph.m.ID] = volatile
 		}()
-		if openPackages[id] {
+		if openPackages[ph.m.ID] {
 			return true
 		}
-		m := meta.metadata[id]
-		if m != nil {
-			for _, dep := range m.DepsByPkgPath {
-				if isVolatile(dep) {
-					return true
-				}
+		for _, dep := range ph.m.DepsByPkgPath {
+			if isVolatile(handles[dep]) {
+				return true
 			}
 		}
 		return false
 	}
-	for dep := range handles {
+	for _, dep := range handles {
 		isVolatile(dep)
 	}
 	for id, volatile := range volatileDeps {
@@ -687,7 +683,7 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (*
 		}()
 	}
 
-	return &Package{ph, pkg}, err
+	return &Package{ph.m, pkg}, err
 }
 
 // awaitPredecessors awaits all packages for m.DepsByPkgPath, returning an
@@ -894,6 +890,7 @@ func (s *snapshot) getPackageHandles(ctx context.Context, ids []PackageID) (map[
 	// Copy handles into the result map.
 	handles := make(map[PackageID]*packageHandle, len(b.nodes))
 	for _, v := range b.nodes {
+		assert(v.ph != nil, "nil handle")
 		handles[v.m.ID] = v.ph
 	}
 

gopls/internal/lsp/cache/pkg.go

@@ -27,14 +27,13 @@ type (
 	ImportPath  = source.ImportPath
 )
 
-// A Package is the union of type-checking inputs (packageHandle) and results
-// (a syntaxPackage).
+// A Package is the union of package metadata and type checking results.
 //
 // TODO(rfindley): for now, we do not persist the post-processing of
-// loadDiagnostics, because the value of the snapshot.packages map  is just the
+// loadDiagnostics, because the value of the snapshot.packages map is just the
 // package handle. Fix this.
 type Package struct {
-	ph  *packageHandle
+	m   *source.Metadata
 	pkg *syntaxPackage
 }
 
@@ -76,9 +75,9 @@ func (p *syntaxPackage) methodsets() *methodsets.Index {
 	return p._methodsets
 }
 
-func (p *Package) String() string { return string(p.ph.m.ID) }
+func (p *Package) String() string { return string(p.m.ID) }
 
-func (p *Package) Metadata() *source.Metadata { return p.ph.m }
+func (p *Package) Metadata() *source.Metadata { return p.m }
 
 // A loadScope defines a package loading scope for use with go/packages.
 //
@@ -163,7 +162,7 @@ func (p *Package) GetTypeErrors() []types.Error {
 
 func (p *Package) DiagnosticsForFile(ctx context.Context, s source.Snapshot, uri span.URI) ([]*source.Diagnostic, error) {
 	var diags []*source.Diagnostic
-	for _, diag := range p.ph.m.Diagnostics {
+	for _, diag := range p.m.Diagnostics {
 		if diag.URI == uri {
 			diags = append(diags, diag)
 		}

gopls/internal/lsp/cache/snapshot.go

@@ -647,7 +647,7 @@ func (s *snapshot) PackageDiagnostics(ctx context.Context, ids ...PackageID) (ma
 		return true
 	}
 	post := func(_ int, pkg *Package) {
-		collect(pkg.ph.m.Diagnostics)
+		collect(pkg.m.Diagnostics)
 		collect(pkg.pkg.diagnostics)
 	}
 	return perFile, s.forEachPackage(ctx, ids, pre, post)
@@ -669,7 +669,7 @@ func (s *snapshot) References(ctx context.Context, ids ...PackageID) ([]source.X
 		return true
 	}
 	post := func(i int, pkg *Package) {
-		indexes[i] = XrefIndex{m: pkg.ph.m, data: pkg.pkg.xrefs()}
+		indexes[i] = XrefIndex{m: pkg.m, data: pkg.pkg.xrefs()}
 	}
 	return indexes, s.forEachPackage(ctx, ids, pre, post)
 }