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>

gopls/doc/settings.md

@@ -154,6 +154,17 @@ be removed.
 
 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*

gopls/internal/regtest/completion/completion_test.go

@@ -322,7 +322,6 @@ func _() {
 		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"`),
 		)

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -7,7 +7,6 @@ package diagnostics
 import (
 	"context"
 	"fmt"
-	"log"
 	"os/exec"
 	"testing"
 
@@ -147,12 +146,14 @@ const a = 3`)
 		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 @@ func TestDeleteTestVariant(t *testing.T) {
 // 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 @@ package main
 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 main() {}
 		)
 	})
 }
+
+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"),
+		)
+	})
+}

gopls/internal/regtest/workspace/workspace_test.go

@@ -341,12 +341,15 @@ func Hello() int {
 		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 @@ func Hello() int {
 			),
 		)
 		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)

internal/lsp/cache/analysis.go

@@ -26,9 +26,7 @@ import (
 
 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
 		}

internal/lsp/cache/check.go

@@ -41,7 +41,7 @@ type packageHandle struct {
 	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 @@ type packageData struct {
 }
 
 // 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 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id packageID, mode so
 		}
 
 		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 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
 	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 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode sour
 			}
 			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 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *pkg) ([]*source.Diagnost
 
 // 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))
+
+		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))
+		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())
 }

internal/lsp/cache/load.go

@@ -58,6 +58,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	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 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
 		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 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
 			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 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
 	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

internal/lsp/cache/session.go

@@ -221,7 +221,7 @@ func (s *Session) createView(ctx context.Context, name string, folder, tempWorks
 		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),