internal/lsp: surface diagnostics for invalid go.mod files
This change will surface errors that come from the mod package. It will handle incorrect usages, invalid directives, and other errors that occur when parsing go.mod files.
Updates golang/go#31999
Change-Id: Icd817c02a4b656b2a71914ee60be4dbe2bea062d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/213779
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cache/builtin.go b/internal/lsp/cache/builtin.go
index f9d2fe5..eee8297 100644
--- a/internal/lsp/cache/builtin.go
+++ b/internal/lsp/cache/builtin.go
@@ -7,6 +7,7 @@
import (
"context"
"go/ast"
+ "strings"
"golang.org/x/tools/go/packages"
"golang.org/x/tools/internal/lsp/source"
@@ -35,6 +36,10 @@
func (v *view) buildBuiltinPackage(ctx context.Context) error {
cfg := v.Config(ctx)
pkgs, err := packages.Load(cfg, "builtin")
+ // If the error is related to a go.mod parse error, we want to continue loading.
+ if err != nil && strings.Contains(err.Error(), ".mod:") {
+ return nil
+ }
if err != nil {
return err
}
diff --git a/internal/lsp/cache/parse_mod.go b/internal/lsp/cache/parse_mod.go
index 7c358d3..07b219f 100644
--- a/internal/lsp/cache/parse_mod.go
+++ b/internal/lsp/cache/parse_mod.go
@@ -6,11 +6,16 @@
import (
"context"
+ "regexp"
+ "strconv"
+ "strings"
"golang.org/x/mod/modfile"
+ "golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/lsp/telemetry"
"golang.org/x/tools/internal/memoize"
+ "golang.org/x/tools/internal/telemetry/log"
"golang.org/x/tools/internal/telemetry/trace"
errors "golang.org/x/xerrors"
)
@@ -39,7 +44,7 @@
}
}
-func parseMod(ctx context.Context, fh source.FileHandle) (modifle *modfile.File, err error) {
+func parseMod(ctx context.Context, fh source.FileHandle) (*modfile.File, error) {
ctx, done := trace.StartSpan(ctx, "cache.parseMod", telemetry.File.Of(fh.Identity().URI.Filename()))
defer done()
@@ -49,7 +54,25 @@
}
f, err := modfile.Parse(fh.Identity().URI.Filename(), buf, nil)
if err != nil {
- return nil, err
+ // TODO(golang/go#36486): This can be removed when modfile.Parse returns structured errors.
+ re := regexp.MustCompile(`.*:([\d]+): (.+)`)
+ matches := re.FindStringSubmatch(strings.TrimSpace(err.Error()))
+ if len(matches) < 3 {
+ log.Error(ctx, "could not parse golang/x/mod error message", err)
+ return nil, err
+ }
+ line, e := strconv.Atoi(matches[1])
+ if e != nil {
+ return nil, err
+ }
+ contents := strings.Split(string(buf), "\n")[line-1]
+ return nil, &source.Error{
+ Message: matches[2],
+ Range: protocol.Range{
+ Start: protocol.Position{Line: float64(line - 1), Character: float64(0)},
+ End: protocol.Position{Line: float64(line - 1), Character: float64(len(contents))},
+ },
+ }
}
return f, nil
}
diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go
index eb5ff40..2d8b04e 100644
--- a/internal/lsp/mod/diagnostics.go
+++ b/internal/lsp/mod/diagnostics.go
@@ -9,6 +9,7 @@
import (
"context"
"fmt"
+ "strings"
"golang.org/x/mod/modfile"
"golang.org/x/tools/internal/lsp/protocol"
@@ -36,10 +37,23 @@
cfg := snapshot.View().Config(ctx)
args := append([]string{"mod", "tidy"}, cfg.BuildFlags...)
if _, err := source.InvokeGo(ctx, snapshot.View().Folder().Filename(), cfg.Env, args...); err != nil {
- return source.FileIdentity{}, nil, err
+ // Ignore parse errors here. They'll be handled below.
+ if !strings.Contains(err.Error(), "errors parsing go.mod") {
+ return source.FileIdentity{}, nil, err
+ }
}
realMod, err := snapshot.View().Session().Cache().ParseModHandle(realfh).Parse(ctx)
+ if err, ok := err.(*source.Error); ok {
+ return realfh.Identity(), []source.Diagnostic{
+ {
+ Message: err.Message,
+ Source: "go mod tidy",
+ Range: err.Range,
+ Severity: protocol.SeverityError,
+ },
+ }, nil
+ }
if err != nil {
return source.FileIdentity{}, nil, err
}
@@ -48,8 +62,8 @@
return source.FileIdentity{}, nil, err
}
- reports := []source.Diagnostic{}
// Check indirect vs direct, and removal of dependencies.
+ reports := []source.Diagnostic{}
realReqs := make(map[string]*modfile.Require, len(realMod.Require))
tempReqs := make(map[string]*modfile.Require, len(tempMod.Require))
for _, req := range realMod.Require {
@@ -84,7 +98,7 @@
return realfh.Identity(), reports, nil
}
-// TODO: Check to see if we need to go through internal/span.
+// TODO: Check to see if we need to go through internal/span (for multiple byte characters).
func getPos(pos modfile.Position) protocol.Position {
return protocol.Position{
Line: float64(pos.Line - 1),
diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go
index 558bb77..8f2e17c 100644
--- a/internal/lsp/mod/mod_test.go
+++ b/internal/lsp/mod/mod_test.go
@@ -27,6 +27,8 @@
os.Exit(m.Run())
}
+// TODO(golang/go#36091): This file can be refactored to look like lsp_test.go
+// when marker support gets added for go.mod files.
func TestModfileRemainsUnchanged(t *testing.T) {
ctx := tests.Context(t)
cache := cache.New(nil)
@@ -35,8 +37,6 @@
options.TempModfile = true
options.Env = append(os.Environ(), "GOPACKAGESDRIVER=off", "GOROOT=")
- // TODO: Once we refactor this to work with go/packages/packagestest. We do not
- // need to copy to a temporary directory.
// Make sure to copy the test directory to a temporary directory so we do not
// modify the test code or add go.sum files when we run the tests.
folder, err := copyToTempDir(filepath.Join("testdata", "unchanged"))
@@ -65,6 +65,8 @@
}
}
+// TODO(golang/go#36091): This file can be refactored to look like lsp_test.go
+// when marker support gets added for go.mod files.
func TestDiagnostics(t *testing.T) {
ctx := tests.Context(t)
cache := cache.New(nil)
@@ -81,8 +83,10 @@
testdir: "indirect",
want: []source.Diagnostic{
{
- Message: "golang.org/x/tools should not be an indirect dependency.",
- Source: "go mod tidy",
+ Message: "golang.org/x/tools should not be an indirect dependency.",
+ Source: "go mod tidy",
+ // TODO(golang/go#36091): When marker support gets added for go.mod files, we
+ // can remove these hard coded positions.
Range: protocol.Range{Start: getPos(4, 0), End: getPos(4, 61)},
Severity: protocol.SeverityWarning,
},
@@ -99,10 +103,41 @@
},
},
},
+ {
+ testdir: "invalidrequire",
+ want: []source.Diagnostic{
+ {
+ Message: "usage: require module/path v1.2.3",
+ Source: "go mod tidy",
+ Range: protocol.Range{Start: getPos(4, 0), End: getPos(4, 17)},
+ Severity: protocol.SeverityError,
+ },
+ },
+ },
+ {
+ testdir: "invalidgo",
+ want: []source.Diagnostic{
+ {
+ Message: "usage: go 1.23",
+ Source: "go mod tidy",
+ Range: protocol.Range{Start: getPos(2, 0), End: getPos(2, 4)},
+ Severity: protocol.SeverityError,
+ },
+ },
+ },
+ {
+ testdir: "unknowndirective",
+ want: []source.Diagnostic{
+ {
+ Message: "unknown directive: yo",
+ Source: "go mod tidy",
+ Range: protocol.Range{Start: getPos(6, 0), End: getPos(6, 2)},
+ Severity: protocol.SeverityError,
+ },
+ },
+ },
} {
t.Run(tt.testdir, func(t *testing.T) {
- // TODO: Once we refactor this to work with go/packages/packagestest. We do not
- // need to copy to a temporary directory.
// Make sure to copy the test directory to a temporary directory so we do not
// modify the test code or add go.sum files when we run the tests.
folder, err := copyToTempDir(filepath.Join("testdata", tt.testdir))
diff --git a/internal/lsp/mod/testdata/indirect/go.mod b/internal/lsp/mod/testdata/indirect/go.mod
index 2e5dc13..84e1b53 100644
--- a/internal/lsp/mod/testdata/indirect/go.mod
+++ b/internal/lsp/mod/testdata/indirect/go.mod
@@ -1,4 +1,4 @@
-module indirect
+module invalidrequire
go 1.12
diff --git a/internal/lsp/mod/testdata/invalidgo/go.mod b/internal/lsp/mod/testdata/invalidgo/go.mod
new file mode 100644
index 0000000..ca06b60
--- /dev/null
+++ b/internal/lsp/mod/testdata/invalidgo/go.mod
@@ -0,0 +1,5 @@
+module invalidgo
+
+go 1
+
+require golang.org/x/tools v0.0.0-20191219192050-56b0b28a00f7 // indirect
diff --git a/internal/lsp/mod/testdata/invalidgo/main.go b/internal/lsp/mod/testdata/invalidgo/main.go
new file mode 100644
index 0000000..5577a36
--- /dev/null
+++ b/internal/lsp/mod/testdata/invalidgo/main.go
@@ -0,0 +1,10 @@
+// Package invalidgo does something
+package invalidgo
+
+import (
+ "golang.org/x/tools/go/packages"
+)
+
+func Yo() {
+ var _ packages.Config
+}
diff --git a/internal/lsp/mod/testdata/invalidrequire/go.mod b/internal/lsp/mod/testdata/invalidrequire/go.mod
new file mode 100644
index 0000000..6829b9c
--- /dev/null
+++ b/internal/lsp/mod/testdata/invalidrequire/go.mod
@@ -0,0 +1,5 @@
+module indirect
+
+go 1.12
+
+require golang.or
diff --git a/internal/lsp/mod/testdata/invalidrequire/main.go b/internal/lsp/mod/testdata/invalidrequire/main.go
new file mode 100644
index 0000000..dd24341
--- /dev/null
+++ b/internal/lsp/mod/testdata/invalidrequire/main.go
@@ -0,0 +1,10 @@
+// Package invalidrequire does something
+package invalidrequire
+
+import (
+ "golang.org/x/tools/go/packages"
+)
+
+func Yo() {
+ var _ packages.Config
+}
diff --git a/internal/lsp/mod/testdata/unknowndirective/go.mod b/internal/lsp/mod/testdata/unknowndirective/go.mod
new file mode 100644
index 0000000..4f07729
--- /dev/null
+++ b/internal/lsp/mod/testdata/unknowndirective/go.mod
@@ -0,0 +1,7 @@
+module unknowndirective
+
+go 1.12
+
+require golang.org/x/tools v0.0.0-20191219192050-56b0b28a00f7
+
+yo
diff --git a/internal/lsp/mod/testdata/unknowndirective/main.go b/internal/lsp/mod/testdata/unknowndirective/main.go
new file mode 100644
index 0000000..5ee984e
--- /dev/null
+++ b/internal/lsp/mod/testdata/unknowndirective/main.go
@@ -0,0 +1,10 @@
+// Package unknowndirective does something
+package unknowndirective
+
+import (
+ "golang.org/x/tools/go/packages"
+)
+
+func Yo() {
+ var _ packages.Config
+}
