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>

Author: heschi

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"),
 		)
 	})
 }

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)
 		}

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<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,106 +233,108 @@ 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:
 //
 //    example.com@v1.2.2: reading example.com/@v/v1.2.2.mod: no such file or directory
 //    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]
+	pm, err := s.ParseMod(ctx, fh)
+	if err != nil {
+		return nil
+	}
+
+	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(version) {
+		if source.IsWorkspaceModuleVersion(ver.Version) {
 			continue
 		}
-		if err := module.Check(path, version); err != nil {
+		if err := module.Check(ver.Path, ver.Version); err != nil {
 			continue
 		}
-		v.Path, v.Version = path, version
-		break
+		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
+			}
+		}
 	}
-	pm, err := s.ParseMod(ctx, fh)
+
+	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
 	}
-	toSourceError := func(line *modfile.Line) *source.Error {
-		rng, err := rangeFromPositions(pm.Mapper, line.Start, line.End)
+	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,
-		}
-	}
-	// 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)
-	}
-	for _, ex := range pm.File.Exclude {
-		if ex.Mod != v {
-			continue
+			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,
+				},
+			}},
 		}
-		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 &source.Error{
+		Message: goCmdError,
+		Range:   rng,
+		URI:     fh.URI(),
+		Kind:    source.ListError,
 	}
-	return toSourceError(pm.File.Module.Syntax)
 }
 
 // errorPositionRe matches errors messages of the form <filename>:<line>:<col>,

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
 }