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 <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go
index c7c3d63..86154db 100644
--- a/gopls/internal/regtest/modfile/modfile_test.go
+++ b/gopls/internal/regtest/modfile/modfile_test.go
@@ -701,19 +701,22 @@
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 @@
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 534174e..cad358e 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -204,7 +204,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 28bf692..7949918 100644
--- a/internal/lsp/cache/mod.go
+++ b/internal/lsp/cache/mod.go
@@ -10,7 +10,6 @@
"path/filepath"
"regexp"
"strings"
- "unicode"
"golang.org/x/mod/modfile"
"golang.org/x/mod/module"
@@ -217,8 +216,6 @@
return mwh.why(ctx, s)
}
-var moduleAtVersionRe = regexp.MustCompile(`^(?P<module>.*)@(?P<version>.*)$`)
-
// 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 @@
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 @@
// 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 <filename>:<line>:<col>,
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 55bb910..7bea58e 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -21,6 +21,7 @@
"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 @@
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 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 @@
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 @@
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 @@
}
}
}
+ file.SortBlocks()
return file, nil
}
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index c07e31b..7a576a6d 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -617,12 +617,22 @@
// 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
}
