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 <rstambler@golang.org>
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

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", "-e", "-f", format)
-	stdout, err := source.InvokeGo(ctx, folder, append(env, "GO111MODULE=off"), "list", "-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 {
+	if len(lines) < 2 && stdout.String() != "" {
-		return "", false, errors.Errorf("unexpected stdout: %q", stdout)
+		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")

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 go.mod files.
-	// Publish empty diagnostics for files.
 	s.publishReports(ctx, reports, true)
 }
 

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).

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 != "" {
+			if len(reports) != 1 {
-				t.Error(diff)
+				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)
+				}
 			}
 		})
 	}