From e55fb40e67ba08fdaebb2e477544eb560f198210 Mon Sep 17 00:00:00 2001
From: Robert Findley <rfindley@google.com>
Date: Mon, 15 Aug 2022 18:41:45 -0400
Subject: [PATCH] internal/lsp/cache: clear shouldLoad IDs on load

CL 417576 externalized shouldLoad tracking into a map, which was used to
trigger a reload and cleared once reload completes. Unfortunately, it
overlooked the fact that we may also reload the entire workspace (via
reinitialization). In this case, we should clear newly loaded IDs from
the shouldLoad map, so that they are not subsequently loaded again.

Fixes golang/go#54473

Change-Id: I26f49552cae502644142dc4a4e946294db37f6f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/424074
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
---
 .../regtest/workspace/workspace_test.go       | 25 +++++++++++++++++++
 internal/lsp/cache/load.go                    |  4 +++
 internal/lsp/regtest/expectation.go           |  6 ++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index 3e980de95d..5196aa9096 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -156,6 +156,31 @@ func TestClearAnalysisDiagnostics(t *testing.T) {
 	})
 }
 
+// TestReloadOnlyOnce checks that changes to the go.mod file do not result in
+// redundant package loads (golang/go#54473).
+//
+// Note that this test may be fragile, as it depends on specific structure to
+// log messages around reinitialization. Nevertheless, it is important for
+// guarding against accidentally duplicate reloading.
+func TestReloadOnlyOnce(t *testing.T) {
+	WithOptions(
+		ProxyFiles(workspaceProxy),
+		WorkspaceFolders("pkg"),
+	).Run(t, workspaceModule, func(t *testing.T, env *Env) {
+		dir := env.Sandbox.Workdir.URI("goodbye").SpanURI().Filename()
+		goModWithReplace := fmt.Sprintf(`%s
+replace random.org => %s
+`, env.ReadWorkspaceFile("pkg/go.mod"), dir)
+		env.WriteWorkspaceFile("pkg/go.mod", goModWithReplace)
+		env.Await(
+			OnceMet(
+				env.DoneWithChangeWatchedFiles(),
+				LogMatching(protocol.Info, `packages\.Load #\d+\n`, 2, false),
+			),
+		)
+	})
+}
+
 // This test checks that gopls updates the set of files it watches when a
 // replace target is added to the go.mod.
 func TestWatchReplaceTargets(t *testing.T) {
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 6d9c598606..2bc9277a02 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -126,11 +126,14 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	if ctx.Err() != nil {
 		return ctx.Err()
 	}
+
+	// This log message is sought for by TestReloadOnlyOnce.
 	if err != nil {
 		event.Error(ctx, eventName, err, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
 	} else {
 		event.Log(ctx, eventName, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
 	}
+
 	if len(pkgs) == 0 {
 		if err == nil {
 			err = fmt.Errorf("no packages returned")
@@ -210,6 +213,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 		if existing := s.meta.metadata[m.ID]; existing == nil || !existing.Valid {
 			updates[m.ID] = m
 			updatedIDs = append(updatedIDs, m.ID)
+			delete(s.shouldLoad, m.ID)
 		}
 	}
 
diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go
index eb88a4c858..e85f40955d 100644
--- a/internal/lsp/regtest/expectation.go
+++ b/internal/lsp/regtest/expectation.go
@@ -337,7 +337,11 @@ func NoErrorLogs() LogExpectation {
 }
 
 // LogMatching asserts that the client has received a log message
-// of type typ matching the regexp re.
+// of type typ matching the regexp re a certain number of times.
+//
+// The count argument specifies the expected number of matching logs. If
+// atLeast is set, this is a lower bound, otherwise there must be exactly cound
+// matching logs.
 func LogMatching(typ protocol.MessageType, re string, count int, atLeast bool) LogExpectation {
 	rec, err := regexp.Compile(re)
 	if err != nil {