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 <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index c6d1344..5d36f5e 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -169,7 +169,6 @@
defer done()
s.mu.Lock()
- meta := s.meta
lastImportGraph := s.importGraph
s.mu.Unlock()
@@ -211,28 +210,25 @@
//
// 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 @@
}()
}
- return &Package{ph, pkg}, err
+ return &Package{ph.m, pkg}, err
}
// awaitPredecessors awaits all packages for m.DepsByPkgPath, returning an
@@ -894,6 +890,7 @@
// 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
}
diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go
index ec15e02..68ec38a 100644
--- a/gopls/internal/lsp/cache/pkg.go
+++ b/gopls/internal/lsp/cache/pkg.go
@@ -27,14 +27,13 @@
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 @@
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) 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)
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 94a463b..863e488 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -647,7 +647,7 @@
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 @@
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)
}
