internal/lsp: only reload invalid metadata when necessary

This change adds a shouldLoad field to knownMetadata so that we can be
more selective about reloading these.

If a package has invalid metadata, but its metadata hasn't changed, we
shouldn't attempt to reload it until the metadata changes.

Fixes golang/go#40312

Change-Id: Icf5a13fd179421b8f70a5eab6a74b30aaf841f49
Reviewed-on: https://go-review.googlesource.com/c/tools/+/298489
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>

Author: stamblerre

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -2045,3 +2045,92 @@ func Hello() {}
 		)
 	})
 }
+
+func TestReloadInvalidMetadata(t *testing.T) {
+	// We only use invalid metadata for Go versions > 1.12.
+	testenv.NeedsGo1Point(t, 13)
+
+	const mod = `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- main.go --
+package main
+
+func _() {}
+`
+	WithOptions(
+		EditorConfig{
+			ExperimentalUseInvalidMetadata: true,
+		},
+		// ExperimentalWorkspaceModule has a different failure mode for this
+		// case.
+		Modes(Singleton),
+	).Run(t, mod, func(t *testing.T, env *Env) {
+		env.Await(
+			OnceMet(
+				InitialWorkspaceLoad,
+				CompletedWork("Load", 1, false),
+			),
+		)
+
+		// Break the go.mod file on disk, expecting a reload.
+		env.WriteWorkspaceFile("go.mod", `modul mod.com
+
+go 1.12
+`)
+		env.Await(
+			OnceMet(
+				env.DoneWithChangeWatchedFiles(),
+				env.DiagnosticAtRegexp("go.mod", "modul"),
+				CompletedWork("Load", 1, false),
+			),
+		)
+
+		env.OpenFile("main.go")
+		env.Await(env.DoneWithOpen())
+		// The first edit after the go.mod file invalidation should cause a reload.
+		// Any subsequent simple edits should not.
+		content := `package main
+
+func main() {
+	_ = 1
+}
+`
+		env.EditBuffer("main.go", fake.NewEdit(0, 0, 3, 0, content))
+		env.Await(
+			OnceMet(
+				env.DoneWithChange(),
+				CompletedWork("Load", 2, false),
+				NoLogMatching(protocol.Error, "error loading file"),
+			),
+		)
+		env.RegexpReplace("main.go", "_ = 1", "_ = 2")
+		env.Await(
+			OnceMet(
+				env.DoneWithChange(),
+				CompletedWork("Load", 2, false),
+				NoLogMatching(protocol.Error, "error loading file"),
+			),
+		)
+		// Add an import to the main.go file and confirm that it does get
+		// reloaded, but the reload fails, so we see a diagnostic on the new
+		// "fmt" import.
+		env.EditBuffer("main.go", fake.NewEdit(0, 0, 5, 0, `package main
+
+import "fmt"
+
+func main() {
+	fmt.Println("")
+}
+`))
+		env.Await(
+			OnceMet(
+				env.DoneWithChange(),
+				env.DiagnosticAtRegexp("main.go", `"fmt"`),
+				CompletedWork("Load", 3, false),
+			),
+		)
+	})
+}

gopls/internal/regtest/workspace/workspace_test.go

@@ -365,7 +365,6 @@ 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/load.go

