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/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() {
