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>

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) {

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)
 		}
 	}
 

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 {