internal/lsp/cache: get control of reloadOrphanedFiles

According to its comment, reloadOrphanedFiles is intended to work around
overlay bugs. But as of 1.16, there are no overlay bugs that we know of.
So what is it still doing?

Apparently, quite a bit, not much of it useful. Clean up as much as
possible.

- Files with no valid package declaration are ignored by the go command.
There's no point trying to reload them; stop.
- During metadata invalidation, we clear out all IDs for a file, even if
only one of its IDs is invalidated, e.g. when a test package is removed.
That leaves valid metadata for the non-test, so we don't refresh it in
the workspace reload, and only catch it as an orphan. It seems to me we
should only remove the invalidated ID.
- If the client incorrectly sends us a didOpen for a non-Go file, we
will attempt to load it as an orphaned file. Fix the regtest that did
that.
- TestEmptyGOPATHXTest_40825 set up an invalid GOPATH: you can't work in
GOPATH/src. However, it exists to test code that no longer exists, so
just delete it.

After this change, almost none of the regression tests trigger orphaned
file reloading. It's difficult/impractical to rule it out entirely
because some of them only appear racily. Since I intend to remove the
code path, I'm not too worried about more creeping in before I'm done.

The only useful case is multiple ad-hoc packages. Because we only
allow one "command-line-arguments" package in the workspace, if you
switch between two the old one becomes orphaned. I hope to work on that
soon.

Change-Id: Ia355cf104280ce51f6189c6638e8da8f4aef2ace
Reviewed-on: https://go-review.googlesource.com/c/tools/+/302089
Trust: Heschi Kreinick <heschi@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -572,12 +572,8 @@ hi mom
 			WithOptions(EditorConfig{
 				Env: map[string]string{"GO111MODULE": go111module},
 			}).Run(t, files, func(t *testing.T, env *Env) {
-				env.OpenFile("hello.txt")
 				env.Await(
-					OnceMet(
-						env.DoneWithOpen(),
-						NoShowMessage(),
-					),
+					NoOutstandingWork(),
 				)
 			})
 		})
@@ -603,7 +599,14 @@ func main() {
 	fmt.Println("")
 }
 `
-	WithOptions(InGOPATH()).Run(t, collision, func(t *testing.T, env *Env) {
+	WithOptions(
+		InGOPATH(),
+		EditorConfig{
+			Env: map[string]string{
+				"GO111MODULE": "off",
+			},
+		},
+	).Run(t, collision, func(t *testing.T, env *Env) {
 		env.OpenFile("x/x.go")
 		env.Await(
 			env.DiagnosticAtRegexpWithMessage("x/x.go", `^`, "found packages main (main.go) and x (x.go)"),
@@ -898,24 +901,6 @@ package foo_
 	})
 }
 
-// Reproduces golang/go#40825.
-func TestEmptyGOPATHXTest_40825(t *testing.T) {
-	const files = `
--- x.go --
-package x
--- x_test.go --
-`
-
-	WithOptions(InGOPATH()).Run(t, files, func(t *testing.T, env *Env) {
-		env.OpenFile("x_test.go")
-		env.EditBuffer("x_test.go", fake.NewEdit(0, 0, 0, 0, "pack"))
-		env.Await(
-			env.DoneWithChange(),
-			NoShowMessage(),
-		)
-	})
-}
-
 func TestIgnoredFiles(t *testing.T) {
 	const ws = `
 -- go.mod --
