internal/lsp: move the workspaceMode into the snapshot

Workspace mode makes more sense as a property of the snapshot, since
it is determined based on the modules in the workspace. Move it to the
snapshot and enable the GOPATH to modules test. The mode switch means
that we may run `go mod` commands before a `go.mod` is on-disk, so add
handling for that case.

Also, remove the code added in CL 258121 to treat packages starting with
a "_/" the same way as command-line arguments--that's not actually
correct because perfectly valid packages can also have a "_/" package
path prefix.

Fixes golang/go#40340

Change-Id: I35044f5d108983ba00df1359698bf14217caa982
Reviewed-on: https://go-review.googlesource.com/c/tools/+/260078
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/gopls/internal/regtest/watch_test.go b/gopls/internal/regtest/watch_test.go
index 7895787..1462bb3 100644
--- a/gopls/internal/regtest/watch_test.go
+++ b/gopls/internal/regtest/watch_test.go
@@ -10,6 +10,7 @@
 	"golang.org/x/tools/internal/lsp"
 	"golang.org/x/tools/internal/lsp/fake"
 	"golang.org/x/tools/internal/lsp/protocol"
+	"golang.org/x/tools/internal/testenv"
 )
 
 func TestEditFile(t *testing.T) {
@@ -583,7 +584,7 @@
 
 // Reproduces golang/go#40340.
 func TestSwitchFromGOPATHToModules(t *testing.T) {
-	t.Skip("golang/go#40340 is not yet resolved.")
+	testenv.NeedsGo1Point(t, 13)
 
 	const files = `
 -- foo/blah/blah.go --
@@ -599,7 +600,10 @@
 	_ = blah.Name
 }
 `
-	withOptions(InGOPATH()).run(t, files, func(t *testing.T, env *Env) {
+	withOptions(
+		InGOPATH(),
+		WithModes(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 {
@@ -608,15 +612,12 @@
 		env.Await(
 			OnceMet(
 				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1),
-				env.DiagnosticAtRegexp("foo/main.go", `"blah`),
+				env.DiagnosticAtRegexp("foo/main.go", `"blah"`),
 			),
 		)
 		env.RegexpReplace("foo/main.go", `"blah"`, `"mod.com/blah"`)
 		env.Await(
-			OnceMet(
-				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1),
-				NoDiagnostics("foo/main.go"),
-			),
+			EmptyDiagnostics("foo/main.go"),
 		)
 	})
 }
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 709554c..c7bf6ec 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -144,7 +144,7 @@
 	for _, depID := range depList {
 		depHandle, err := s.buildPackageHandle(ctx, depID, s.workspaceParseMode(depID))
 		if err != nil {
-			event.Error(ctx, "no dep handle", err, tag.Package.Of(string(depID)))
+			event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, tag.Snapshot.Of(s.id))
 			if ctx.Err() != nil {
 				return nil, nil, ctx.Err()
 			}
diff --git a/internal/lsp/cache/imports.go b/internal/lsp/cache/imports.go
index 6359115..6274792 100644
--- a/internal/lsp/cache/imports.go
+++ b/internal/lsp/cache/imports.go
@@ -15,8 +15,7 @@
 )
 
 type importsState struct {
-	view *View
-	ctx  context.Context
+	ctx context.Context
 
 	mu                      sync.Mutex
 	processEnv              *imports.ProcessEnv
@@ -46,7 +45,7 @@
 	// should be removed ASAP.
 	var match *moduleRoot
 	for _, m := range snapshot.modules {
-		if m.rootURI == s.view.rootURI {
+		if m.rootURI == snapshot.view.rootURI {
 			match = m
 		}
 	}
@@ -65,13 +64,13 @@
 	}
 	// v.goEnv is immutable -- changes make a new view. Options can change.
 	// We can't compare build flags directly because we may add -modfile.
-	s.view.optionsMu.Lock()
-	localPrefix := s.view.options.Local
-	currentBuildFlags := s.view.options.BuildFlags
+	snapshot.view.optionsMu.Lock()
+	localPrefix := snapshot.view.options.Local
+	currentBuildFlags := snapshot.view.options.BuildFlags
 	changed := !reflect.DeepEqual(currentBuildFlags, s.cachedBuildFlags) ||
-		s.view.options.VerboseOutput != (s.processEnv.Logf != nil) ||
+		snapshot.view.options.VerboseOutput != (s.processEnv.Logf != nil) ||
 		modFileIdentifier != s.cachedModFileIdentifier
-	s.view.optionsMu.Unlock()
+	snapshot.view.optionsMu.Unlock()
 
 	// If anything relevant to imports has changed, clear caches and
 	// update the processEnv. Clearing caches blocks on any background
@@ -88,7 +87,7 @@
 		}
 		s.cachedModFileIdentifier = modFileIdentifier
 		s.cachedBuildFlags = currentBuildFlags
-		s.cleanupProcessEnv, err = s.populateProcessEnv(ctx, modFH, sumFH)
+		s.cleanupProcessEnv, err = s.populateProcessEnv(ctx, snapshot, modFH, sumFH)
 		if err != nil {
 			return err
 		}
@@ -126,26 +125,26 @@
 
 // populateProcessEnv sets the dynamically configurable fields for the view's
 // process environment. Assumes that the caller is holding the s.view.importsMu.
-func (s *importsState) populateProcessEnv(ctx context.Context, modFH, sumFH source.FileHandle) (cleanup func(), err error) {
+func (s *importsState) populateProcessEnv(ctx context.Context, snapshot *snapshot, modFH, sumFH source.FileHandle) (cleanup func(), err error) {
 	cleanup = func() {}
 	pe := s.processEnv
 
-	s.view.optionsMu.Lock()
-	pe.BuildFlags = append([]string(nil), s.view.options.BuildFlags...)
-	if s.view.options.VerboseOutput {
+	snapshot.view.optionsMu.Lock()
+	pe.BuildFlags = append([]string(nil), snapshot.view.options.BuildFlags...)
+	if snapshot.view.options.VerboseOutput {
 		pe.Logf = func(format string, args ...interface{}) {
 			event.Log(ctx, fmt.Sprintf(format, args...))
 		}
 	} else {
 		pe.Logf = nil
 	}
-	s.view.optionsMu.Unlock()
+	snapshot.view.optionsMu.Unlock()
 
 	pe.Env = map[string]string{}
-	for k, v := range s.view.goEnv {
+	for k, v := range snapshot.view.goEnv {
 		pe.Env[k] = v
 	}
-	pe.Env["GO111MODULE"] = s.view.go111module
+	pe.Env["GO111MODULE"] = snapshot.view.go111module
 
 	var modURI span.URI
 	var modContent []byte
@@ -156,7 +155,7 @@
 			return nil, err
 		}
 	}
-	modmod, err := s.view.needsModEqualsMod(ctx, modURI, modContent)
+	modmod, err := snapshot.needsModEqualsMod(ctx, modURI, modContent)
 	if err != nil {
 		return cleanup, err
 	}
@@ -167,7 +166,7 @@
 	}
 
 	// Add -modfile to the build flags, if we are using it.
-	if s.view.workspaceMode&tempModfile != 0 && modFH != nil {
+	if snapshot.workspaceMode()&tempModfile != 0 && modFH != nil {
 		var tmpURI span.URI
 		tmpURI, cleanup, err = tempModFile(modFH, sumFH)
 		if err != nil {
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 66a6165..890b738 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -96,7 +96,7 @@
 	var modURI span.URI
 	var modContent []byte
 	switch {
-	case s.view.workspaceMode&usesWorkspaceModule != 0:
+	case s.workspaceMode()&usesWorkspaceModule != 0:
 		var (
 			tmpDir span.URI
 			err    error
@@ -111,7 +111,7 @@
 		if err != nil {
 			return err
 		}
-	case s.view.workspaceMode&tempModfile != 0:
+	case s.workspaceMode()&tempModfile != 0:
 		// -modfile is unsupported when there are > 1 modules in the workspace.
 		if len(s.modules) != 1 {
 			panic(fmt.Sprintf("unsupported use of -modfile, expected 1 module, got %v", len(s.modules)))
@@ -147,7 +147,7 @@
 	cfg := s.config(ctx, wdir)
 	cfg.BuildFlags = append(cfg.BuildFlags, buildFlags...)
 
-	modMod, err := s.view.needsModEqualsMod(ctx, modURI, modContent)
+	modMod, err := s.needsModEqualsMod(ctx, modURI, modContent)
 	if err != nil {
 		return err
 	}
@@ -219,7 +219,7 @@
 // packages.Loads that occur from within the workspace module.
 func (s *snapshot) tempWorkspaceModule(ctx context.Context) (_ span.URI, cleanup func(), err error) {
 	cleanup = func() {}
-	if s.view.workspaceMode&usesWorkspaceModule == 0 {
+	if s.workspaceMode()&usesWorkspaceModule == 0 {
 		return "", cleanup, nil
 	}
 	wsModuleHandle, err := s.getWorkspaceModuleHandle(ctx)
diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go
index 62eb8b2..50c94f7 100644
--- a/internal/lsp/cache/mod.go
+++ b/internal/lsp/cache/mod.go
@@ -324,7 +324,7 @@
 		// Run "go list -mod readonly -u -m all" to be able to see which deps can be
 		// upgraded without modifying mod file.
 		args := []string{"-u", "-m", "-json", "all"}
-		if s.view.workspaceMode&tempModfile == 0 || containsVendor(fh.URI()) {
+		if s.workspaceMode()&tempModfile == 0 || containsVendor(fh.URI()) {
 			// Use -mod=readonly if the module contains a vendor directory
 			// (see golang/go#38711).
 			args = append([]string{"-mod", "readonly"}, args...)
diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go
index 2a6c850..67feb17 100644
--- a/internal/lsp/cache/mod_tidy.go
+++ b/internal/lsp/cache/mod_tidy.go
@@ -9,6 +9,7 @@
 	"fmt"
 	"go/ast"
 	"io/ioutil"
+	"os"
 	"path/filepath"
 	"sort"
 	"strconv"
@@ -55,12 +56,19 @@
 	if fh.Kind() != source.Mod {
 		return nil, fmt.Errorf("%s is not a go.mod file", fh.URI())
 	}
-	if s.view.workspaceMode&tempModfile == 0 {
+	if s.workspaceMode()&tempModfile == 0 {
 		return nil, source.ErrTmpModfileUnsupported
 	}
 	if handle := s.getModTidyHandle(fh.URI()); handle != nil {
 		return handle.tidy(ctx, s)
 	}
+	// If the file handle is an overlay, it may not be written to disk.
+	// The go.mod file has to be on disk for `go mod tidy` to work.
+	if _, ok := fh.(*overlay); ok {
+		if info, _ := os.Stat(fh.URI().Filename()); info == nil {
+			return nil, source.ErrNoModOnDisk
+		}
+	}
 	workspacePkgs, err := s.WorkspacePackages(ctx)
 	if err != nil {
 		return nil, err
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 58c29e3..00fc455 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -174,11 +174,6 @@
 		return nil, nil, func() {}, err
 	}
 
-	// Now that we have set all required fields,
-	// check if the view has a valid build configuration.
-	validBuildConfiguration := validBuildConfiguration(folder, ws, modules)
-	mode := determineWorkspaceMode(options, validBuildConfiguration, ws, modules)
-
 	// We want a true background context and not a detached context here
 	// the spans need to be unrelated and no tag values should pollute it.
 	baseCtx := event.Detach(xcontext.Detach(ctx))
@@ -198,12 +193,10 @@
 		folder:               folder,
 		filesByURI:           make(map[span.URI]*fileBase),
 		filesByBase:          make(map[string][]*fileBase),
-		workspaceMode:        mode,
 		workspaceInformation: *ws,
 	}
 	v.importsState = &importsState{
-		view: v,
-		ctx:  backgroundCtx,
+		ctx: backgroundCtx,
 		processEnv: &imports.ProcessEnv{
 			GocmdRunner: s.gocmdRunner,
 			WorkingDir:  folder.Filename(),
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 5a542a5..ae45f7d 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -138,6 +138,44 @@
 	return validBuildConfiguration(s.view.rootURI, &s.view.workspaceInformation, s.modules)
 }
 
+// workspaceMode describes the way in which the snapshot's workspace should
+// be loaded.
+func (s *snapshot) workspaceMode() workspaceMode {
+	var mode workspaceMode
+
+	// If the view has an invalid configuration, don't build the workspace
+	// module.
+	validBuildConfiguration := s.ValidBuildConfiguration()
+	if !validBuildConfiguration {
+		return mode
+	}
+	// If the view is not in a module and contains no modules, but still has a
+	// valid workspace configuration, do not create the workspace module.
+	// It could be using GOPATH or a different build system entirely.
+	if len(s.modules) == 0 && validBuildConfiguration {
+		return mode
+	}
+	mode |= moduleMode
+	options := s.view.Options()
+	// The -modfile flag is available for Go versions >= 1.14.
+	if options.TempModfile && s.view.workspaceInformation.goversion >= 14 {
+		mode |= tempModfile
+	}
+	// If the user is intentionally limiting their workspace scope, don't
+	// enable multi-module workspace mode.
+	// TODO(rstambler): This should only change the calculation of the root,
+	// not the mode.
+	if !options.ExpandWorkspaceToModule {
+		return mode
+	}
+	// The workspace module has been disabled by the user.
+	if !options.ExperimentalWorkspaceModule {
+		return mode
+	}
+	mode |= usesWorkspaceModule
+	return mode
+}
+
 // config returns the configuration used for the snapshot's interaction with
 // the go/packages API. It uses the given working directory.
 //
@@ -224,7 +262,7 @@
 	default:
 		buildFlags = append(cfg.BuildFlags, buildFlags...)
 	}
-	if allowTempModfile && s.view.workspaceMode&tempModfile != 0 {
+	if allowTempModfile && s.workspaceMode()&tempModfile != 0 {
 		if modURI == "" {
 			return "", nil, nil, cleanup, fmt.Errorf("no go.mod file found in %s", cfg.Dir)
 		}
@@ -257,7 +295,7 @@
 				return "", nil, nil, nil, err
 			}
 		}
-		modMod, err := s.view.needsModEqualsMod(ctx, modURI, modContent)
+		modMod, err := s.needsModEqualsMod(ctx, modURI, modContent)
 		if err != nil {
 			return "", nil, nil, cleanup, err
 		}
@@ -1089,7 +1127,7 @@
 			rootURI := span.URIFromPath(filepath.Dir(withoutURI.Filename()))
 			if currentMod {
 				if _, ok := result.modules[rootURI]; !ok {
-					if m := getViewModule(ctx, s.view.rootURI, currentFH.URI(), s.view.options); m != nil {
+					if m := getViewModule(ctx, s.view.rootURI, currentFH.URI(), s.view.Options()); m != nil {
 						result.modules[m.rootURI] = m
 						shouldReinitializeView = true
 					}
@@ -1215,11 +1253,11 @@
 	}
 	// Copy the set of initally loaded packages.
 	for id, pkgPath := range s.workspacePackages {
-		// Packages with the id "command-line-arguments" or with a package path
-		// prefixed with "_/" are generated by the go command when the user is
-		// outside of GOPATH and outside of a module. Do not cache them as
-		// workspace packages for longer than necessary.
-		if id == "command-line-arguments" || strings.HasPrefix(string(pkgPath), "_/") {
+		// Packages with the id "command-line-arguments" are generated by the
+		// go command when the user is outside of GOPATH and outside of a
+		// module. Do not cache them as workspace packages for longer than
+		// necessary.
+		if id == "command-line-arguments" {
 			if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
 				continue
 			}
@@ -1269,6 +1307,12 @@
 	if shouldReinitializeView {
 		reinitialize = definitelyReinit
 	}
+
+	// If the snapshot's workspace mode has changed, the packages loaded using
+	// the previous mode are no longer relevant, so clear them out.
+	if s.workspaceMode() != result.workspaceMode() {
+		result.workspacePackages = map[packageID]packagePath{}
+	}
 	return result, reinitialize
 }
 
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 43eeace..3f806a4 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -99,10 +99,6 @@
 	// workspaceInformation tracks various details about this view's
 	// environment variables, go version, and use of modules.
 	workspaceInformation
-
-	// workspaceMode describes the way in which the view's workspace should be
-	// loaded.
-	workspaceMode workspaceMode
 }
 
 type workspaceInformation struct {
@@ -830,12 +826,12 @@
 // after we have a version of the workspace go.mod file on disk. Getting a
 // FileHandle from the cache for temporary files is problematic, since we
 // cannot delete it.
-func (v *View) needsModEqualsMod(ctx context.Context, modURI span.URI, modContent []byte) (bool, error) {
-	if v.goversion < 16 || v.workspaceMode&moduleMode == 0 {
+func (s *snapshot) needsModEqualsMod(ctx context.Context, modURI span.URI, modContent []byte) (bool, error) {
+	if s.view.goversion < 16 || s.workspaceMode()&moduleMode == 0 {
 		return false, nil
 	}
 
-	matches := modFlagRegexp.FindStringSubmatch(v.goEnv["GOFLAGS"])
+	matches := modFlagRegexp.FindStringSubmatch(s.view.goEnv["GOFLAGS"])
 	var modFlag string
 	if len(matches) != 0 {
 		modFlag = matches[1]
@@ -851,44 +847,9 @@
 	if err != nil {
 		return false, err
 	}
-	if fi, err := os.Stat(filepath.Join(v.rootURI.Filename(), "vendor")); err != nil || !fi.IsDir() {
+	if fi, err := os.Stat(filepath.Join(s.view.rootURI.Filename(), "vendor")); err != nil || !fi.IsDir() {
 		return true, nil
 	}
 	vendorEnabled := modFile.Go != nil && modFile.Go.Version != "" && semver.Compare("v"+modFile.Go.Version, "v1.14") >= 0
 	return !vendorEnabled, nil
 }
-
-// determineWorkspaceMode determines the workspace mode for the given view.
-func determineWorkspaceMode(options *source.Options, validBuildConfiguration bool, ws *workspaceInformation, modules map[span.URI]*moduleRoot) workspaceMode {
-	var mode workspaceMode
-
-	// If the view has an invalid configuration, don't build the workspace
-	// module.
-	if !validBuildConfiguration {
-		return mode
-	}
-	// If the view is not in a module and contains no modules, but still has a
-	// valid workspace configuration, do not create the workspace module.
-	// It could be using GOPATH or a different build system entirely.
-	if len(modules) == 0 && validBuildConfiguration {
-		return mode
-	}
-	mode |= moduleMode
-	// The -modfile flag is available for Go versions >= 1.14.
-	if options.TempModfile && ws.goversion >= 14 {
-		mode |= tempModfile
-	}
-	// If the user is intentionally limiting their workspace scope, don't
-	// enable multi-module workspace mode.
-	// TODO(rstambler): This should only change the calculation of the root,
-	// not the mode.
-	if !options.ExpandWorkspaceToModule {
-		return mode
-	}
-	// The workspace module has been disabled by the user.
-	if !options.ExperimentalWorkspaceModule {
-		return mode
-	}
-	mode |= usesWorkspaceModule
-	return mode
-}
diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go
index 3da9d83..090ffd1 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -53,7 +53,7 @@
 	case source.Mod:
 		if diagnostics := params.Context.Diagnostics; len(diagnostics) > 0 {
 			modQuickFixes, err := moduleQuickFixes(ctx, snapshot, fh, diagnostics)
-			if err == source.ErrTmpModfileUnsupported {
+			if source.IsNonFatalGoModError(err) {
 				return nil, nil
 			}
 			if err != nil {
@@ -63,7 +63,7 @@
 		}
 		if wanted[protocol.SourceOrganizeImports] {
 			action, err := goModTidy(ctx, snapshot, fh)
-			if err == source.ErrTmpModfileUnsupported {
+			if source.IsNonFatalGoModError(err) {
 				return nil, nil
 			}
 			if err != nil {
@@ -135,7 +135,7 @@
 				// If there are any diagnostics relating to the go.mod file,
 				// add their corresponding quick fixes.
 				modQuickFixes, err := moduleQuickFixes(ctx, snapshot, fh, diagnostics)
-				if err != nil {
+				if source.IsNonFatalGoModError(err) {
 					// Not a fatal error.
 					event.Error(ctx, "module suggested fixes failed", err, tag.Directory.Of(snapshot.View().Folder()))
 				}
@@ -470,9 +470,6 @@
 		}
 	}
 	tidied, err := snapshot.ModTidy(ctx, modFH)
-	if err == source.ErrTmpModfileUnsupported {
-		return nil, nil
-	}
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 56e0e3d..5360612 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -165,7 +165,7 @@
 		return nil, nil
 	}
 	if modErr != nil {
-		event.Error(ctx, "warning: diagnose go.mod", modErr, tag.Directory.Of(snapshot.View().Folder().Filename()))
+		event.Error(ctx, "warning: diagnose go.mod", modErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
 	}
 	for id, diags := range modReports {
 		if id.URI == "" {
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index b8c2153..649cecc 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -342,6 +342,11 @@
 }
 
 var ErrTmpModfileUnsupported = errors.New("-modfile is unsupported for this Go version")
+var ErrNoModOnDisk = errors.New("go.mod file is not on disk")
+
+func IsNonFatalGoModError(err error) bool {
+	return err == ErrTmpModfileUnsupported || err == ErrNoModOnDisk
+}
 
 // ParseMode controls the content of the AST produced when parsing a source file.
 type ParseMode int