diff --git a/internal/lsp/cache/builtin.go b/internal/lsp/cache/builtin.go index f9d2fe52d7..eee8297f2a 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 7c358d3db1..07b219f75b 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 eb5ff40a55..2d8b04e7f5 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 558bb77f69..8f2e17c194 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 2e5dc1384a..84e1b537c0 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 0000000000..ca06b60146 --- /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 0000000000..5577a36e55 --- /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 0000000000..6829b9c21a --- /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 0000000000..dd24341ae8 --- /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 0000000000..4f07729db3 --- /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 0000000000..5ee984e8e4 --- /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 +}