gopls/internal/lsp/cache: memoize active packages after every operation
We were memoizing active packages only after a call to TypeCheck. This
means that we may duplicate work if diagnostics, references, or
implements requests execute before the first call to TypeCheck for an
open package.
Fix this by pushing the memoization logic down into forEachPackage.
This uncovered a bug in go/types that may have already been affecting
users: golang/go#61561. This bug is consistent with user reports on
slack of strange errors related to missing methods.
Avoid this bug in most cases by ensuring that all instantiated
interfaces are completed immediately after type checking. However, this
doesn't completely avoid the bug as it may not complete parameterized
interface literals elsewhere in the type definition. For example, the
following definition is still susceptible to the bug:
type T[P any] interface { m() interface{ n(P) } }
For golang/go#60926
Fixes golang/go#61005
Change-Id: I324cd13bac7c17b1eb5642973157bdbfb2368650
Reviewed-on: https://go-review.googlesource.com/c/tools/+/512636
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 02de896..d96043e 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -50,6 +50,10 @@
// It shares state such as parsed files and imports, to optimize type-checking
// for packages with overlapping dependency graphs.
type typeCheckBatch struct {
+ activePackageCache interface {
+ getActivePackage(id PackageID) *Package
+ setActivePackage(id PackageID, pkg *Package)
+ }
syntaxIndex map[PackageID]int // requested ID -> index in ids
pre preTypeCheck
post postTypeCheck
@@ -89,39 +93,10 @@
// the type-checking operation.
func (s *snapshot) TypeCheck(ctx context.Context, ids ...PackageID) ([]source.Package, error) {
pkgs := make([]source.Package, len(ids))
-
- var (
- needIDs []PackageID // ids to type-check
- indexes []int // original index of requested ids
- )
-
- // Check for existing active packages.
- //
- // Since gopls can't depend on package identity, any instance of the
- // requested package must be ok to return.
- //
- // This is an optimization to avoid redundant type-checking: following
- // changes to an open package many LSP clients send several successive
- // requests for package information for the modified package (semantic
- // tokens, code lens, inlay hints, etc.)
- for i, id := range ids {
- if pkg := s.getActivePackage(id); pkg != nil {
- pkgs[i] = pkg
- } else {
- needIDs = append(needIDs, id)
- indexes = append(indexes, i)
- }
- }
-
post := func(i int, pkg *Package) {
- if alt := s.memoizeActivePackage(pkg.ph.m.ID, pkg); alt != nil && alt != pkg {
- // pkg is an open package, but we've lost a race and an existing package
- // has already been memoized.
- pkg = alt
- }
- pkgs[indexes[i]] = pkg
+ pkgs[i] = pkg
}
- return pkgs, s.forEachPackage(ctx, needIDs, nil, post)
+ return pkgs, s.forEachPackage(ctx, ids, nil, post)
}
// getImportGraph returns a shared import graph use for this snapshot, or nil.
@@ -355,15 +330,16 @@
// If a non-nil importGraph is provided, imports in this graph will be reused.
func (s *snapshot) forEachPackageInternal(ctx context.Context, importGraph *importGraph, importIDs, syntaxIDs []PackageID, pre preTypeCheck, post postTypeCheck, handles map[PackageID]*packageHandle) (*typeCheckBatch, error) {
b := &typeCheckBatch{
- parseCache: s.view.parseCache,
- pre: pre,
- post: post,
- handles: handles,
- fset: fileSetWithBase(reservedForParsing),
- syntaxIndex: make(map[PackageID]int),
- cpulimit: make(chan unit, runtime.GOMAXPROCS(0)),
- syntaxPackages: make(map[PackageID]*futurePackage),
- importPackages: make(map[PackageID]*futurePackage),
+ activePackageCache: s,
+ pre: pre,
+ post: post,
+ handles: handles,
+ parseCache: s.view.parseCache,
+ fset: fileSetWithBase(reservedForParsing),
+ syntaxIndex: make(map[PackageID]int),
+ cpulimit: make(chan unit, runtime.GOMAXPROCS(0)),
+ syntaxPackages: make(map[PackageID]*futurePackage),
+ importPackages: make(map[PackageID]*futurePackage),
}
if importGraph != nil {
@@ -500,6 +476,20 @@
return nil, nil // skip: export data only
}
+ // Check for existing active packages.
+ //
+ // Since gopls can't depend on package identity, any instance of the
+ // requested package must be ok to return.
+ //
+ // This is an optimization to avoid redundant type-checking: following
+ // changes to an open package many LSP clients send several successive
+ // requests for package information for the modified package (semantic
+ // tokens, code lens, inlay hints, etc.)
+ if pkg := b.activePackageCache.getActivePackage(id); ok {
+ b.post(i, pkg)
+ return nil, nil // skip: not checked in this batch
+ }
+
if err := b.awaitPredecessors(ctx, ph.m); err != nil {
// One failed precessesor should not fail the entire type checking
// operation. Errors related to imports will be reported as type checking
@@ -527,7 +517,9 @@
if err != nil {
return nil, err
}
+ b.activePackageCache.setActivePackage(id, syntaxPkg)
b.post(i, syntaxPkg)
+
return syntaxPkg.pkg.types, nil
}
@@ -1465,6 +1457,14 @@
}
}
+ // Work around golang/go#61561: interface instances aren't concurrency-safe
+ // as they are not completed by the type checker.
+ for _, inst := range typeparams.GetInstances(pkg.typesInfo) {
+ if iface, _ := inst.Type.Underlying().(*types.Interface); iface != nil {
+ iface.Complete()
+ }
+ }
+
return pkg, nil
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 12cbc99..d709854 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -864,26 +864,21 @@
return nil
}
-// memoizeActivePackage checks if pkg is active, and if so either records it in
+// setActivePackage checks if pkg is active, and if so either records it in
// the active packages map or returns the existing memoized active package for id.
-//
-// The resulting package is non-nil if and only if the specified package is open.
-func (s *snapshot) memoizeActivePackage(id PackageID, pkg *Package) (active *Package) {
+func (s *snapshot) setActivePackage(id PackageID, pkg *Package) {
s.mu.Lock()
defer s.mu.Unlock()
- if value, ok := s.activePackages.Get(id); ok {
- return value.(*Package) // possibly nil, if we have already checked this id.
+ if _, ok := s.activePackages.Get(id); ok {
+ return // already memoized
}
- defer func() {
- s.activePackages.Set(id, active, nil) // store the result either way: remember that pkg is not open
- }()
-
if containsOpenFileLocked(s, pkg.Metadata()) {
- return pkg
+ s.activePackages.Set(id, pkg, nil)
+ } else {
+ s.activePackages.Set(id, (*Package)(nil), nil) // remember that pkg is not open
}
- return nil
}
func (s *snapshot) resetActivePackagesLocked() {
