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>

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
 	}

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
 }

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

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

internal/lsp/mod/testdata/indirect/go.mod

@@ -1,4 +1,4 @@
-module indirect
+module invalidrequire
 
 go 1.12
 

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

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
+}

internal/lsp/mod/testdata/invalidrequire/go.mod

@@ -0,0 +1,5 @@
+module indirect
+
+go 1.12
+
+require golang.or

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
+}

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

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
+}