From aecdb2d3f498fd6222ab68ba4a1dff0e50b6a97f Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Fri, 2 Feb 2024 15:54:15 -0500 Subject: [PATCH] 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 Reviewed-by: Alan Donovan --- gopls/internal/cache/mod.go | 6 +- gopls/internal/cache/snapshot.go | 72 +++++++++---------- gopls/internal/cache/view.go | 43 ----------- .../test/integration/modfile/modfile_test.go | 4 +- .../integration/workspace/workspace_test.go | 27 +++++++ 5 files changed, 64 insertions(+), 88 deletions(-) diff --git a/gopls/internal/cache/mod.go b/gopls/internal/cache/mod.go index 35946f81084..a120037e221 100644 --- 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 index 3eb87415ad5..a8b137fffb8 100644 --- 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 index 1c425ac97ea..83b41825cea 100644 --- 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 index 49ec0c0a5c5..a71caaebe97 100644 --- 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 index 781e9027f5c..46184ae4ff8 100644 --- 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 --