@@ -1491,6 +1476,11 @@ package foo_
 	WithOptions(
 		ProxyFiles(proxy),
 		InGOPATH(),
+		EditorConfig{
+			Env: map[string]string{
+				"GO111MODULE": "off",
+			},
+		},
 	).Run(t, contents, func(t *testing.T, env *Env) {
 		// Simulate typing character by character.
 		env.OpenFile("foo/foo_test.go")

gopls/internal/regtest/watch/watch_test.go

@@ -617,19 +617,18 @@ func main() {
 `
 	WithOptions(
 		InGOPATH(),
+		EditorConfig{
+			Env: map[string]string{
+				"GO111MODULE": "auto",
+			},
+		},
 		Modes(Experimental), // module is in a subdirectory
 	).Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("foo/main.go")
 		env.Await(env.DiagnosticAtRegexp("foo/main.go", `"blah"`))
 		if err := env.Sandbox.RunGoCommand(env.Ctx, "foo", "mod", []string{"init", "mod.com"}); err != nil {
 			t.Fatal(err)
 		}
-		env.Await(
-			OnceMet(
-				env.DoneWithChangeWatchedFiles(),
-				env.DiagnosticAtRegexp("foo/main.go", `"blah"`),
-			),
-		)
 		env.RegexpReplace("foo/main.go", `"blah"`, `"mod.com/blah"`)
 		env.Await(
 			EmptyDiagnostics("foo/main.go"),
@@ -661,6 +660,11 @@ func main() {
 `
 	WithOptions(
 		InGOPATH(),
+		EditorConfig{
+			Env: map[string]string{
+				"GO111MODULE": "auto",
+			},
+		},
 	).Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("foo/main.go")
 		env.RemoveWorkspaceFile("foo/go.mod")

internal/lsp/cache/check.go

@@ -764,7 +764,7 @@ func isValidImport(pkgPath, importPkgPath packagePath) bool {
 	if i == -1 {
 		return true
 	}
-	if pkgPath == "command-line-arguments" {
+	if isCommandLineArguments(string(pkgPath)) {
 		return true
 	}
 	return strings.HasPrefix(string(pkgPath), string(importPkgPath[:i]))

internal/lsp/cache/snapshot.go

@@ -1085,7 +1085,7 @@ func shouldShowAdHocPackagesWarning(snapshot source.Snapshot, pkgs []source.Pack
 
 func containsCommandLineArguments(pkgs []source.Package) bool {
 	for _, pkg := range pkgs {
-		if strings.Contains(pkg.ID(), "command-line-arguments") {
+		if isCommandLineArguments(pkg.ID()) {
 			return true
 		}
 	}
@@ -1096,6 +1096,13 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr
 	// Do not return results until the snapshot's view has been initialized.
 	s.AwaitInitialized(ctx)
 
+	// TODO(rstambler): Should we be more careful about returning the
+	// initialization error? Is it possible for the initialization error to be
+	// corrected without a successful reinitialization?
+	if s.initializedErr != nil {
+		return s.initializedErr
+	}
+
 	if ctx.Err() != nil {
 		return &source.CriticalError{MainError: ctx.Err()}
 	}
@@ -1114,10 +1121,7 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr
 			DiagList:  diags,
 		}
 	}
-	// TODO(rstambler): Should we be more careful about returning the
-	// initialization error? Is it possible for the initialization error to be
-	// corrected without a successful reinitialization?
-	return s.initializedErr
+	return nil
 }
 
 func (s *snapshot) AwaitInitialized(ctx context.Context) {
@@ -1173,11 +1177,27 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error {
 	// that exist only in overlays. As a workaround, we search all of the files
 	// available in the snapshot and reload their metadata individually using a
 	// file= query if the metadata is unavailable.
-	scopes := s.orphanedFileScopes()
+	files := s.orphanedFiles()
+
+	// Files without a valid package declaration can't be loaded. Don't try.
+	var scopes []interface{}
+	for _, file := range files {
+		pgf, err := s.ParseGo(ctx, file, source.ParseHeader)
+		if err != nil {
+			continue
+		}
+		if !pgf.File.Package.IsValid() {
+			continue
+		}
+		scopes = append(scopes, fileURI(file.URI()))
+	}
+
 	if len(scopes) == 0 {
 		return nil
 	}
 
+	// The regtests match this exact log message, keep them in sync.
+	event.Log(ctx, "reloadOrphanedFiles reloading", tag.Query.Of(scopes))
 	err := s.load(ctx, false, scopes...)
 
 	// If we failed to load some files, i.e. they have no metadata,
@@ -1203,11 +1223,11 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error {
 	return nil
 }
 
-func (s *snapshot) orphanedFileScopes() []interface{} {
+func (s *snapshot) orphanedFiles() []source.VersionedFileHandle {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	scopeSet := make(map[span.URI]struct{})
+	var files []source.VersionedFileHandle
 	for uri, fh := range s.files {
 		// Don't try to reload metadata for go.mod files.
 		if fh.Kind() != source.Go {
@@ -1228,14 +1248,10 @@ func (s *snapshot) orphanedFileScopes() []interface{} {
 			continue
 		}
 		if s.getMetadataForURILocked(uri) == nil {
-			scopeSet[uri] = struct{}{}
+			files = append(files, fh)
 		}
 	}
-	var scopes []interface{}
-	for uri := range scopeSet {
-		scopes = append(scopes, fileURI(uri))
-	}
-	return scopes
+	return files
 }
 
 func contains(views []*View, view *View) bool {
@@ -1485,14 +1501,17 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 	}
 	// Copy the URI to package ID mappings, skipping only those URIs whose
 	// metadata will be reloaded in future calls to load.
-copyIDs:
 	for k, ids := range s.ids {
+		var newIDs []packageID
 		for _, id := range ids {
 			if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
-				continue copyIDs
+				continue
 			}
+			newIDs = append(newIDs, id)
+		}
+		if len(newIDs) != 0 {
+			result.ids[k] = newIDs
 		}
-		result.ids[k] = ids
 	}
 	// Copy the set of initially loaded packages.
 	for id, pkgPath := range s.workspacePackages {