gopls/internal/cache: support workspace vendoring

Support workspace vendoring by simply removing logic in gopls for deriving the `-mod` flag. If we don't need to set `-mod=mod`, let the go command decide what to do. We don't need to worry about explicitly setting `-mod=readonly`, since this has been the default since Go 1.16. For golang/go#63375 Change-Id: I1adbe20cef5b9e3edcb7ac2445c4d5ae63f3a3a9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/560466 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
golang · Feb 2, 2024 · aecdb2d · aecdb2d
1 parent e80085c
commit aecdb2d
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 88 deletions.
diff --git a/gopls/internal/cache/mod.go b/gopls/internal/cache/mod.go
@@ -15,8 +15,8 @@ import (
 	"golang.org/x/mod/modfile"
 	"golang.org/x/mod/module"
 	"golang.org/x/tools/gopls/internal/file"
-	"golang.org/x/tools/gopls/internal/protocol/command"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/protocol/command"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/event/tag"
 	"golang.org/x/tools/internal/gocommand"
@@ -350,7 +350,7 @@ func (s *Snapshot) extractGoCommandErrors(ctx context.Context, goCmdError error)
 				// file/position information, so don't even try to find it.
 				continue
 			}
-			loc, found, err := s.matchErrorToModule(ctx, pm, msg)
+			loc, found, err := s.matchErrorToModule(pm, msg)
 			if err != nil {
 				event.Error(ctx, "matching error to module", err)
 				continue
@@ -395,7 +395,7 @@ var moduleVersionInErrorRe = regexp.MustCompile(`[:\s]([+-._~0-9A-Za-z]+)@([+-._
 //
 // It returns the location of a reference to the one of the modules and true
 // if one exists. If none is found it returns a fallback location and false.
-func (s *Snapshot) matchErrorToModule(ctx context.Context, pm *ParsedModule, goCmdError string) (protocol.Location, bool, error) {
+func (s *Snapshot) matchErrorToModule(pm *ParsedModule, goCmdError string) (protocol.Location, bool, error) {
 	var reference *modfile.Line
 	matches := moduleVersionInErrorRe.FindAllStringSubmatch(goCmdError, -1)
 

diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go
@@ -547,51 +547,17 @@ func (s *Snapshot) goCommandInvocation(ctx context.Context, flags InvocationFlag
 	//
 	// TODO(rfindley): should we set -overlays here?
 
-	var modURI protocol.DocumentURI
-	// Select the module context to use.
-	// If we're type checking, we need to use the workspace context, meaning
-	// the main (workspace) module. Otherwise, we should use the module for
-	// the passed-in working dir.
-	if mode == LoadWorkspace {
-		// TODO(rfindley): this seems unnecessary and overly complicated. Remove
-		// this along with 'allowModFileModifications'.
-		if s.view.typ == GoModView {
-			modURI = s.view.gomod
-		}
-	} else {
-		modURI = s.GoModForFile(protocol.URIFromPath(inv.WorkingDir))
-	}
-
-	var modContent []byte
-	if modURI != "" {
-		modFH, err := s.ReadFile(ctx, modURI)
-		if err != nil {
-			return "", nil, cleanup, err
-		}
-		modContent, err = modFH.Content()
-		if err != nil {
-			return "", nil, cleanup, err
-		}
-	}
-
-	// TODO(rfindley): in the case of go.work mode, modURI is empty and we fall
-	// back on the default behavior of vendorEnabled with an empty modURI. Figure
-	// out what is correct here and implement it explicitly.
-	vendorEnabled, err := s.vendorEnabled(modURI, modContent)
-	if err != nil {
-		return "", nil, cleanup, err
-	}
-
 	const mutableModFlag = "mod"
+
 	// If the mod flag isn't set, populate it based on the mode and workspace.
+	//
+	// (As noted in various TODOs throughout this function, this is very
+	// confusing and not obviously correct, but tests pass and we will eventually
+	// rewrite this entire function.)
 	if inv.ModFlag == "" {
 		switch mode {
 		case LoadWorkspace, Normal:
-			if vendorEnabled {
-				inv.ModFlag = "vendor"
-			} else if !allowModfileModificationOption {
-				inv.ModFlag = "readonly"
-			} else {
+			if allowModfileModificationOption {
 				inv.ModFlag = mutableModFlag
 			}
 		case WriteTemporaryModFile:
@@ -608,6 +574,32 @@ func (s *Snapshot) goCommandInvocation(ctx context.Context, flags InvocationFlag
 
 	// If the invocation needs to mutate the modfile, we must use a temp mod.
 	if inv.ModFlag == mutableModFlag {
+		var modURI protocol.DocumentURI
+		// Select the module context to use.
+		// If we're type checking, we need to use the workspace context, meaning
+		// the main (workspace) module. Otherwise, we should use the module for
+		// the passed-in working dir.
+		if mode == LoadWorkspace {
+			// TODO(rfindley): this seems unnecessary and overly complicated. Remove
+			// this along with 'allowModFileModifications'.
+			if s.view.typ == GoModView {
+				modURI = s.view.gomod
+			}
+		} else {
+			modURI = s.GoModForFile(protocol.URIFromPath(inv.WorkingDir))
+		}
+
+		var modContent []byte
+		if modURI != "" {
+			modFH, err := s.ReadFile(ctx, modURI)
+			if err != nil {
+				return "", nil, cleanup, err
+			}
+			modContent, err = modFH.Content()
+			if err != nil {
+				return "", nil, cleanup, err
+			}
+		}
 		if modURI == "" {
 			return "", nil, cleanup, fmt.Errorf("no go.mod file found in %s", inv.WorkingDir)
 		}

diff --git a/gopls/internal/cache/view.go b/gopls/internal/cache/view.go
@@ -25,8 +25,6 @@ import (
 	"sync"
 	"time"
 
-	"golang.org/x/mod/modfile"
-	"golang.org/x/mod/semver"
 	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
@@ -1301,47 +1299,6 @@ func globsMatchPath(globs, target string) bool {
 
 var modFlagRegexp = regexp.MustCompile(`-mod[ =](\w+)`)
 
-// TODO(rstambler): Consolidate modURI and modContent back into a FileHandle
-// 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.
-//
-// TODO(rfindley): move this to snapshot.go.
-func (s *Snapshot) vendorEnabled(modURI protocol.DocumentURI, modContent []byte) (bool, error) {
-	// Legacy GOPATH workspace?
-	if len(s.view.workspaceModFiles) == 0 {
-		return false, nil
-	}
-
-	// Explicit -mod flag?
-	matches := modFlagRegexp.FindStringSubmatch(s.view.folder.Env.GOFLAGS)
-	if len(matches) != 0 {
-		modFlag := matches[1]
-		if modFlag != "" {
-			// Don't override an explicit '-mod=vendor' argument.
-			// We do want to override '-mod=readonly': it would break various module code lenses,
-			// and on 1.16 we know -modfile is available, so we won't mess with go.mod anyway.
-			return modFlag == "vendor", nil
-		}
-	}
-
-	modFile, err := modfile.Parse(modURI.Path(), modContent, nil)
-	if err != nil {
-		return false, err
-	}
-
-	// No vendor directory?
-	// TODO(golang/go#57514): this is wrong if the working dir is not the module
-	// root.
-	if fi, err := os.Stat(filepath.Join(s.view.root.Path(), "vendor")); err != nil || !fi.IsDir() {
-		return false, nil
-	}
-
-	// Vendoring enabled by default by go declaration in go.mod?
-	vendorEnabled := modFile.Go != nil && modFile.Go.Version != "" && semver.Compare("v"+modFile.Go.Version, "v1.14") >= 0
-	return vendorEnabled, nil
-}
-
 // TODO(rfindley): clean up the redundancy of allFilesExcluded,
 // pathExcludedByFilterFunc, pathExcludedByFilter, view.filterFunc...
 func allFilesExcluded(files []string, filterFunc func(protocol.DocumentURI) bool) bool {

diff --git a/gopls/internal/test/integration/modfile/modfile_test.go b/gopls/internal/test/integration/modfile/modfile_test.go
@@ -492,8 +492,8 @@ var _ = blah.Name
 		env.AfterChange(
 			// We would like for the error to appear in the v2 module, but
 			// as of writing non-workspace packages are not diagnosed.
-			Diagnostics(env.AtRegexp("a/main.go", `"example.com/blah/v2"`), WithMessage("cannot find module providing")),
-			Diagnostics(env.AtRegexp("a/go.mod", `require example.com/blah/v2`), WithMessage("cannot find module providing")),
+			Diagnostics(env.AtRegexp("a/main.go", `"example.com/blah/v2"`), WithMessage("no required module provides")),
+			Diagnostics(env.AtRegexp("a/go.mod", `require example.com/blah/v2`), WithMessage("no required module provides")),
 			ReadDiagnostics("a/go.mod", &modDiags),
 		)
 

diff --git a/gopls/internal/test/integration/workspace/workspace_test.go b/gopls/internal/test/integration/workspace/workspace_test.go
@@ -16,6 +16,7 @@ import (
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/goversion"
 	"golang.org/x/tools/internal/gocommand"
+	"golang.org/x/tools/internal/testenv"
 
 	. "golang.org/x/tools/gopls/internal/test/integration"
 )
@@ -248,6 +249,32 @@ func TestAutomaticWorkspaceModule_Interdependent(t *testing.T) {
 	})
 }
 
+func TestWorkspaceVendoring(t *testing.T) {
+	testenv.NeedsGo1Point(t, 22)
+	WithOptions(
+		ProxyFiles(workspaceModuleProxy),
+	).Run(t, multiModule, func(t *testing.T, env *Env) {
+		env.RunGoCommand("work", "init")
+		env.RunGoCommand("work", "use", "moda/a")
+		env.AfterChange()
+		env.OpenFile("moda/a/a.go")
+		env.RunGoCommand("work", "vendor")
+
+		// Make an on-disk go.mod change to force a workspace reinitialization.
+		// This will be fixed in a follow-up CL.
+		env.OpenFile("moda/a/go.mod")
+		env.EditBuffer("moda/a/go.mod", protocol.TextEdit{NewText: "// arbitrary\n"})
+		env.SaveBuffer("moda/a/go.mod")
+
+		env.AfterChange()
+		loc := env.GoToDefinition(env.RegexpSearch("moda/a/a.go", "b.(Hello)"))
+		const want = "vendor/b.com/b/b.go"
+		if got := env.Sandbox.Workdir.URIToPath(loc.URI); got != want {
+			t.Errorf("Definition: got location %q, want %q", got, want)
+		}
+	})
+}
+
 func TestModuleWithExclude(t *testing.T) {
 	const proxy = `
 -- c.com@v1.2.3/go.mod --