internal/lsp/cache: clean up getOrLoadIDsForURI
Address some TODOs in getOrLoadIDsForURI. In particular, after this
change files will be marked as unloadable if getOrLoadIDsForURI returns
fails to load valid IDs.
Change-Id: I1f09d1c7edd776e46bf8178157bcf9e439b9f293
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447742
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 656d08f..11288e5 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -734,23 +734,14 @@
// Start with the set of package associations derived from the last load.
ids := s.meta.ids[uri]
- hasValidID := false // whether we have any valid package metadata containing uri
shouldLoad := false // whether any packages containing uri are marked 'shouldLoad'
for _, id := range ids {
- // TODO(rfindley): remove the defensiveness here. s.meta.metadata[id] must
- // exist.
- if _, ok := s.meta.metadata[id]; ok {
- hasValidID = true
- }
if len(s.shouldLoad[id]) > 0 {
shouldLoad = true
}
}
// Check if uri is known to be unloadable.
- //
- // TODO(rfindley): shouldn't we also mark uri as unloadable if the load below
- // fails? Otherwise we endlessly load files with no packages.
_, unloadable := s.unloadableFiles[uri]
s.mu.Unlock()
@@ -759,7 +750,7 @@
// - uri is not contained in any valid packages
// - ...or one of the packages containing uri is marked 'shouldLoad'
// - ...but uri is not unloadable
- if (shouldLoad || !hasValidID) && !unloadable {
+ if (shouldLoad || len(ids) == 0) && !unloadable {
scope := fileLoadScope(uri)
err := s.load(ctx, false, scope)
@@ -788,14 +779,11 @@
s.mu.Lock()
ids = s.meta.ids[uri]
- var validIDs []PackageID
- for _, id := range ids {
- // TODO(rfindley): remove the defensiveness here as well.
- if _, ok := s.meta.metadata[id]; ok {
- validIDs = append(validIDs, id)
- }
+ // metadata is only ever added by loading, so if we get here and still have
+ // no ids, uri is unloadable.
+ if !unloadable && len(ids) == 0 {
+ s.unloadableFiles[uri] = struct{}{}
}
- ids = validIDs
s.mu.Unlock()
return ids, nil
@@ -1745,6 +1733,8 @@
// TODO(rfindley): this looks wrong. Shouldn't we clear unloadableFiles on
// changes to environment or workspace layout, or more generally on any
// metadata change?
+ //
+ // Maybe not, as major configuration changes cause a new view.
for k, v := range s.unloadableFiles {
result.unloadableFiles[k] = v
}