@@ -54,22 +54,21 @@ type metadata struct {
 
 // load calls packages.Load for the given scopes, updating package metadata,
 // import graph, and mapped files with the result.
-func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) error {
-	if s.view.Options().VerboseWorkDoneProgress {
-		work := s.view.session.progress.Start(ctx, "Load", fmt.Sprintf("Loading scopes %s", scopes), nil, nil)
-		defer func() {
-			go func() {
-				work.End("Done.")
-			}()
-		}()
-	}
-
+func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) (err error) {
 	var query []string
 	var containsDir bool // for logging
 	for _, scope := range scopes {
-		if scope == "" {
+		if !s.shouldLoad(scope) {
 			continue
 		}
+		// Unless the context was canceled, set "shouldLoad" to false for all
+		// of the metadata we attempted to load.
+		defer func() {
+			if errors.Is(err, context.Canceled) {
+				return
+			}
+			s.clearShouldLoad(scope)
+		}()
 		switch scope := scope.(type) {
 		case packagePath:
 			if source.IsCommandLineArguments(string(scope)) {
@@ -110,6 +109,15 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	}
 	sort.Strings(query) // for determinism
 
+	if s.view.Options().VerboseWorkDoneProgress {
+		work := s.view.session.progress.Start(ctx, "Load", fmt.Sprintf("Loading query=%s", query), nil, nil)
+		defer func() {
+			go func() {
+				work.End("Done.")
+			}()
+		}()
+	}
+
 	ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query))
 	defer done()
 
@@ -452,6 +460,8 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
 
 	// If we've already set the metadata for this snapshot, reuse it.
 	if original, ok := s.metadata[m.id]; ok && original.valid {
+		// Since we've just reloaded, clear out shouldLoad.
+		original.shouldLoad = false
 		m = original.metadata
 	} else {
 		s.metadata[m.id] = &knownMetadata{

internal/lsp/cache/snapshot.go

@@ -138,6 +138,9 @@ type knownMetadata struct {
 	// valid is true if the given metadata is valid.
 	// Invalid metadata can still be used if a metadata reload fails.
 	valid bool
+
+	// shouldLoad is true if the given metadata should be reloaded.
+	shouldLoad bool
 }
 
 func (s *snapshot) ID() uint64 {
@@ -1028,6 +1031,71 @@ func (s *snapshot) getMetadata(id packageID) *knownMetadata {
 	return s.metadata[id]
 }
 
+func (s *snapshot) shouldLoad(scope interface{}) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	switch scope := scope.(type) {
+	case packagePath:
+		var meta *knownMetadata
+		for _, m := range s.metadata {
+			if m.pkgPath != scope {
+				continue
+			}
+			meta = m
+		}
+		if meta == nil || meta.shouldLoad {
+			return true
+		}
+		return false
+	case fileURI:
+		uri := span.URI(scope)
+		ids := s.ids[uri]
+		if len(ids) == 0 {
+			return true
+		}
+		for _, id := range ids {
+			m, ok := s.metadata[id]
+			if !ok || m.shouldLoad {
+				return true
+			}
+		}
+		return false
+	default:
+		return true
+	}
+}
+
+func (s *snapshot) clearShouldLoad(scope interface{}) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	switch scope := scope.(type) {
+	case packagePath:
+		var meta *knownMetadata
+		for _, m := range s.metadata {
+			if m.pkgPath == scope {
+				meta = m
+			}
+		}
+		if meta == nil {
+			return
+		}
+		meta.shouldLoad = false
+	case fileURI:
+		uri := span.URI(scope)
+		ids := s.ids[uri]
+		if len(ids) == 0 {
+			return
+		}
+		for _, id := range ids {
+			if m, ok := s.metadata[id]; ok {
+				m.shouldLoad = false
+			}
+		}
+	}
+}
+
 // noValidMetadataForURILocked reports whether there is any valid metadata for
 // the given URI.
 func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool {
@@ -1573,9 +1641,12 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 
 		// Check if the file's package name or imports have changed,
 		// and if so, invalidate this file's packages' metadata.
-		shouldInvalidateMetadata, pkgNameChanged, importDeleted := s.shouldInvalidateMetadata(ctx, result, originalFH, change.fileHandle)
-		anyImportDeleted = anyImportDeleted || importDeleted
+		var shouldInvalidateMetadata, pkgNameChanged, importDeleted bool
+		if !isGoMod(uri) {
+			shouldInvalidateMetadata, pkgNameChanged, importDeleted = s.shouldInvalidateMetadata(ctx, result, originalFH, change.fileHandle)
+		}
 		invalidateMetadata := forceReloadMetadata || workspaceReload || shouldInvalidateMetadata
+		anyImportDeleted = anyImportDeleted || importDeleted
 
 		// Mark all of the package IDs containing the given file.
 		// TODO: if the file has moved into a new package, we should invalidate that too.
@@ -1748,8 +1819,9 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		invalidateMetadata := idsToInvalidate[k]
 		// Mark invalidated metadata rather than deleting it outright.
 		result.metadata[k] = &knownMetadata{
-			metadata: v.metadata,
-			valid:    v.valid && !invalidateMetadata,
+			metadata:   v.metadata,
+			valid:      v.valid && !invalidateMetadata,
+			shouldLoad: v.shouldLoad || invalidateMetadata,
 		}
 	}
 	// Copy the URI to package ID mappings, skipping only those URIs whose

internal/lsp/regtest/expectation.go

@@ -29,7 +29,7 @@ type Expectation interface {
 var (
 	// InitialWorkspaceLoad is an expectation that the workspace initial load has
 	// completed. It is verified via workdone reporting.
-	InitialWorkspaceLoad = CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1)
+	InitialWorkspaceLoad = CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1, false)
 )
 
 // A Verdict is the result of checking an expectation against the current
@@ -196,7 +196,7 @@ func ShowMessageRequest(title string) SimpleExpectation {
 // to be completely processed.
 func (e *Env) DoneWithOpen() Expectation {
 	opens := e.Editor.Stats().DidOpen
-	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), opens)
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), opens, true)
 }
 
 // StartedChange expects there to have been i work items started for
