Skip to content

Commit 9b8df07

Browse files
committed
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 <[email protected]> Reviewed-by: Robert Findley <[email protected]> Reviewed-by: Rebecca Stambler <[email protected]>
1 parent f1f686b commit 9b8df07

File tree

5 files changed

+128
-95
lines changed

5 files changed

+128
-95
lines changed

gopls/internal/regtest/modfile/modfile_test.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -701,19 +701,22 @@ func TestMultiModuleModDiagnostics(t *testing.T) {
701701

702702
const mod = `
703703
-- a/go.mod --
704-
module mod.com
704+
module moda.com
705705
706706
go 1.14
707707
708708
require (
709709
example.com v1.2.3
710710
)
711+
-- a/go.sum --
712+
example.com v1.2.3 h1:Yryq11hF02fEf2JlOS2eph+ICE2/ceevGV3C9dl5V/c=
713+
example.com v1.2.3/go.mod h1:Y2Rc5rVWjWur0h3pd9aEvK5Pof8YKDANh9gHA2Maujo=
711714
-- a/main.go --
712715
package main
713716
714717
func main() {}
715718
-- b/go.mod --
716-
module mod.com
719+
module modb.com
717720
718721
go 1.14
719722
-- b/main.go --
@@ -730,8 +733,8 @@ func main() {
730733
Modes(Experimental),
731734
).Run(t, mod, func(t *testing.T, env *Env) {
732735
env.Await(
733-
env.DiagnosticAtRegexp("a/go.mod", "example.com v1.2.3"),
734-
env.DiagnosticAtRegexp("b/go.mod", "module mod.com"),
736+
env.DiagnosticAtRegexpWithMessage("a/go.mod", "example.com v1.2.3", "is not used"),
737+
env.DiagnosticAtRegexpWithMessage("b/go.mod", "module modb.com", "not in your go.mod file"),
735738
)
736739
})
737740
}

gopls/internal/regtest/workspace/workspace_test.go

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,9 @@ func TestAutomaticWorkspaceModule_Interdependent(t *testing.T) {
204204
module a.com
205205
206206
require b.com v1.2.3
207-
207+
-- moda/a/go.sum --
208+
b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI=
209+
b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8=
208210
-- moda/a/a.go --
209211
package a
210212
@@ -246,7 +248,9 @@ func TestDeleteModule_Interdependent(t *testing.T) {
246248
module a.com
247249
248250
require b.com v1.2.3
249-
251+
-- moda/a/go.sum --
252+
b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI=
253+
b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8=
250254
-- moda/a/a.go --
251255
package a
252256
@@ -311,7 +315,9 @@ func TestCreateModule_Interdependent(t *testing.T) {
311315
module a.com
312316
313317
require b.com v1.2.3
314-
318+
-- moda/a/go.sum --
319+
b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI=
320+
b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8=
315321
-- moda/a/a.go --
316322
package a
317323
@@ -414,7 +420,9 @@ func TestUseGoplsMod(t *testing.T) {
414420
module a.com
415421
416422
require b.com v1.2.3
417-
423+
-- moda/a/go.sum --
424+
b.com v1.2.3 h1:tXrlXP0rnjRpKNmkbLYoWBdq0ikb3C3bKK9//moAWBI=
425+
b.com v1.2.3/go.mod h1:D+J7pfFBZK5vdIdZEFquR586vKKIkqG7Qjw9AxG5BQ8=
418426
-- moda/a/a.go --
419427
package a
420428
@@ -430,6 +438,9 @@ func main() {
430438
module b.com
431439
432440
require example.com v1.2.3
441+
-- modb/go.sum --
442+
example.com v1.2.3 h1:Yryq11hF02fEf2JlOS2eph+ICE2/ceevGV3C9dl5V/c=
443+
example.com v1.2.3/go.mod h1:Y2Rc5rVWjWur0h3pd9aEvK5Pof8YKDANh9gHA2Maujo=
433444
-- modb/b/b.go --
434445
package b
435446
@@ -477,8 +488,8 @@ replace a.com => $SANDBOX_WORKDIR/moda/a
477488
env.WriteWorkspaceFile("gopls.mod", fmt.Sprintf(`module gopls-workspace
478489
479490
require (
480-
a.com v0.0.0-goplsworkspace
481-
b.com v0.0.0-goplsworkspace
491+
a.com v1.9999999.0-goplsworkspace
492+
b.com v1.9999999.0-goplsworkspace
482493
)
483494
484495
replace a.com => %s/moda/a
@@ -493,7 +504,7 @@ replace b.com => %s/modb
493504
var d protocol.PublishDiagnosticsParams
494505
env.Await(
495506
OnceMet(
496-
env.DiagnosticAtRegexp("modb/go.mod", `require example.com v1.2.3`),
507+
env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"),
497508
ReadDiagnostics("modb/go.mod", &d),
498509
),
499510
)
@@ -648,7 +659,7 @@ func main() {
648659
t.Fatalf("reading expected workspace modfile: %v", err)
649660
}
650661
got := string(gotb)
651-
for _, want := range []string{"a.com v0.0.0-goplsworkspace", "b.com v0.0.0-goplsworkspace"} {
662+
for _, want := range []string{"a.com v1.9999999.0-goplsworkspace", "b.com v1.9999999.0-goplsworkspace"} {
652663
if !strings.Contains(got, want) {
653664
// want before got here, since the go.mod is multi-line
654665
t.Fatalf("workspace go.mod missing %q. got:\n%s", want, got)
@@ -659,7 +670,7 @@ func main() {
659670
module gopls-workspace
660671
661672
require (
662-
a.com v0.0.0-goplsworkspace
673+
a.com v1.9999999.0-goplsworkspace
663674
)
664675
665676
replace a.com => %s/moda/a
@@ -670,7 +681,7 @@ func main() {
670681
t.Fatalf("reading expected workspace modfile: %v", err)
671682
}
672683
got = string(gotb)
673-
want := "b.com v0.0.0-goplsworkspace"
684+
want := "b.com v1.9999999.0-goplsworkspace"
674685
if strings.Contains(got, want) {
675686
t.Fatalf("workspace go.mod contains unexpected %q. got:\n%s", want, got)
676687
}

internal/lsp/cache/mod.go

Lines changed: 74 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"path/filepath"
1111
"regexp"
1212
"strings"
13-
"unicode"
1413

1514
"golang.org/x/mod/modfile"
1615
"golang.org/x/mod/module"
@@ -217,8 +216,6 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string
217216
return mwh.why(ctx, s)
218217
}
219218

220-
var moduleAtVersionRe = regexp.MustCompile(`^(?P<module>.*)@(?P<version>.*)$`)
221-
222219
// extractGoCommandError tries to parse errors that come from the go command
223220
// and shape them into go.mod diagnostics.
224221
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
236233
return srcErrs
237234
}
238235

236+
var moduleVersionInErrorRe = regexp.MustCompile(`[:\s]([+-._~0-9A-Za-z]+)@([+-._~0-9A-Za-z]+)[:\s]`)
237+
239238
// matchErrorToModule attempts to match module version in error messages.
240239
// Some examples:
241240
//
242241
// [email protected]: reading example.com/@v/v1.2.2.mod: no such file or directory
243242
// go: github.com/cockroachdb/apd/[email protected]: reading github.com/cockroachdb/apd/go.mod at revision v2.0.72: unknown revision v2.0.72
244243
// go: [email protected] requires\n\[email protected]: parsing go.mod:\n\tmodule declares its path as: bob.org\n\tbut was required as: random.org
245244
//
246-
// We split on colons and whitespace, and attempt to match on something
247-
// that matches module@version. If we're able to find a match, we try to
248-
// find anything that matches it in the go.mod file.
245+
// We search for module@version, starting from the end to find the most
246+
// relevant module, e.g. [email protected] above. Then we associate the error
247+
// with a directive that references any of the modules mentioned.
249248
func (s *snapshot) matchErrorToModule(ctx context.Context, fh source.FileHandle, goCmdError string) *source.Error {
250-
var v module.Version
251-
fields := strings.FieldsFunc(goCmdError, func(r rune) bool {
252-
return unicode.IsSpace(r) || r == ':'
253-
})
254-
for _, field := range fields {
255-
match := moduleAtVersionRe.FindStringSubmatch(field)
256-
if match == nil {
257-
continue
258-
}
259-
path, version := match[1], match[2]
249+
pm, err := s.ParseMod(ctx, fh)
250+
if err != nil {
251+
return nil
252+
}
253+
254+
var innermost *module.Version
255+
var reference *modfile.Line
256+
matches := moduleVersionInErrorRe.FindAllStringSubmatch(goCmdError, -1)
257+
258+
outer:
259+
for i := len(matches) - 1; i >= 0; i-- {
260+
ver := module.Version{Path: matches[i][1], Version: matches[i][2]}
260261
// Any module versions that come from the workspace module should not
261262
// be shown to the user.
262-
if source.IsWorkspaceModuleVersion(version) {
263+
if source.IsWorkspaceModuleVersion(ver.Version) {
263264
continue
264265
}
265-
if err := module.Check(path, version); err != nil {
266+
if err := module.Check(ver.Path, ver.Version); err != nil {
266267
continue
267268
}
268-
v.Path, v.Version = path, version
269-
break
269+
if innermost == nil {
270+
innermost = &ver
271+
}
272+
273+
for _, req := range pm.File.Require {
274+
if req.Mod == ver {
275+
reference = req.Syntax
276+
break outer
277+
}
278+
}
279+
for _, ex := range pm.File.Exclude {
280+
if ex.Mod == ver {
281+
reference = ex.Syntax
282+
break outer
283+
}
284+
}
285+
for _, rep := range pm.File.Replace {
286+
if rep.New == ver || rep.Old == ver {
287+
reference = rep.Syntax
288+
break outer
289+
}
290+
}
270291
}
271-
pm, err := s.ParseMod(ctx, fh)
292+
293+
if reference == nil {
294+
// No match for the module path was found in the go.mod file.
295+
// Show the error on the module declaration, if one exists.
296+
if pm.File.Module == nil {
297+
return nil
298+
}
299+
reference = pm.File.Module.Syntax
300+
}
301+
302+
rng, err := rangeFromPositions(pm.Mapper, reference.Start, reference.End)
272303
if err != nil {
273304
return nil
274305
}
275-
toSourceError := func(line *modfile.Line) *source.Error {
276-
rng, err := rangeFromPositions(pm.Mapper, line.Start, line.End)
306+
disabledByGOPROXY := strings.Contains(goCmdError, "disabled by GOPROXY=off")
307+
shouldAddDep := strings.Contains(goCmdError, "to add it")
308+
if innermost != nil && (disabledByGOPROXY || shouldAddDep) {
309+
args, err := source.MarshalArgs(fh.URI(), false, []string{fmt.Sprintf("%v@%v", innermost.Path, innermost.Version)})
277310
if err != nil {
278311
return nil
279312
}
280-
disabledByGOPROXY := strings.Contains(goCmdError, "disabled by GOPROXY=off")
281-
shouldAddDep := strings.Contains(goCmdError, "to add it")
282-
if v.Path != "" && (disabledByGOPROXY || shouldAddDep) {
283-
args, err := source.MarshalArgs(fh.URI(), false, []string{fmt.Sprintf("%v@%v", v.Path, v.Version)})
284-
if err != nil {
285-
return nil
286-
}
287-
msg := goCmdError
288-
if disabledByGOPROXY {
289-
msg = fmt.Sprintf("%v@%v has not been downloaded", v.Path, v.Version)
290-
}
291-
return &source.Error{
292-
Message: msg,
293-
Kind: source.ListError,
294-
Range: rng,
295-
URI: fh.URI(),
296-
SuggestedFixes: []source.SuggestedFix{{
297-
Title: fmt.Sprintf("Download %v@%v", v.Path, v.Version),
298-
Command: &protocol.Command{
299-
Title: source.CommandAddDependency.Title,
300-
Command: source.CommandAddDependency.ID(),
301-
Arguments: args,
302-
},
303-
}},
304-
}
313+
msg := goCmdError
314+
if disabledByGOPROXY {
315+
msg = fmt.Sprintf("%v@%v has not been downloaded", innermost.Path, innermost.Version)
305316
}
306317
return &source.Error{
307-
Message: goCmdError,
318+
Message: msg,
319+
Kind: source.ListError,
308320
Range: rng,
309321
URI: fh.URI(),
310-
Kind: source.ListError,
311-
}
312-
}
313-
// Check if there are any require, exclude, or replace statements that
314-
// match this module version.
315-
for _, req := range pm.File.Require {
316-
if req.Mod != v {
317-
continue
318-
}
319-
return toSourceError(req.Syntax)
320-
}
321-
for _, ex := range pm.File.Exclude {
322-
if ex.Mod != v {
323-
continue
322+
SuggestedFixes: []source.SuggestedFix{{
323+
Title: fmt.Sprintf("Download %v@%v", innermost.Path, innermost.Version),
324+
Command: &protocol.Command{
325+
Title: source.CommandAddDependency.Title,
326+
Command: source.CommandAddDependency.ID(),
327+
Arguments: args,
328+
},
329+
}},
324330
}
325-
return toSourceError(ex.Syntax)
326331
}
327-
for _, rep := range pm.File.Replace {
328-
if rep.New != v && rep.Old != v {
329-
continue
330-
}
331-
return toSourceError(rep.Syntax)
332-
}
333-
// No match for the module path was found in the go.mod file.
334-
// Show the error on the module declaration, if one exists.
335-
if pm.File.Module == nil {
336-
return nil
332+
return &source.Error{
333+
Message: goCmdError,
334+
Range: rng,
335+
URI: fh.URI(),
336+
Kind: source.ListError,
337337
}
338-
return toSourceError(pm.File.Module.Syntax)
339338
}
340339

341340
// errorPositionRe matches errors messages of the form <filename>:<line>:<col>,

internal/lsp/cache/snapshot.go

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121

2222
"golang.org/x/mod/modfile"
2323
"golang.org/x/mod/module"
24+
"golang.org/x/mod/semver"
2425
"golang.org/x/tools/go/analysis"
2526
"golang.org/x/tools/go/packages"
2627
"golang.org/x/tools/internal/event"
@@ -323,12 +324,8 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat
323324
case source.LoadWorkspace, source.Normal:
324325
if vendorEnabled {
325326
inv.ModFlag = "vendor"
326-
} else if s.workspaceMode()&usesWorkspaceModule == 0 && !allowModfileModificationOption {
327+
} else if !allowModfileModificationOption {
327328
inv.ModFlag = "readonly"
328-
} else {
329-
// Temporarily allow updates for multi-module workspace mode:
330-
// it doesn't create a go.sum at all. golang/go#42509.
331-
inv.ModFlag = mutableModFlag
332329
}
333330
case source.UpdateUserModFile, source.WriteTemporaryModFile:
334331
inv.ModFlag = mutableModFlag
@@ -1721,6 +1718,9 @@ func BuildGoplsMod(ctx context.Context, root span.URI, s source.Snapshot) (*modf
17211718
func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{}, fs source.FileSource) (*modfile.File, error) {
17221719
file := &modfile.File{}
17231720
file.AddModuleStmt("gopls-workspace")
1721+
// Track the highest Go version, to be set on the workspace module.
1722+
// Fall back to 1.12 -- old versions insist on having some version.
1723+
goVersion := "1.12"
17241724

17251725
paths := make(map[string]span.URI)
17261726
for modURI := range modFiles {
@@ -1739,7 +1739,13 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{},
17391739
if file == nil || parsed.Module == nil {
17401740
return nil, fmt.Errorf("no module declaration for %s", modURI)
17411741
}
1742+
if parsed.Go != nil && semver.Compare(goVersion, parsed.Go.Version) < 0 {
1743+
goVersion = parsed.Go.Version
1744+
}
17421745
path := parsed.Module.Mod.Path
1746+
if _, ok := paths[path]; ok {
1747+
return nil, fmt.Errorf("found module %q twice in the workspace", path)
1748+
}
17431749
paths[path] = modURI
17441750
// If the module's path includes a major version, we expect it to have
17451751
// a matching major version.
@@ -1753,6 +1759,9 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{},
17531759
return nil, err
17541760
}
17551761
}
1762+
if goVersion != "" {
1763+
file.AddGoStmt(goVersion)
1764+
}
17561765
// Go back through all of the modules to handle any of their replace
17571766
// statements.
17581767
for modURI := range modFiles {
@@ -1792,6 +1801,7 @@ func buildWorkspaceModFile(ctx context.Context, modFiles map[span.URI]struct{},
17921801
}
17931802
}
17941803
}
1804+
file.SortBlocks()
17951805
return file, nil
17961806
}
17971807

0 commit comments

Comments
 (0)