From 9b8df07b91d4b6173fcdd3d9118ce57eeec01259 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 26 Jan 2021 18:18:51 -0500 Subject: [PATCH] internal/lsp: enable -mod=readonly in workspace module mode Now that workspace module mode generates a combined go.sum there are relatively few blockers to enabling -mod=readonly. Fix them and do it. This CL is a bit of a grab bag, but the fixes are relatively separate. I can split it into multiple CLs if desired. - If module A depends on module B at v1.0.0, the go command will want to upgrade the workspace module from v0.0.0-goplsworkspace to v1.0.0. To prevent that, use vN.999999.0 as the base pseudoversion, adjusting v0 to v1 where appropriate. A few test cases needed updating as a result. - For old Go versions, sort the generated workspace module and synthesize a go statement from the maximum go version declared in the workspace. - Some regtests need go.sum files created. - matchErrorToModule created incorrect quick fixes: it would try to download the top-level module mentioned in the error message, not the one that actually caused the problem. Now it issues quick fixes for the lowest-level module. - TestMultiModuleModDiagnostics accidentally included the same module in the workspace twice. Fix it, and make that an error. Fixes golang/go#43346. Change-Id: I605f762a4d23bedd914241525e64c1b3ecc42150 Reviewed-on: https://go-review.googlesource.com/c/tools/+/287032 Trust: Heschi Kreinick Reviewed-by: Robert Findley Reviewed-by: Rebecca Stambler --- .../internal/regtest/modfile/modfile_test.go | 11 +- .../regtest/workspace/workspace_test.go | 31 ++-- internal/lsp/cache/mod.go | 161 +++++++++--------- internal/lsp/cache/snapshot.go | 20 ++- internal/lsp/source/view.go | 12 +- 5 files changed, 134 insertions(+), 101 deletions(-) diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go index c7c3d636ba..86154db9d0 100644 --- a/gopls/internal/regtest/modfile/modfile_test.go +++ b/gopls/internal/regtest/modfile/modfile_test.go @@ -701,19 +701,22 @@ func TestMultiModuleModDiagnostics(t *testing.T) { const mod = ` -- a/go.mod -- -module mod.com +module moda.com go 1.14 require ( example.com v1.2.3 ) +-- a/go.sum -- +example.com v1.2.3 h1:Yryq11hF02fEf2JlOS2eph+ICE2/ceevGV3C9dl5V/c= +example.com v1.2.3/go.mod h1:Y2Rc5rVWjWur0h3pd9aEvK5Pof8YKDANh9gHA2Maujo= -- a/main.go -- package main func main() {} -- b/go.mod -- -module mod.com +module modb.com go 1.14 -- b/main.go -- @@ -730,8 +733,8 @@ func main() { Modes(Experimental), ).Run(t, mod, func(t *testing.T, env *Env) { env.Await( - env.DiagnosticAtRegexp("a/go.mod", "example.com v1.2.3"), - env.DiagnosticAtRegexp("b/go.mod", "module mod.com"), + env.DiagnosticAtRegexpWithMessage("a/go.mod", "example.com v1.2.3", "is not used"), + env.DiagnosticAtRegexpWithMessage("b/go.mod", "module modb.com", "not in your go.mod file"), ) }) } diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index 534174e585..cad358e692 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -204,7 +204,9 @@ func TestAutomaticWorkspaceModule_Interdependent(t *testing.T) { module a.com require b.com v1.2.3 - +-- moda/a/go.sum -- +b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI= +b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8= -- moda/a/a.go -- package a @@ -246,7 +248,9 @@ func TestDeleteModule_Interdependent(t *testing.T) { module a.com require b.com v1.2.3 - +-- moda/a/go.sum -- +b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI= +b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8= -- moda/a/a.go -- package a @@ -311,7 +315,9 @@ func TestCreateModule_Interdependent(t *testing.T) { module a.com require b.com v1.2.3 - +-- moda/a/go.sum -- +b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI= +b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8= -- moda/a/a.go -- package a @@ -414,7 +420,9 @@ func TestUseGoplsMod(t *testing.T) { module a.com require b.com v1.2.3 - +-- moda/a/go.sum -- +b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI= +b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8= -- moda/a/a.go -- package a @@ -430,6 +438,9 @@ func main() { module b.com require example.com v1.2.3 +-- modb/go.sum -- +example.com v1.2.3 h1:Yryq11hF02fEf2JlOS2eph+ICE2/ceevGV3C9dl5V/c= +example.com v1.2.3/go.mod h1:Y2Rc5rVWjWur0h3pd9aEvK5Pof8YKDANh9gHA2Maujo= -- modb/b/b.go -- package b @@ -477,8 +488,8 @@ replace a.com => $SANDBOX_WORKDIR/moda/a env.WriteWorkspaceFile("gopls.mod", fmt.Sprintf(`module gopls-workspace require ( - a.com v0.0.0-goplsworkspace - b.com v0.0.0-goplsworkspace + a.com v1.9999999.0-goplsworkspace + b.com v1.9999999.0-goplsworkspace ) replace a.com => %s/moda/a @@ -493,7 +504,7 @@ replace b.com => %s/modb var d protocol.PublishDiagnosticsParams env.Await( OnceMet( - env.DiagnosticAtRegexp("modb/go.mod", `require example.com v1.2.3`), + env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"), ReadDiagnostics("modb/go.mod", &d), ), ) @@ -648,7 +659,7 @@ func main() { t.Fatalf("reading expected workspace modfile: %v", err) } got := string(gotb) - for _, want := range []string{"a.com v0.0.0-goplsworkspace", "b.com v0.0.0-goplsworkspace"} { + for _, want := range []string{"a.com v1.9999999.0-goplsworkspace", "b.com v1.9999999.0-goplsworkspace"} { if !strings.Contains(got, want) { // want before got here, since the go.mod is multi-line t.Fatalf("workspace go.mod missing %q. got:\n%s", want, got) @@ -659,7 +670,7 @@ func main() { module gopls-workspace require ( - a.com v0.0.0-goplsworkspace + a.com v1.9999999.0-goplsworkspace ) replace a.com => %s/moda/a @@ -670,7 +681,7 @@ func main() { t.Fatalf("reading expected workspace modfile: %v", err) } got = string(gotb) - want := "b.com v0.0.0-goplsworkspace" + want := "b.com v1.9999999.0-goplsworkspace" if strings.Contains(got, want) { t.Fatalf("workspace go.mod contains unexpected %q. got:\n%s", want, got) } diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 28bf692048..7949918dfe 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -10,7 +10,6 @@ import ( "path/filepath" "regexp" "strings" - "unicode" "golang.org/x/mod/modfile" "golang.org/x/mod/module" @@ -217,8 +216,6 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string return mwh.why(ctx, s) } -var moduleAtVersionRe = regexp.MustCompile(`^(?P.*)@(?P.*)$`) - // extractGoCommandError tries to parse errors that come from the go command // and shape them into go.mod diagnostics. func (s *snapshot) extractGoCommandErrors(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, goCmdError string) []*source.Error { @@ -236,6 +233,8 @@ func (s *snapshot) extractGoCommandErrors(ctx context.Context, snapshot source.S return srcErrs } +var moduleVersionInErrorRe = regexp.MustCompile(`[:\s]([+-._~0-9A-Za-z]+)@([+-._~0-9A-Za-z]+)[:\s]`) + // matchErrorToModule attempts to match module version in error messages. // Some examples: // @@ -243,99 +242,99 @@ func (s *snapshot) extractGoCommandErrors(ctx context.Context, snapshot source.S // go: github.com/cockroachdb/apd/v2@v2.0.72: reading github.com/cockroachdb/apd/go.mod at revision v2.0.72: unknown revision v2.0.72 // go: example.com@v1.2.3 requires\n\trandom.org@v1.2.3: parsing go.mod:\n\tmodule declares its path as: bob.org\n\tbut was required as: random.org // -// We split on colons and whitespace, and attempt to match on something -// that matches module@version. If we're able to find a match, we try to -// find anything that matches it in the go.mod file. +// We search for module@version, starting from the end to find the most +// relevant module, e.g. random.org@v1.2.3 above. Then we associate the error +// with a directive that references any of the modules mentioned. func (s *snapshot) matchErrorToModule(ctx context.Context, fh source.FileHandle, goCmdError string) *source.Error { - var v module.Version - fields := strings.FieldsFunc(goCmdError, func(r rune) bool { - return unicode.IsSpace(r) || r == ':' - }) - for _, field := range fields { - match := moduleAtVersionRe.FindStringSubmatch(field) - if match == nil { - continue - } - path, version := match[1], match[2] - // Any module versions that come from the workspace module should not - // be shown to the user. - if source.IsWorkspaceModuleVersion(version) { - continue - } - if err := module.Check(path, version); err != nil { - continue - } - v.Path, v.Version = path, version - break - } pm, err := s.ParseMod(ctx, fh) if err != nil { return nil } - toSourceError := func(line *modfile.Line) *source.Error { - rng, err := rangeFromPositions(pm.Mapper, line.Start, line.End) + + var innermost *module.Version + var reference *modfile.Line + matches := moduleVersionInErrorRe.FindAllStringSubmatch(goCmdError, -1) + +outer: + for i := len(matches) - 1; i >= 0; i-- { + ver := module.Version{Path: matches[i][1], Version: matches[i][2]} + // Any module versions that come from the workspace module should not + // be shown to the user. + if source.IsWorkspaceModuleVersion(ver.Version) { + continue + } + if err := module.Check(ver.Path, ver.Version); err != nil { + continue + } + if innermost == nil { + innermost = &ver + } + + for _, req := range pm.File.Require { + if req.Mod == ver { + reference = req.Syntax + break outer + } + } + for _, ex := range pm.File.Exclude { + if ex.Mod == ver { + reference = ex.Syntax + break outer + } + } + for _, rep := range pm.File.Replace { + if rep.New == ver || rep.Old == ver { + reference = rep.Syntax + break outer + } + } + } + + if reference == nil { + // No match for the module path was found in the go.mod file. + // Show the error on the module declaration, if one exists. + if pm.File.Module == nil { + return nil + } + reference = pm.File.Module.Syntax + } + + rng, err := rangeFromPositions(pm.Mapper, reference.Start, reference.End) + if err != nil { + return nil + } + disabledByGOPROXY := strings.Contains(goCmdError, "disabled by GOPROXY=off") + shouldAddDep := strings.Contains(goCmdError, "to add it") + if innermost != nil && (disabledByGOPROXY || shouldAddDep) { + args, err := source.MarshalArgs(fh.URI(), false, []string{fmt.Sprintf("%v@%v", innermost.Path, innermost.Version)}) if err != nil { return nil } - disabledByGOPROXY := strings.Contains(goCmdError, "disabled by GOPROXY=off") - shouldAddDep := strings.Contains(goCmdError, "to add it") - if v.Path != "" && (disabledByGOPROXY || shouldAddDep) { - args, err := source.MarshalArgs(fh.URI(), false, []string{fmt.Sprintf("%v@%v", v.Path, v.Version)}) - if err != nil { - return nil - } - msg := goCmdError - if disabledByGOPROXY { - msg = fmt.Sprintf("%v@%v has not been downloaded", v.Path, v.Version) - } - return &source.Error{ - Message: msg, - Kind: source.ListError, - Range: rng, - URI: fh.URI(), - SuggestedFixes: []source.SuggestedFix{{ - Title: fmt.Sprintf("Download %v@%v", v.Path, v.Version), - Command: &protocol.Command{ - Title: source.CommandAddDependency.Title, - Command: source.CommandAddDependency.ID(), - Arguments: args, - }, - }}, - } + msg := goCmdError + if disabledByGOPROXY { + msg = fmt.Sprintf("%v@%v has not been downloaded", innermost.Path, innermost.Version) } return &source.Error{ - Message: goCmdError, + Message: msg, + Kind: source.ListError, Range: rng, URI: fh.URI(), - Kind: source.ListError, + SuggestedFixes: []source.SuggestedFix{{ + Title: fmt.Sprintf("Download %v@%v", innermost.Path, innermost.Version), + Command: &protocol.Command{ + Title: source.CommandAddDependency.Title, + Command: source.CommandAddDependency.ID(), + Arguments: args, + }, + }}, } } - // Check if there are any require, exclude, or replace statements that - // match this module version. - for _, req := range pm.File.Require { - if req.Mod != v { - continue - } - return toSourceError(req.Syntax) + return &source.Error{ + Message: goCmdError, + Range: rng, + URI: fh.URI(), + Kind: source.ListError, } - for _, ex := range pm.File.Exclude { - if ex.Mod != v { - continue - } - return toSourceError(ex.Syntax) - } - for _, rep := range pm.File.Replace { - if rep.New != v && rep.Old != v { - continue - } - return toSourceError(rep.Syntax) - } - // No match for the module path was found in the go.mod file. - // Show the error on the module declaration, if one exists. - if pm.File.Module == nil { - return nil - } - return toSourceError(pm.File.Module.Syntax) } // errorPositionRe matches errors messages of the form ::, diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 55bb910403..7bea58e2a3 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -21,6 +21,7 @@ import ( "golang.org/x/mod/modfile" "golang.org/x/mod/module" + "golang.org/x/mod/semver" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/event" @@ -323,12 +324,8 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat case source.LoadWorkspace, source.Normal: if vendorEnabled { inv.ModFlag = "vendor" - } else if s.workspaceMode()&usesWorkspaceModule == 0 && !allowModfileModificationOption { + } else if !allowModfileModificationOption { inv.ModFlag = "readonly" - } else { - // Temporarily allow updates for multi-module workspace mode: - // it doesn't create a go.sum at all. golang/go#42509. - inv.ModFlag = mutableModFlag } case source.UpdateUserModFile, source.WriteTemporaryModFile: inv.ModFlag = mutableModFlag @@ -1721,6 +1718,9 @@ func BuildGoplsMod(ctx context.Context, root span.URI, s source.Snapshot) (*modf func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, fs source.FileSource) (*modfile.File, error) { file := &modfile.File{} file.AddModuleStmt("gopls-workspace") + // Track the highest Go version, to be set on the workspace module. + // Fall back to 1.12 -- old versions insist on having some version. + goVersion := "1.12" paths := make(map[string]span.URI) for modURI := range modFiles { @@ -1739,7 +1739,13 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, if file == nil || parsed.Module == nil { return nil, fmt.Errorf("no module declaration for %s", modURI) } + if parsed.Go != nil && semver.Compare(goVersion, parsed.Go.Version) < 0 { + goVersion = parsed.Go.Version + } path := parsed.Module.Mod.Path + if _, ok := paths[path]; ok { + return nil, fmt.Errorf("found module %q twice in the workspace", path) + } paths[path] = modURI // If the module's path includes a major version, we expect it to have // a matching major version. @@ -1753,6 +1759,9 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, return nil, err } } + if goVersion != "" { + file.AddGoStmt(goVersion) + } // Go back through all of the modules to handle any of their replace // statements. for modURI := range modFiles { @@ -1792,6 +1801,7 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, } } } + file.SortBlocks() return file, nil } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index c07e31bd74..7a576a6dac 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -617,12 +617,22 @@ var ( // sure not to show this version to end users in error messages, to avoid // confusion. // The major version is not included, as that depends on the module path. -const workspaceModuleVersion = ".0.0-goplsworkspace" +// +// If workspace module A is dependent on workspace module B, we need our +// nonexistant version to be greater than the version A mentions. +// Otherwise, the go command will try to update to that version. Use a very +// high minor version to make that more likely. +const workspaceModuleVersion = ".9999999.0-goplsworkspace" func IsWorkspaceModuleVersion(version string) bool { return strings.HasSuffix(version, workspaceModuleVersion) } func WorkspaceModuleVersion(majorVersion string) string { + // Use the highest compatible major version to avoid unwanted upgrades. + // See the comment on workspaceModuleVersion. + if majorVersion == "v0" { + majorVersion = "v1" + } return majorVersion + workspaceModuleVersion }