@@ -209,28 +209,28 @@ func StartedChange(i uint64) Expectation {
 // editor to be completely processed.
 func (e *Env) DoneWithChange() Expectation {
 	changes := e.Editor.Stats().DidChange
-	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), changes)
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), changes, true)
 }
 
 // DoneWithSave expects all didSave notifications currently sent by the editor
 // to be completely processed.
 func (e *Env) DoneWithSave() Expectation {
 	saves := e.Editor.Stats().DidSave
-	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidSave), saves)
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidSave), saves, true)
 }
 
 // DoneWithChangeWatchedFiles expects all didChangeWatchedFiles notifications
 // currently sent by the editor to be completely processed.
 func (e *Env) DoneWithChangeWatchedFiles() Expectation {
 	changes := e.Editor.Stats().DidChangeWatchedFiles
-	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), changes)
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), changes, true)
 }
 
 // DoneWithClose expects all didClose notifications currently sent by the
 // editor to be completely processed.
 func (e *Env) DoneWithClose() Expectation {
 	changes := e.Editor.Stats().DidClose
-	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidClose), changes)
+	return CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidClose), changes, true)
 }
 
 // StartedWork expect a work item to have been started >= atLeast times.
@@ -253,16 +253,20 @@ func StartedWork(title string, atLeast uint64) SimpleExpectation {
 //
 // Since the Progress API doesn't include any hidden metadata, we must use the
 // progress notification title to identify the work we expect to be completed.
-func CompletedWork(title string, atLeast uint64) SimpleExpectation {
+func CompletedWork(title string, count uint64, atLeast bool) SimpleExpectation {
 	check := func(s State) Verdict {
-		if s.completedWork[title] >= atLeast {
+		if s.completedWork[title] == count || atLeast && s.completedWork[title] > count {
 			return Met
 		}
 		return Unmet
 	}
+	desc := fmt.Sprintf("completed work %q %v times", title, count)
+	if atLeast {
+		desc = fmt.Sprintf("completed work %q at least %d time(s)", title, count)
+	}
 	return SimpleExpectation{
 		check:       check,
-		description: fmt.Sprintf("completed work %q at least %d time(s)", title, atLeast),
+		description: desc,
 	}
 }