From 30cae5f2fb06c7657da3a9e33b9bd82bf5fc45c3 Mon Sep 17 00:00:00 2001 From: Rohan Challa Date: Tue, 17 Dec 2019 16:13:33 -0500 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/builtin.go | 5 ++ internal/lsp/cache/parse_mod.go | 27 ++++++++++- internal/lsp/mod/diagnostics.go | 20 ++++++-- internal/lsp/mod/mod_test.go | 47 ++++++++++++++++--- internal/lsp/mod/testdata/indirect/go.mod | 2 +- internal/lsp/mod/testdata/invalidgo/go.mod | 5 ++ internal/lsp/mod/testdata/invalidgo/main.go | 10 ++++ .../lsp/mod/testdata/invalidrequire/go.mod | 5 ++ .../lsp/mod/testdata/invalidrequire/main.go | 10 ++++ .../lsp/mod/testdata/unknowndirective/go.mod | 7 +++ .../lsp/mod/testdata/unknowndirective/main.go | 10 ++++ 11 files changed, 136 insertions(+), 12 deletions(-) create mode 100644 internal/lsp/mod/testdata/invalidgo/go.mod create mode 100644 internal/lsp/mod/testdata/invalidgo/main.go create mode 100644 internal/lsp/mod/testdata/invalidrequire/go.mod create mode 100644 internal/lsp/mod/testdata/invalidrequire/main.go create mode 100644 internal/lsp/mod/testdata/unknowndirective/go.mod create mode 100644 internal/lsp/mod/testdata/unknowndirective/main.go diff --git a/internal/lsp/cache/builtin.go b/internal/lsp/cache/builtin.go index f9d2fe52d76..eee8297f2a6 100644 --- a/internal/lsp/cache/builtin.go +++ b/internal/lsp/cache/builtin.go @@ -7,6 +7,7 @@ package cache import ( "context" "go/ast" + "strings" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/lsp/source" @@ -35,6 +36,10 @@ func (b *builtinPkg) CompiledGoFiles() []source.ParseGoHandle { 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 7c358d3db17..07b219f75be 100644 --- a/internal/lsp/cache/parse_mod.go +++ b/internal/lsp/cache/parse_mod.go @@ -6,11 +6,16 @@ package cache 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 (c *cache) ParseModHandle(fh source.FileHandle) source.ParseModHandle { } } -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 @@ func parseMod(ctx context.Context, fh source.FileHandle) (modifle *modfile.File, } 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 eb5ff40a55b..2d8b04e7f54 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -9,6 +9,7 @@ package mod import ( "context" "fmt" + "strings" "golang.org/x/mod/modfile" "golang.org/x/tools/internal/lsp/protocol" @@ -36,10 +37,23 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (source.FileIden 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 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (source.FileIden 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 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (source.FileIden 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 558bb77f69e..8f2e17c1949 100644 --- a/internal/lsp/mod/mod_test.go +++ b/internal/lsp/mod/mod_test.go @@ -27,6 +27,8 @@ func TestMain(m *testing.M) { 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 @@ func TestModfileRemainsUnchanged(t *testing.T) { 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 @@ func TestModfileRemainsUnchanged(t *testing.T) { } } +// 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 @@ func TestDiagnostics(t *testing.T) { 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 @@ func TestDiagnostics(t *testing.T) { }, }, }, + { + 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 2e5dc1384a3..84e1b537c09 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 00000000000..ca06b601463 --- /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 00000000000..5577a36e553 --- /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 00000000000..6829b9c21ab --- /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 00000000000..dd24341ae86 --- /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 00000000000..4f07729db3c --- /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 00000000000..5ee984e8e46 --- /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 +}