internal/lsp/cache: don't delete metadata until it's reloaded
Retrying CL 271477, this time with parts of CL 322650 incorporated.
This CL moves to a model where we don't automatically delete invalidated
metadata, but rather preserve it and mark it invalid. This way, we can
continue to use invalid metadata for all features even if there is an
issue with the user's workspace.
To keep track of the metadata's validity, we add an invalid flag to
track the status of the metadata. We still reload at the same rate--the
next CL changes the way we reload data.
We also add a configuration to opt-in (currently, this is off by
default).
In some cases, like switches between GOPATH and module modes, and when a
file is deleted, the metadata *must* be deleted outright.
Also, handle an empty GOMODCACHE in the directory filters (from a
previous CL).
Updates golang/go#42266
Change-Id: Idc778dc92cfcf1e4d14116c79754bcca0229e63d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/324394
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/doc/settings.md b/gopls/doc/settings.md
index d2aecc8..27803da 100644
--- a/gopls/doc/settings.md
+++ b/gopls/doc/settings.md
@@ -154,6 +154,17 @@
Default: `false`.
+#### **experimentalUseInvalidMetadata** *bool*
+
+**This setting is experimental and may be deleted.**
+
+experimentalUseInvalidMetadata enables gopls to fall back on outdated
+package metadata to provide editor features if the go command fails to
+load packages for some reason (like an invalid go.mod file). This will
+eventually be the default behavior, and this setting will be removed.
+
+Default: `false`.
+
### Formatting
#### **local** *string*
diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go
index 7dca27c..cd70cca 100644
--- a/gopls/internal/regtest/completion/completion_test.go
+++ b/gopls/internal/regtest/completion/completion_test.go
@@ -322,7 +322,6 @@
env.AcceptCompletion("main.go", pos, item)
// Await the diagnostics to add example.com/blah to the go.mod file.
- env.SaveBufferWithoutActions("main.go")
env.Await(
env.DiagnosticAtRegexp("main.go", `"example.com/blah"`),
)
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 13be65d..689ecad 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -7,7 +7,6 @@
import (
"context"
"fmt"
- "log"
"os/exec"
"testing"
@@ -147,12 +146,14 @@
env.Await(
env.DiagnosticAtRegexp("a.go", "a = 1"),
env.DiagnosticAtRegexp("b.go", "a = 2"),
- env.DiagnosticAtRegexp("c.go", "a = 3"))
+ env.DiagnosticAtRegexp("c.go", "a = 3"),
+ )
env.CloseBuffer("c.go")
env.Await(
env.DiagnosticAtRegexp("a.go", "a = 1"),
env.DiagnosticAtRegexp("b.go", "a = 2"),
- EmptyDiagnostics("c.go"))
+ EmptyDiagnostics("c.go"),
+ )
})
}
@@ -225,7 +226,6 @@
// Tests golang/go#38878: deleting a test file on disk while it's still open
// should not clear its errors.
func TestDeleteTestVariant_DiskOnly(t *testing.T) {
- log.SetFlags(log.Lshortfile)
Run(t, test38878, func(t *testing.T, env *Env) {
env.OpenFile("a_test.go")
env.Await(DiagnosticAt("a_test.go", 5, 3))
@@ -1294,7 +1294,6 @@
func main() {}
`
Run(t, dir, func(t *testing.T, env *Env) {
- log.SetFlags(log.Lshortfile)
env.OpenFile("main.go")
env.OpenFile("other.go")
x := env.DiagnosticsFor("main.go")
@@ -1977,3 +1976,71 @@
)
})
}
+
+func TestUseOfInvalidMetadata(t *testing.T) {
+ testenv.NeedsGo1Point(t, 13)
+
+ const mod = `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- main.go --
+package main
+
+import (
+ "mod.com/a"
+ //"os"
+)
+
+func _() {
+ a.Hello()
+ os.Getenv("")
+ //var x int
+}
+-- a/a.go --
+package a
+
+func Hello() {}
+`
+ WithOptions(
+ EditorConfig{
+ ExperimentalUseInvalidMetadata: true,
+ },
+ Modes(Singleton),
+ ).Run(t, mod, func(t *testing.T, env *Env) {
+ env.OpenFile("go.mod")
+ env.RegexpReplace("go.mod", "module mod.com", "modul mod.com") // break the go.mod file
+ env.SaveBufferWithoutActions("go.mod")
+ env.Await(
+ env.DiagnosticAtRegexp("go.mod", "modul"),
+ )
+ // Confirm that language features work with invalid metadata.
+ env.OpenFile("main.go")
+ file, pos := env.GoToDefinition("main.go", env.RegexpSearch("main.go", "Hello"))
+ wantPos := env.RegexpSearch("a/a.go", "Hello")
+ if file != "a/a.go" && pos != wantPos {
+ t.Fatalf("expected a/a.go:%s, got %s:%s", wantPos, file, pos)
+ }
+ // Confirm that new diagnostics appear with invalid metadata by adding
+ // an unused variable to the body of the function.
+ env.RegexpReplace("main.go", "//var x int", "var x int")
+ env.Await(
+ env.DiagnosticAtRegexp("main.go", "x"),
+ )
+ // Add an import and confirm that we get a diagnostic for it, since the
+ // metadata will not have been updated.
+ env.RegexpReplace("main.go", "//\"os\"", "\"os\"")
+ env.Await(
+ env.DiagnosticAtRegexp("main.go", `"os"`),
+ )
+ // Fix the go.mod file and expect the diagnostic to resolve itself.
+ env.RegexpReplace("go.mod", "modul mod.com", "module mod.com")
+ env.SaveBuffer("go.mod")
+ env.Await(
+ env.DiagnosticAtRegexp("main.go", "x"),
+ env.NoDiagnosticAtRegexp("main.go", `"os"`),
+ EmptyDiagnostics("go.mod"),
+ )
+ })
+}
diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index 24b89db..b367152 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -341,12 +341,15 @@
Modes(Experimental),
).Run(t, multiModule, func(t *testing.T, env *Env) {
env.OpenFile("moda/a/a.go")
+ env.Await(env.DoneWithOpen())
original, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
if want := "modb/b/b.go"; !strings.HasSuffix(original, want) {
t.Errorf("expected %s, got %v", want, original)
}
env.CloseBuffer(original)
+ env.Await(env.DoneWithClose())
+
env.RemoveWorkspaceFile("modb/b/b.go")
env.RemoveWorkspaceFile("modb/go.mod")
env.Await(
@@ -361,6 +364,8 @@
),
)
env.ApplyQuickFixes("moda/a/go.mod", d.Diagnostics)
+ env.Await(env.DoneWithChangeWatchedFiles())
+
got, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello"))
if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(got, want) {
t.Errorf("expected %s, got %v", want, got)
diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go
index c9c50f9..4f5f0bc 100644
--- a/internal/lsp/cache/analysis.go
+++ b/internal/lsp/cache/analysis.go
@@ -26,9 +26,7 @@
func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
var roots []*actionHandle
-
for _, a := range analyzers {
-
if !a.IsEnabled(s.view) {
continue
}
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 00f24eb..d7447b2 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -41,7 +41,7 @@
mode source.ParseMode
// m is the metadata associated with the package.
- m *metadata
+ m *knownMetadata
// key is the hashed key for the package.
key packageHandleKey
@@ -81,6 +81,9 @@
}
// buildPackageHandle returns a packageHandle for a given package and mode.
+// It assumes that the given ID already has metadata available, so it does not
+// attempt to reload missing or invalid metadata. The caller must reload
+// metadata if needed.
func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode source.ParseMode) (*packageHandle, error) {
if ph := s.getPackage(id, mode); ph != nil {
return ph, nil
@@ -117,7 +120,7 @@
}
data := &packageData{}
- data.pkg, data.err = typeCheck(ctx, snapshot, m, mode, deps)
+ data.pkg, data.err = typeCheck(ctx, snapshot, m.metadata, mode, deps)
// Make sure that the workers above have finished before we return,
// especially in case of cancellation.
wg.Wait()
@@ -167,14 +170,22 @@
var depKeys []packageHandleKey
for _, depID := range depList {
depHandle, err := s.buildPackageHandle(ctx, depID, s.workspaceParseMode(depID))
- if err != nil {
- event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, tag.Snapshot.Of(s.id))
+ // Don't use invalid metadata for dependencies if the top-level
+ // metadata is valid. We only load top-level packages, so if the
+ // top-level is valid, all of its dependencies should be as well.
+ if err != nil || m.valid && !depHandle.m.valid {
+ if err != nil {
+ event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, tag.Snapshot.Of(s.id))
+ } else {
+ event.Log(ctx, fmt.Sprintf("%s: invalid dep handle for %s", id, depID), tag.Snapshot.Of(s.id))
+ }
+
if ctx.Err() != nil {
return nil, nil, ctx.Err()
}
// One bad dependency should not prevent us from checking the entire package.
// Add a special key to mark a bad dependency.
- depKeys = append(depKeys, packageHandleKey(fmt.Sprintf("%s import not found", id)))
+ depKeys = append(depKeys, packageHandleKey(fmt.Sprintf("%s import not found", depID)))
continue
}
deps[depHandle.m.pkgPath] = depHandle
@@ -498,7 +509,7 @@
}
dep := resolveImportPath(pkgPath, pkg, deps)
if dep == nil {
- return nil, snapshot.missingPkgError(pkgPath)
+ return nil, snapshot.missingPkgError(ctx, pkgPath)
}
if !source.IsValidImport(string(m.pkgPath), string(dep.m.pkgPath)) {
return nil, errors.Errorf("invalid use of internal package %s", pkgPath)
@@ -713,18 +724,22 @@
// missingPkgError returns an error message for a missing package that varies
// based on the user's workspace mode.
-func (s *snapshot) missingPkgError(pkgPath string) error {
- if s.workspaceMode()&moduleMode != 0 {
- return fmt.Errorf("no required module provides package %q", pkgPath)
- }
- gorootSrcPkg := filepath.FromSlash(filepath.Join(s.view.goroot, "src", pkgPath))
-
+func (s *snapshot) missingPkgError(ctx context.Context, pkgPath string) error {
var b strings.Builder
- b.WriteString(fmt.Sprintf("cannot find package %q in any of \n\t%s (from $GOROOT)", pkgPath, gorootSrcPkg))
+ if s.workspaceMode()&moduleMode == 0 {
+ gorootSrcPkg := filepath.FromSlash(filepath.Join(s.view.goroot, "src", pkgPath))
- for _, gopath := range strings.Split(s.view.gopath, ":") {
- gopathSrcPkg := filepath.FromSlash(filepath.Join(gopath, "src", pkgPath))
- b.WriteString(fmt.Sprintf("\n\t%s (from $GOPATH)", gopathSrcPkg))
+ b.WriteString(fmt.Sprintf("cannot find package %q in any of \n\t%s (from $GOROOT)", pkgPath, gorootSrcPkg))
+
+ for _, gopath := range strings.Split(s.view.gopath, ":") {
+ gopathSrcPkg := filepath.FromSlash(filepath.Join(gopath, "src", pkgPath))
+ b.WriteString(fmt.Sprintf("\n\t%s (from $GOPATH)", gopathSrcPkg))
+ }
+ } else {
+ b.WriteString(fmt.Sprintf("no required module provides package %q", pkgPath))
+ if err := s.getInitializationError(ctx); err != nil {
+ b.WriteString(fmt.Sprintf("(workspace configuration error: %s)", err.MainError))
+ }
}
return errors.New(b.String())
}
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 8387915..88596d6 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -58,6 +58,9 @@
var query []string
var containsDir bool // for logging
for _, scope := range scopes {
+ if scope == "" {
+ continue
+ }
switch scope := scope.(type) {
case packagePath:
if source.IsCommandLineArguments(string(scope)) {
@@ -393,16 +396,18 @@
m.errors = append(m.errors, err)
}
+ uris := map[span.URI]struct{}{}
for _, filename := range pkg.CompiledGoFiles {
uri := span.URIFromPath(filename)
m.compiledGoFiles = append(m.compiledGoFiles, uri)
- s.addID(uri, m.id)
+ uris[uri] = struct{}{}
}
for _, filename := range pkg.GoFiles {
uri := span.URIFromPath(filename)
m.goFiles = append(m.goFiles, uri)
- s.addID(uri, m.id)
+ uris[uri] = struct{}{}
}
+ s.updateIDForURIs(id, uris)
// TODO(rstambler): is this still necessary?
copied := map[packageID]struct{}{
@@ -425,7 +430,7 @@
m.missingDeps[importPkgPath] = struct{}{}
continue
}
- if s.getMetadata(importID) == nil {
+ if s.noValidMetadataForID(importID) {
if _, err := s.setMetadata(ctx, importPkgPath, importPkg, cfg, copied); err != nil {
event.Error(ctx, "error in dependency", err)
}
@@ -436,13 +441,14 @@
s.mu.Lock()
defer s.mu.Unlock()
- // TODO: We should make sure not to set duplicate metadata,
- // and instead panic here. This can be done by making sure not to
- // reset metadata information for packages we've already seen.
- if original, ok := s.metadata[m.id]; ok {
- m = original
+ // If we've already set the metadata for this snapshot, reuse it.
+ if original, ok := s.metadata[m.id]; ok && original.valid {
+ m = original.metadata
} else {
- s.metadata[m.id] = m
+ s.metadata[m.id] = &knownMetadata{
+ metadata: m,
+ valid: true,
+ }
}
// Set the workspace packages. If any of the package's files belong to the
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index f1dc4ef..4e3531d 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -221,7 +221,7 @@
generation: s.cache.store.Generation(generationName(v, 0)),
packages: make(map[packageKey]*packageHandle),
ids: make(map[span.URI][]packageID),
- metadata: make(map[packageID]*metadata),
+ metadata: make(map[packageID]*knownMetadata),
files: make(map[span.URI]source.VersionedFileHandle),
goFiles: make(map[parseKey]*parseGoHandle),
importedBy: make(map[packageID][]packageID),
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index f67f4cd..8981ade 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -73,7 +73,7 @@
// metadata maps file IDs to their associated metadata.
// It may invalidated on calls to go/packages.
- metadata map[packageID]*metadata
+ metadata map[packageID]*knownMetadata
// importedBy maps package IDs to the list of packages that import them.
importedBy map[packageID][]packageID
@@ -131,6 +131,15 @@
analyzer *analysis.Analyzer
}
+// knownMetadata is a wrapper around metadata that tracks its validity.
+type knownMetadata struct {
+ *metadata
+
+ // valid is true if the given metadata is valid.
+ // Invalid metadata can still be used if a metadata reload fails.
+ valid bool
+}
+
func (s *snapshot) ID() uint64 {
return s.id
}
@@ -511,13 +520,13 @@
if fh.Kind() != source.Go {
return nil, fmt.Errorf("no packages for non-Go file %s", uri)
}
- ids := s.getIDsForURI(uri)
- reload := len(ids) == 0
- for _, id := range ids {
+ knownIDs := s.getIDsForURI(uri)
+ reload := len(knownIDs) == 0
+ for _, id := range knownIDs {
// Reload package metadata if any of the metadata has missing
// dependencies, in case something has changed since the last time we
// reloaded it.
- if m := s.getMetadata(id); m == nil {
+ if s.noValidMetadataForID(id) {
reload = true
break
}
@@ -526,13 +535,21 @@
// calls to packages.Load. Determine what we should do instead.
}
if reload {
- if err := s.load(ctx, false, fileURI(uri)); err != nil {
+ err = s.load(ctx, false, fileURI(uri))
+
+ if !s.useInvalidMetadata() && err != nil {
+ return nil, err
+ }
+ // We've tried to reload and there are still no known IDs for the URI.
+ // Return the load error, if there was one.
+ knownIDs = s.getIDsForURI(uri)
+ if len(knownIDs) == 0 {
return nil, err
}
}
- // Get the list of IDs from the snapshot again, in case it has changed.
+
var phs []*packageHandle
- for _, id := range s.getIDsForURI(uri) {
+ for _, id := range knownIDs {
// Filter out any intermediate test variants. We typically aren't
// interested in these packages for file= style queries.
if m := s.getMetadata(id); m != nil && m.isIntermediateTestVariant {
@@ -560,10 +577,16 @@
phs = append(phs, ph)
}
}
-
return phs, nil
}
+// Only use invalid metadata for Go versions >= 1.13. Go 1.12 and below has
+// issues with overlays that will cause confusing error messages if we reuse
+// old metadata.
+func (s *snapshot) useInvalidMetadata() bool {
+ return s.view.goversion >= 13 && s.view.Options().ExperimentalUseInvalidMetadata
+}
+
func (s *snapshot) GetReverseDependencies(ctx context.Context, id string) ([]source.Package, error) {
if err := s.awaitLoaded(ctx); err != nil {
return nil, err
@@ -593,13 +616,15 @@
return ph.check(ctx, s)
}
-// transitiveReverseDependencies populates the uris map with file URIs
+// transitiveReverseDependencies populates the ids map with package IDs
// belonging to the provided package and its transitive reverse dependencies.
func (s *snapshot) transitiveReverseDependencies(id packageID, ids map[packageID]struct{}) {
if _, ok := ids[id]; ok {
return
}
- if s.getMetadata(id) == nil {
+ m := s.getMetadata(id)
+ // Only use invalid metadata if we support it.
+ if m == nil || !(m.valid || s.useInvalidMetadata()) {
return
}
ids[id] = struct{}{}
@@ -697,6 +722,13 @@
return ids
}
+func (s *snapshot) getWorkspacePkgPath(id packageID) packagePath {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
+ return s.workspacePackages[id]
+}
+
func (s *snapshot) fileWatchingGlobPatterns(ctx context.Context) map[string]struct{} {
// Work-around microsoft/vscode#100870 by making sure that we are,
// at least, watching the user's entire workspace. This will still be
@@ -989,46 +1021,70 @@
return s.ids[uri]
}
-func (s *snapshot) getMetadataForURILocked(uri span.URI) (metadata []*metadata) {
- // TODO(matloob): uri can be a file or directory. Should we update the mappings
- // to map directories to their contained packages?
-
- for _, id := range s.ids[uri] {
- if m, ok := s.metadata[id]; ok {
- metadata = append(metadata, m)
- }
- }
- return metadata
-}
-
-func (s *snapshot) getMetadata(id packageID) *metadata {
+func (s *snapshot) getMetadata(id packageID) *knownMetadata {
s.mu.Lock()
defer s.mu.Unlock()
return s.metadata[id]
}
-func (s *snapshot) addID(uri span.URI, id packageID) {
+// noValidMetadataForURILocked reports whether there is any valid metadata for
+// the given URI.
+func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool {
+ ids, ok := s.ids[uri]
+ if !ok {
+ return true
+ }
+ for _, id := range ids {
+ if m, ok := s.metadata[id]; ok && m.valid {
+ return false
+ }
+ }
+ return true
+}
+
+// noValidMetadataForID reports whether there is no valid metadata for the
+// given ID.
+func (s *snapshot) noValidMetadataForID(id packageID) bool {
+ m := s.getMetadata(id)
+ return m == nil || !m.valid
+}
+
+// updateIDForURIs adds the given ID to the set of known IDs for the given URI.
+// Any existing invalid IDs are removed from the set of known IDs. IDs that are
+// not "command-line-arguments" are preferred, so if a new ID comes in for a
+// URI that previously only had "command-line-arguments", the new ID will
+// replace the "command-line-arguments" ID.
+func (s *snapshot) updateIDForURIs(id packageID, uris map[span.URI]struct{}) {
s.mu.Lock()
defer s.mu.Unlock()
- for i, existingID := range s.ids[uri] {
- // TODO: We should make sure not to set duplicate IDs,
- // and instead panic here. This can be done by making sure not to
- // reset metadata information for packages we've already seen.
- if existingID == id {
- return
+ for uri := range uris {
+ // Collect the new set of IDs, preserving any valid existing IDs.
+ newIDs := []packageID{id}
+ for _, existingID := range s.ids[uri] {
+ // Don't set duplicates of the same ID.
+ if existingID == id {
+ continue
+ }
+ // If the package previously only had a command-line-arguments ID,
+ // delete the command-line-arguments workspace package.
+ if source.IsCommandLineArguments(string(existingID)) {
+ delete(s.workspacePackages, existingID)
+ continue
+ }
+ // If the metadata for an existing ID is invalid, and we are
+ // setting metadata for a new, valid ID--don't preserve the old ID.
+ if m, ok := s.metadata[existingID]; !ok || !m.valid {
+ continue
+ }
+ newIDs = append(newIDs, existingID)
}
- // If the package previously only had a command-line-arguments ID,
- // we should just replace it.
- if source.IsCommandLineArguments(string(existingID)) {
- s.ids[uri][i] = id
- // Delete command-line-arguments if it was a workspace package.
- delete(s.workspacePackages, existingID)
- return
- }
+ sort.Slice(newIDs, func(i, j int) bool {
+ return newIDs[i] < newIDs[j]
+ })
+ s.ids[uri] = newIDs
}
- s.ids[uri] = append(s.ids[uri], id)
}
func (s *snapshot) isWorkspacePackage(id packageID) bool {
@@ -1108,12 +1164,20 @@
func (s *snapshot) awaitLoaded(ctx context.Context) error {
loadErr := s.awaitLoadedAllErrors(ctx)
- // If we still have absolutely no metadata, check if the view failed to
- // initialize and return any errors.
- // TODO(rstambler): Should we clear the error after we return it?
s.mu.Lock()
defer s.mu.Unlock()
- if len(s.metadata) == 0 && loadErr != nil {
+
+ // If we still have absolutely no metadata, check if the view failed to
+ // initialize and return any errors.
+ if s.useInvalidMetadata() && len(s.metadata) > 0 {
+ return nil
+ }
+ for _, m := range s.metadata {
+ if m.valid {
+ return nil
+ }
+ }
+ if loadErr != nil {
return loadErr.MainError
}
return nil
@@ -1210,6 +1274,13 @@
return nil
}
+func (s *snapshot) getInitializationError(ctx context.Context) *source.CriticalError {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
+ return s.initializedErr
+}
+
func (s *snapshot) AwaitInitialized(ctx context.Context) {
select {
case <-ctx.Done():
@@ -1228,7 +1299,7 @@
missingMetadata := len(s.workspacePackages) == 0 || len(s.metadata) == 0
pkgPathSet := map[packagePath]struct{}{}
for id, pkgPath := range s.workspacePackages {
- if s.metadata[id] != nil {
+ if m, ok := s.metadata[id]; ok && m.valid {
continue
}
missingMetadata = true
@@ -1300,7 +1371,7 @@
s.mu.Lock()
for _, scope := range scopes {
uri := span.URI(scope.(fileURI))
- if s.getMetadataForURILocked(uri) == nil {
+ if s.noValidMetadataForURILocked(uri) {
s.unloadableFiles[uri] = struct{}{}
}
}
@@ -1333,7 +1404,7 @@
if _, ok := s.unloadableFiles[uri]; ok {
continue
}
- if s.getMetadataForURILocked(uri) == nil {
+ if s.noValidMetadataForURILocked(uri) {
files = append(files, fh)
}
}
@@ -1411,7 +1482,7 @@
initializedErr: s.initializedErr,
ids: make(map[span.URI][]packageID, len(s.ids)),
importedBy: make(map[packageID][]packageID, len(s.importedBy)),
- metadata: make(map[packageID]*metadata, len(s.metadata)),
+ metadata: make(map[packageID]*knownMetadata, len(s.metadata)),
packages: make(map[packageKey]*packageHandle, len(s.packages)),
actions: make(map[actionKey]*actionHandle, len(s.actions)),
files: make(map[span.URI]source.VersionedFileHandle, len(s.files)),
@@ -1480,6 +1551,7 @@
// directIDs keeps track of package IDs that have directly changed.
// It maps id->invalidateMetadata.
directIDs := map[packageID]bool{}
+
// Invalidate all package metadata if the workspace module has changed.
if workspaceReload {
for k := range s.metadata {
@@ -1566,13 +1638,13 @@
// Invalidate reverse dependencies too.
// TODO(heschi): figure out the locking model and use transitiveReverseDeps?
- // transitiveIDs keeps track of transitive reverse dependencies.
+ // idsToInvalidate keeps track of transitive reverse dependencies.
// If an ID is present in the map, invalidate its types.
// If an ID's value is true, invalidate its metadata too.
- transitiveIDs := make(map[packageID]bool)
+ idsToInvalidate := map[packageID]bool{}
var addRevDeps func(packageID, bool)
addRevDeps = func(id packageID, invalidateMetadata bool) {
- current, seen := transitiveIDs[id]
+ current, seen := idsToInvalidate[id]
newInvalidateMetadata := current || invalidateMetadata
// If we've already seen this ID, and the value of invalidate
@@ -1580,7 +1652,7 @@
if seen && current == newInvalidateMetadata {
return
}
- transitiveIDs[id] = newInvalidateMetadata
+ idsToInvalidate[id] = newInvalidateMetadata
for _, rid := range s.getImportedByLocked(id) {
addRevDeps(rid, invalidateMetadata)
}
@@ -1591,7 +1663,7 @@
// Copy the package type information.
for k, v := range s.packages {
- if _, ok := transitiveIDs[k.id]; ok {
+ if _, ok := idsToInvalidate[k.id]; ok {
continue
}
newGen.Inherit(v.handle)
@@ -1599,26 +1671,93 @@
}
// Copy the package analysis information.
for k, v := range s.actions {
- if _, ok := transitiveIDs[k.pkg.id]; ok {
+ if _, ok := idsToInvalidate[k.pkg.id]; ok {
continue
}
newGen.Inherit(v.handle)
result.actions[k] = v
}
+
+ // If the workspace mode has changed, we must delete all metadata, as it
+ // is unusable and may produce confusing or incorrect diagnostics.
+ // If a file has been deleted, we must delete metadata all packages
+ // containing that file.
+ workspaceModeChanged := s.workspaceMode() != result.workspaceMode()
+ skipID := map[packageID]bool{}
+ for _, c := range changes {
+ if c.exists {
+ continue
+ }
+ // The file has been deleted.
+ if ids, ok := s.ids[c.fileHandle.URI()]; ok {
+ for _, id := range ids {
+ skipID[id] = true
+ }
+ }
+ }
+
+ // Collect all of the IDs that are reachable from the workspace packages.
+ // Any unreachable IDs will have their metadata deleted outright.
+ reachableID := map[packageID]bool{}
+ var addForwardDeps func(packageID)
+ addForwardDeps = func(id packageID) {
+ if reachableID[id] {
+ return
+ }
+ reachableID[id] = true
+ m, ok := s.metadata[id]
+ if !ok {
+ return
+ }
+ for _, depID := range m.deps {
+ addForwardDeps(depID)
+ }
+ }
+ for id := range s.workspacePackages {
+ addForwardDeps(id)
+ }
+
+ // Copy the URI to package ID mappings, skipping only those URIs whose
+ // metadata will be reloaded in future calls to load.
+ deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged
+ idsInSnapshot := map[packageID]bool{} // track all known IDs
+ for uri, ids := range s.ids {
+ for _, id := range ids {
+ invalidateMetadata := idsToInvalidate[id]
+ if skipID[id] || (invalidateMetadata && deleteInvalidMetadata) {
+ continue
+ }
+ // The ID is not reachable from any workspace package, so it should
+ // be deleted.
+ if !reachableID[id] {
+ continue
+ }
+ idsInSnapshot[id] = true
+ result.ids[uri] = append(result.ids[uri], id)
+ }
+ }
+
// Copy the package metadata. We only need to invalidate packages directly
// containing the affected file, and only if it changed in a relevant way.
for k, v := range s.metadata {
- if invalidateMetadata, ok := transitiveIDs[k]; invalidateMetadata && ok {
+ if !idsInSnapshot[k] {
+ // Delete metadata for IDs that are no longer reachable from files
+ // in the snapshot.
continue
}
- result.metadata[k] = v
+ invalidateMetadata := idsToInvalidate[k]
+ // Mark invalidated metadata rather than deleting it outright.
+ result.metadata[k] = &knownMetadata{
+ metadata: v.metadata,
+ valid: v.valid && !invalidateMetadata,
+ }
}
// Copy the URI to package ID mappings, skipping only those URIs whose
// metadata will be reloaded in future calls to load.
for k, ids := range s.ids {
var newIDs []packageID
for _, id := range ids {
- if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
+ if invalidateMetadata, ok := idsToInvalidate[id]; invalidateMetadata && ok {
continue
}
newIDs = append(newIDs, id)
@@ -1627,6 +1766,7 @@
result.ids[k] = newIDs
}
}
+
// Copy the set of initially loaded packages.
for id, pkgPath := range s.workspacePackages {
// Packages with the id "command-line-arguments" are generated by the
@@ -1634,7 +1774,7 @@
// module. Do not cache them as workspace packages for longer than
// necessary.
if source.IsCommandLineArguments(string(id)) {
- if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
+ if invalidateMetadata, ok := idsToInvalidate[id]; invalidateMetadata && ok {
continue
}
}
@@ -1686,7 +1826,7 @@
// If the snapshot's workspace mode has changed, the packages loaded using
// the previous mode are no longer relevant, so clear them out.
- if s.workspaceMode() != result.workspaceMode() {
+ if workspaceModeChanged {
result.workspacePackages = map[packageID]packagePath{}
}
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index c26196d..fea4e81 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -1035,7 +1035,11 @@
gomodcache = strings.TrimPrefix(filepath.ToSlash(strings.TrimPrefix(gomodcache, root)), "/")
excluded := false
- for _, filter := range append(opts.DirectoryFilters, "-"+gomodcache) {
+ filters := opts.DirectoryFilters
+ if gomodcache != "" {
+ filters = append(filters, "-"+gomodcache)
+ }
+ for _, filter := range filters {
op, prefix := filter[0], filter[1:]
// Non-empty prefixes have to be precise directory matches.
if prefix != "" {
diff --git a/internal/lsp/fake/edit.go b/internal/lsp/fake/edit.go
index c3f07e2..8b04c39 100644
--- a/internal/lsp/fake/edit.go
+++ b/internal/lsp/fake/edit.go
@@ -18,6 +18,10 @@
Line, Column int
}
+func (p Pos) String() string {
+ return fmt.Sprintf("%v:%v", p.Line, p.Column)
+}
+
// Range corresponds to protocol.Range, but uses the editor friend Pos
// instead of UTF-16 oriented protocol.Position
type Range struct {
diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go
index 989357d..c270e05 100644
--- a/internal/lsp/fake/editor.go
+++ b/internal/lsp/fake/editor.go
@@ -114,11 +114,10 @@
// Whether to edit files with windows line endings.
WindowsLineEndings bool
- DirectoryFilters []string
-
- VerboseOutput bool
-
- ImportShortcut string
+ ImportShortcut string
+ DirectoryFilters []string
+ VerboseOutput bool
+ ExperimentalUseInvalidMetadata bool
}
// NewEditor Creates a new Editor.
@@ -230,6 +229,9 @@
if e.Config.DirectoryFilters != nil {
config["directoryFilters"] = e.Config.DirectoryFilters
}
+ if e.Config.ExperimentalUseInvalidMetadata {
+ config["experimentalUseInvalidMetadata"] = true
+ }
if e.Config.CodeLenses != nil {
config["codelenses"] = e.Config.CodeLenses
}
diff --git a/internal/lsp/fake/workdir.go b/internal/lsp/fake/workdir.go
index aa9ef84..d836deb 100644
--- a/internal/lsp/fake/workdir.go
+++ b/internal/lsp/fake/workdir.go
@@ -190,6 +190,9 @@
if err := os.RemoveAll(fp); err != nil {
return errors.Errorf("removing %q: %w", path, err)
}
+ w.fileMu.Lock()
+ defer w.fileMu.Unlock()
+
evts := []FileEvent{{
Path: path,
ProtocolEvent: protocol.FileEvent{
@@ -198,6 +201,7 @@
},
}}
w.sendEvents(ctx, evts)
+ delete(w.files, path)
return nil
}
diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go
index 5eeacd8..c245fa7 100644
--- a/internal/lsp/regtest/runner.go
+++ b/internal/lsp/regtest/runner.go
@@ -44,7 +44,7 @@
// SeparateProcess forwards connection to a shared separate gopls process.
SeparateProcess
// Experimental enables all of the experimental configurations that are
- // being developed. Currently, it enables the workspace module.
+ // being developed.
Experimental
)
@@ -240,7 +240,7 @@
{"singleton", Singleton, singletonServer},
{"forwarded", Forwarded, r.forwardedServer},
{"separate_process", SeparateProcess, r.separateProcessServer},
- {"experimental_workspace_module", Experimental, experimentalWorkspaceModule},
+ {"experimental", Experimental, experimentalServer},
}
for _, tc := range tests {
@@ -395,9 +395,12 @@
return lsprpc.NewStreamServer(cache.New(optsHook), false)
}
-func experimentalWorkspaceModule(_ context.Context, _ *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
+func experimentalServer(_ context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
options := func(o *source.Options) {
optsHook(o)
+ o.EnableAllExperiments()
+ // ExperimentalWorkspaceModule is not (as of writing) enabled by
+ // source.Options.EnableAllExperiments, but we want to test it.
o.ExperimentalWorkspaceModule = true
}
return lsprpc.NewStreamServer(cache.New(options), false)
diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go
index a30111f..f546726 100755
--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@@ -145,6 +145,19 @@
Hierarchy: "build",
},
{
+ Name: "experimentalUseInvalidMetadata",
+ Type: "bool",
+ Doc: "experimentalUseInvalidMetadata enables gopls to fall back on outdated\npackage metadata to provide editor features if the go command fails to\nload packages for some reason (like an invalid go.mod file). This will\neventually be the default behavior, and this setting will be removed.\n",
+ EnumKeys: EnumKeys{
+ ValueType: "",
+ Keys: nil,
+ },
+ EnumValues: nil,
+ Default: "false",
+ Status: "experimental",
+ Hierarchy: "build",
+ },
+ {
Name: "hoverKind",
Type: "enum",
Doc: "hoverKind controls the information that appears in the hover text.\nSingleLine and Structured are intended for use only by authors of editor plugins.\n",
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index c0e4c90..1162544 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -264,6 +264,12 @@
// downloads rather than requiring user action. This option will eventually
// be removed.
AllowImplicitNetworkAccess bool `status:"experimental"`
+
+ // ExperimentalUseInvalidMetadata enables gopls to fall back on outdated
+ // package metadata to provide editor features if the go command fails to
+ // load packages for some reason (like an invalid go.mod file). This will
+ // eventually be the default behavior, and this setting will be removed.
+ ExperimentalUseInvalidMetadata bool `status:"experimental"`
}
type UIOptions struct {
@@ -606,7 +612,7 @@
for name, value := range opts {
if b, ok := value.(bool); name == "allExperiments" && ok && b {
enableExperiments = true
- options.enableAllExperiments()
+ options.EnableAllExperiments()
}
}
seen := map[string]struct{}{}
@@ -718,13 +724,14 @@
}
}
-// enableAllExperiments turns on all of the experimental "off-by-default"
+// EnableAllExperiments turns on all of the experimental "off-by-default"
// features offered by gopls. Any experimental features specified in maps
// should be enabled in enableAllExperimentMaps.
-func (o *Options) enableAllExperiments() {
+func (o *Options) EnableAllExperiments() {
o.SemanticTokens = true
o.ExperimentalPostfixCompletions = true
o.ExperimentalTemplateSupport = true
+ o.ExperimentalUseInvalidMetadata = true
}
func (o *Options) enableAllExperimentMaps() {
@@ -924,6 +931,9 @@
case "allowImplicitNetworkAccess":
result.setBool(&o.AllowImplicitNetworkAccess)
+ case "experimentalUseInvalidMetadata":
+ result.setBool(&o.ExperimentalUseInvalidMetadata)
+
case "allExperiments":
// This setting should be handled before all of the other options are
// processed, so do nothing here.
