From 544dc8ea2d5f86eca70831850e4d5fe51174a016 Mon Sep 17 00:00:00 2001 From: Rohan Challa Date: Fri, 10 Jan 2020 13:20:44 -0500 Subject: [PATCH] internal/lsp: change go1.14 check to be more lenient This change adds the -e flag to ensure that we get the release tags when we are checking if the go version is at least 1.14. This also adjusts the check to be more lenient when it comes to processing the output of the version check. This also fixes another issue where if the version is not 1.14 we were publishing empty diagnostics for an empty uri. This arose because we did not check if there was a valid go.mod file before we published the reports. Fixes golang/go#36488 Change-Id: I5a8057c153f49167811e2bf8e4ade52bf6171d6b Reviewed-on: https://go-review.googlesource.com/c/tools/+/214290 Reviewed-by: Rebecca Stambler Run-TryBot: Rohan Challa TryBot-Result: Gobot Gobot --- internal/lsp/cache/modfiles.go | 13 ++++++++----- internal/lsp/diagnostics.go | 5 ++--- internal/lsp/mod/diagnostics.go | 29 ++++++++++++++++------------- internal/lsp/mod/mod_test.go | 17 +++++++++++------ 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/internal/lsp/cache/modfiles.go b/internal/lsp/cache/modfiles.go index 63403d01e1..d45a5ca9db 100644 --- a/internal/lsp/cache/modfiles.go +++ b/internal/lsp/cache/modfiles.go @@ -20,16 +20,19 @@ import ( // This function will return the main go.mod file for this folder if it exists and whether the -modfile // flag exists for this version of go. func modfileFlagExists(ctx context.Context, folder string, env []string) (string, bool, error) { - // Borrowed from (internal/imports/mod.go:620) + // Check the Go version by running go list with GO111MODULE=off. + // If the output is anything other than "go1.14\n", assume -modfile is not supported. + // Borrowed from internal/imports/mod.go:620 const format = `{{range context.ReleaseTags}}{{if eq . "go1.14"}}{{.}}{{end}}{{end}}` - // Check the go version by running "go list" with modules off. - stdout, err := source.InvokeGo(ctx, folder, append(env, "GO111MODULE=off"), "list", "-f", format) + stdout, err := source.InvokeGo(ctx, folder, append(env, "GO111MODULE=off"), "list", "-e", "-f", format) if err != nil { return "", false, err } + // If the output is not go1.14 or an empty string, then it could be an error. lines := strings.Split(stdout.String(), "\n") - if len(lines) < 2 { - return "", false, errors.Errorf("unexpected stdout: %q", stdout) + if len(lines) < 2 && stdout.String() != "" { + log.Error(ctx, "unexpected stdout when checking for go1.14", errors.Errorf("%q", stdout), telemetry.Directory.Of(folder)) + return "", false, nil } // Get the go.mod file associated with this module. b, err := source.InvokeGo(ctx, folder, env, "env", "GOMOD") diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index b359ecf87d..3e5068ef13 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -69,15 +69,14 @@ func (s *Server) diagnoseModfile(snapshot source.Snapshot) { ctx, done := trace.StartSpan(ctx, "lsp:background-worker") defer done() - f, diags, err := mod.Diagnostics(ctx, snapshot) + reports, err := mod.Diagnostics(ctx, snapshot) if err != nil { if err != context.Canceled { log.Error(ctx, "diagnoseModfile: could not generate diagnostics", err) } return } - reports := map[source.FileIdentity][]source.Diagnostic{f: diags} - // Publish empty diagnostics for files. + // Publish empty diagnostics for go.mod files. s.publishReports(ctx, reports, true) } diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index 2d8b04e7f5..850411f94f 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -18,15 +18,15 @@ import ( "golang.org/x/tools/internal/telemetry/trace" ) -func Diagnostics(ctx context.Context, snapshot source.Snapshot) (source.FileIdentity, []source.Diagnostic, error) { +func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.FileIdentity][]source.Diagnostic, error) { // TODO: We will want to support diagnostics for go.mod files even when the -modfile flag is turned off. realfh, tempfh, err := snapshot.ModFiles(ctx) if err != nil { - return source.FileIdentity{}, nil, err + return nil, err } // Check the case when the tempModfile flag is turned off. if realfh == nil || tempfh == nil { - return source.FileIdentity{}, nil, nil + return nil, nil } ctx, done := trace.StartSpan(ctx, "modfiles.Diagnostics", telemetry.File.Of(realfh.Identity().URI)) @@ -39,31 +39,34 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (source.FileIden if _, err := source.InvokeGo(ctx, snapshot.View().Folder().Filename(), cfg.Env, args...); err != nil { // Ignore parse errors here. They'll be handled below. if !strings.Contains(err.Error(), "errors parsing go.mod") { - return source.FileIdentity{}, nil, err + return nil, err } } realMod, err := snapshot.View().Session().Cache().ParseModHandle(realfh).Parse(ctx) + // If the go.mod file fails to parse, return errors right away. if err, ok := err.(*source.Error); ok { - return realfh.Identity(), []source.Diagnostic{ - { + return map[source.FileIdentity][]source.Diagnostic{ + realfh.Identity(): []source.Diagnostic{{ Message: err.Message, - Source: "go mod tidy", + Source: "syntax", Range: err.Range, Severity: protocol.SeverityError, - }, + }}, }, nil } if err != nil { - return source.FileIdentity{}, nil, err + return nil, err } tempMod, err := snapshot.View().Session().Cache().ParseModHandle(tempfh).Parse(ctx) if err != nil { - return source.FileIdentity{}, nil, err + return nil, err } // Check indirect vs direct, and removal of dependencies. - reports := []source.Diagnostic{} + reports := map[source.FileIdentity][]source.Diagnostic{ + realfh.Identity(): []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 { @@ -93,9 +96,9 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (source.FileIden diag.Message = fmt.Sprintf("%s should not be an indirect dependency.", dep) } } - reports = append(reports, *diag) + reports[realfh.Identity()] = append(reports[realfh.Identity()], *diag) } - return realfh.Identity(), reports, nil + return reports, nil } // TODO: Check to see if we need to go through internal/span (for multiple byte characters). diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go index 8f2e17c194..76ed109a06 100644 --- a/internal/lsp/mod/mod_test.go +++ b/internal/lsp/mod/mod_test.go @@ -108,7 +108,7 @@ func TestDiagnostics(t *testing.T) { want: []source.Diagnostic{ { Message: "usage: require module/path v1.2.3", - Source: "go mod tidy", + Source: "syntax", Range: protocol.Range{Start: getPos(4, 0), End: getPos(4, 17)}, Severity: protocol.SeverityError, }, @@ -119,7 +119,7 @@ func TestDiagnostics(t *testing.T) { want: []source.Diagnostic{ { Message: "usage: go 1.23", - Source: "go mod tidy", + Source: "syntax", Range: protocol.Range{Start: getPos(2, 0), End: getPos(2, 4)}, Severity: protocol.SeverityError, }, @@ -130,7 +130,7 @@ func TestDiagnostics(t *testing.T) { want: []source.Diagnostic{ { Message: "unknown directive: yo", - Source: "go mod tidy", + Source: "syntax", Range: protocol.Range{Start: getPos(6, 0), End: getPos(6, 2)}, Severity: protocol.SeverityError, }, @@ -153,12 +153,17 @@ func TestDiagnostics(t *testing.T) { if !hasTempModfile(ctx, snapshot) { return } - fileID, got, err := mod.Diagnostics(ctx, snapshot) + reports, err := mod.Diagnostics(ctx, snapshot) if err != nil { t.Fatal(err) } - if diff := tests.DiffDiagnostics(fileID.URI, tt.want, got); diff != "" { - t.Error(diff) + if len(reports) != 1 { + t.Errorf("expected 1 fileHandle, got %d", len(reports)) + } + for fh, got := range reports { + if diff := tests.DiffDiagnostics(fh.URI, tt.want, got); diff != "" { + t.Error(diff) + } } }) }