internal/lsp: remove command-line-arguments as a workspace package

If a package starts out as command-line-arguments, and then becomes
"valid" (i.e., gets a package declaration), we shouldn't continue to try
to diagnose "command-line-arguments". We should remove
"command-line-arguments" from workspace packages any time its metadata
is invalidated (assuming it may get added back if a file= query produces
it again).

Include the relevant regression test.

Fixes golang/go#37978

Change-Id: I7fc51edeb58007b4b4a163336cbeb752a53da322
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/lsp/cache/snapshot.go

@@ -478,6 +478,8 @@ func (s *snapshot) addID(uri span.URI, id packageID) {
 		// had a command-line-arguments ID, we should just replace it.
 		if existingID == "command-line-arguments" {
 			s.ids[uri][i] = id
+			// Delete command-line-arguments if it was a workspace package.
+			delete(s.workspacePackages, existingID)
 			return
 		}
 	}
@@ -585,6 +587,7 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error {
 	// Check for context cancellation so that we don't incorrectly mark files
 	// as unloadable, but don't return before setting all workspace packages.
 	if ctx.Err() == nil && err != nil {
+		event.Error(ctx, "reloadOrphanedFiles: failed to load", err, tag.Query.Of(scopes))
 		s.mu.Lock()
 		for _, scope := range scopes {
 			uri := span.URI(scope.(fileURI))
@@ -693,7 +696,6 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi
 		if invalidateMetadata || fileWasSaved(originalFH, currentFH) {
 			result.modTidyHandle = nil
 		}
-
 		if currentFH.Identity().Kind == source.Mod {
 			// If the view's go.mod file's contents have changed, invalidate the metadata
 			// for all of the packages in the workspace.
@@ -749,10 +751,6 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Fi
 		// Make sure to remove the changed file from the unloadable set.
 		delete(result.unloadableFiles, withoutURI)
 	}
-	// Copy the set of initally loaded packages.
-	for k, v := range s.workspacePackages {
-		result.workspacePackages[k] = v
-	}
 	// Copy the package type information.
 	for k, v := range s.packages {
 		if _, ok := transitiveIDs[k.id]; ok {
@@ -786,6 +784,18 @@ outer:
 		}
 		result.ids[k] = ids
 	}
+	// Copy the set of initally loaded packages.
+	for id, pkgPath := range s.workspacePackages {
+		// TODO(rstambler): For now, we only invalidate "command-line-arguments"
+		// from workspace packages, but in general, we need to handle deletion
+		// of a package.
+		if id == "command-line-arguments" {
+			if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
+				continue
+			}
+		}
+		result.workspacePackages[id] = pkgPath
+	}
 	// Don't bother copying the importedBy graph,
 	// as it changes each time we update metadata.
 	return result

internal/lsp/diagnostics.go

@@ -61,7 +61,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA
 		return nil
 	}
 	if err != nil {
-		event.Error(ctx, "diagnose: could not generate diagnostics for go.mod file", err, tag.Directory.Of(snapshot.View().Folder))
+		event.Error(ctx, "warning: diagnose go.mod", err, tag.Directory.Of(snapshot.View().Folder))
 	}
 	// Ensure that the reports returned from mod.Diagnostics are only related to the
 	// go.mod file for the module.
@@ -85,7 +85,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA
 		return nil
 	}
 	if err != nil {
-		event.Error(ctx, "diagnose: no workspace packages", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder))
+		event.Error(ctx, "failed to load workspace packages, skipping diagnostics", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder))
 		return nil
 	}
 	for _, ph := range wsPackages {
@@ -111,7 +111,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA
 				return
 			}
 			if err != nil {
-				event.Error(ctx, "diagnose: could not generate diagnostics for package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(ph.ID()))
+				event.Error(ctx, "warning: diagnose package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(ph.ID()))
 				return
 			}
 			reportsMu.Lock()

internal/lsp/regtest/diagnostics_test.go

@@ -6,11 +6,14 @@ package regtest
 
 import (
 	"testing"
+
+	"golang.org/x/tools/internal/lsp/fake"
 )
 
+// Use mod.com for all go.mod files due to golang/go#35230.
 const exampleProgram = `
 -- go.mod --
-module mod
+module mod.com
 
 go 1.12
 -- main.go --
@@ -34,7 +37,7 @@ func TestDiagnosticErrorInEditedFile(t *testing.T) {
 
 const onlyMod = `
 -- go.mod --
-module mod
+module mod.com
 
 go 1.12
 `
@@ -65,7 +68,7 @@ func TestDiagnosticErrorInNewFile(t *testing.T) {
 // badPackage contains a duplicate definition of the 'a' const.
 const badPackage = `
 -- go.mod --
-module mod
+module mod.com
 
 go 1.12
 -- a.go --
@@ -115,3 +118,27 @@ const a = 3`)
 			EmptyDiagnostics("c.go"))
 	})
 }
+
+func TestIssue37978(t *testing.T) {
+	runner.Run(t, exampleProgram, func(env *Env) {
+		// Create a new workspace-level directory and empty file.
+		env.CreateBuffer("c/c.go", "")
+
+		// Write the file contents with a missing import.
+		env.EditBuffer("c/c.go", fake.Edit{
+			Text: `package c
+
+const a = http.MethodGet
+`,
+		})
+		env.Await(
+			env.DiagnosticAtRegexp("c/c.go", "http.MethodGet"),
+		)
+		// Save file, which will organize imports, adding the expected import.
+		// Expect the diagnostics to clear.
+		env.SaveBuffer("c/c.go")
+		env.Await(
+			EmptyDiagnostics("c/c.go"),
+		)
+	})
+}

internal/lsp/regtest/wrappers.go

@@ -71,7 +71,7 @@ func (e *Env) RegexpSearch(name, re string) fake.Pos {
 		pos, err = e.W.RegexpSearch(name, re)
 	}
 	if err != nil {
-		e.T.Fatalf("RegexpSearch: %v", err)
+		e.T.Fatalf("RegexpSearch: %v, %v", name, err)
 	}
 	return pos
 }