gopls/internal/cache: don't log packages when selectively reloading

Previously, gopls would log package contents whenever reloading (but not
reinitializing) a workspace--essentially whenever the query did not
contain a '...'. But following a big invalidation, gopls may reload
hundreds or even thousands of packages. Logging them is tremendously
verbose.

Change the behavior to simply log packages if and only if verboseOutput
is set. This is easier to understand and greatly reduces the noisiness
of loading.

Also annotate the one test that broke when I experimented with this
behavior. We should really have a more robust framework for asserting on
loads, but that is a project for another CL.

For golang/go#66746

Change-Id: I0eff7fad1b2deb2f170a1c336abf2c2a9e4f56ba
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577875
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/internal/cache/load.go

@@ -47,8 +47,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 	eventName := fmt.Sprintf("go/packages.Load #%d", id) // unique name for logging
 
 	var query []string
-	var containsDir bool // for logging
-	var standalone bool  // whether this is a load of a standalone file
+	var standalone bool // whether this is a load of a standalone file
 
 	// Keep track of module query -> module path so that we can later correlate query
 	// errors with errors.
@@ -103,10 +102,6 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 		default:
 			panic(fmt.Sprintf("unknown scope type %T", scope))
 		}
-		switch scope.(type) {
-		case viewLoadScope, moduleLoadScope:
-			containsDir = true
-		}
 	}
 	if len(query) == 0 {
 		return nil
@@ -217,7 +212,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 			continue
 		}
 
-		if !containsDir || s.Options().VerboseOutput {
+		if s.Options().VerboseOutput {
 			event.Log(ctx, eventName, append(
 				s.Labels(),
 				tag.Package.Of(pkg.ID),

gopls/internal/test/integration/watch/watch_test.go

@@ -363,6 +363,13 @@ func _() {
 // Tests golang/go#38498. Delete a file and then force a reload.
 // Assert that we no longer try to load the file.
 func TestDeleteFiles(t *testing.T) {
+	// TODO(rfindley): this test is brittle, because it depends on underspecified
+	// logging behavior around loads.
+	//
+	// We should have a robust way to test loads. It should be possible to assert
+	// on the specific loads that have occurred, and without the synchronization
+	// problems associated with logging.
+
 	const pkg = `
 -- go.mod --
 module mod.com
@@ -379,6 +386,8 @@ package a
 `
 	t.Run("close then delete", func(t *testing.T) {
 		WithOptions(
+			// verboseOutput causes Snapshot.load to log package files.
+			// (see the TODO above: this is brittle)
 			Settings{"verboseOutput": true},
 		).Run(t, pkg, func(t *testing.T, env *Env) {
 			env.OpenFile("a/a.go")