internal/lsp: extract filenames from go command errors

Update the logic that extracts positions from error messages to handle
optional column numbers, and then use that function when extracting
go command errors. This will be useful in parsing go list errors.

Change-Id: Ie68de1439f002c30f785c0c121c5cec4f2fea727
Reviewed-on: https://go-review.googlesource.com/c/tools/+/272095
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/lsp/cache/load.go

@@ -174,7 +174,7 @@ func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) source.Err
 		if err != nil {
 			continue
 		}
-		srcErr := extractGoCommandError(ctx, s, fh, loadErr)
+		srcErr := s.extractGoCommandError(ctx, s, fh, loadErr.Error())
 		if srcErr == nil {
 			continue
 		}

internal/lsp/cache/mod.go

@@ -12,7 +12,6 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
-	"strconv"
 	"strings"
 	"unicode"
 
@@ -25,11 +24,11 @@ import (
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/memoize"
 	"golang.org/x/tools/internal/span"
-	errors "golang.org/x/xerrors"
 )
 
 const (
-	SyntaxError = "syntax"
+	SyntaxError    = "syntax"
+	GoCommandError = "go command"
 )
 
 type parseModHandle struct {
@@ -74,7 +73,7 @@ func (s *snapshot) ParseMod(ctx context.Context, modFH source.FileHandle) (*sour
 		// Attempt to convert the error to a standardized parse error.
 		var parseErrors []*source.Error
 		if err != nil {
-			if parseErr, extractErr := extractModParseErrors(modFH.URI(), m, err, contents); extractErr == nil {
+			if parseErr, extractErr := extractErrorWithPosition(ctx, err.Error(), s); extractErr == nil {
 				parseErrors = []*source.Error{parseErr}
 			}
 		}
@@ -123,46 +122,6 @@ func sumFilename(modURI span.URI) string {
 	return strings.TrimSuffix(modURI.Filename(), ".mod") + ".sum"
 }
 
-// extractModParseErrors processes the raw errors returned by modfile.Parse,
-// extracting the filenames and line numbers that correspond to the errors.
-func extractModParseErrors(uri span.URI, m *protocol.ColumnMapper, parseErr error, content []byte) (*source.Error, error) {
-	re := regexp.MustCompile(`.*:([\d]+): (.+)`)
-	matches := re.FindStringSubmatch(strings.TrimSpace(parseErr.Error()))
-	if len(matches) < 3 {
-		return nil, errors.Errorf("could not parse go.mod error message: %s", parseErr)
-	}
-	line, err := strconv.Atoi(matches[1])
-	if err != nil {
-		return nil, err
-	}
-	lines := strings.Split(string(content), "\n")
-	if line > len(lines) {
-		return nil, errors.Errorf("could not parse go.mod error message %q, line number %v out of range", content, line)
-	}
-	// The error returned from the modfile package only returns a line number,
-	// so we assume that the diagnostic should be for the entire line.
-	endOfLine := len(lines[line-1])
-	sOffset, err := m.Converter.ToOffset(line, 1)
-	if err != nil {
-		return nil, err
-	}
-	eOffset, err := m.Converter.ToOffset(line, endOfLine)
-	if err != nil {
-		return nil, err
-	}
-	spn := span.New(uri, span.NewPoint(line, 0, sOffset), span.NewPoint(line, endOfLine, eOffset))
-	rng, err := m.Range(spn)
-	if err != nil {
-		return nil, err
-	}
-	return &source.Error{
-		Category: SyntaxError,
-		Message:  matches[2],
-		Range:    rng,
-		URI:      uri,
-	}, nil
-}
-
 // modKey is uniquely identifies cached data for `go mod why` or dependencies
 // to upgrade.
 type modKey struct {
@@ -380,9 +339,14 @@ func containsVendor(modURI span.URI) bool {
 
 var moduleAtVersionRe = regexp.MustCompile(`^(?P<module>.*)@(?P<version>.*)$`)
 
-// ExtractGoCommandError tries to parse errors that come from the go command
+// extractGoCommandError tries to parse errors that come from the go command
 // and shape them into go.mod diagnostics.
-func extractGoCommandError(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, loadErr error) *source.Error {
+func (s *snapshot) extractGoCommandError(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, goCmdError string) *source.Error {
+	// If the error message contains a position, use that. Don't pass a file
+	// handle in, as it might not be the file associated with the error.
+	if srcErr, err := extractErrorWithPosition(ctx, goCmdError, s); err == nil {
+		return srcErr
+	}
 	// We try to match module versions in error messages. Some examples:
 	//
 	//  example.com@v1.2.2: reading example.com/@v/v1.2.2.mod: no such file or directory
@@ -393,7 +357,7 @@ func extractGoCommandError(ctx context.Context, snapshot source.Snapshot, fh sou
 	// that matches module@version. If we're able to find a match, we try to
 	// find anything that matches it in the go.mod file.
 	var v module.Version
-	fields := strings.FieldsFunc(loadErr.Error(), func(r rune) bool {
+	fields := strings.FieldsFunc(goCmdError, func(r rune) bool {
 		return unicode.IsSpace(r) || r == ':'
 	})
 	for _, s := range fields {
@@ -424,7 +388,7 @@ func extractGoCommandError(ctx context.Context, snapshot source.Snapshot, fh sou
 			return nil
 		}
 		return &source.Error{
-			Message: loadErr.Error(),
+			Message: goCmdError,
 			Range:   rng,
 			URI:     fh.URI(),
 		}
@@ -456,3 +420,55 @@ func extractGoCommandError(ctx context.Context, snapshot source.Snapshot, fh sou
 	}
 	return toSourceError(pm.File.Module.Syntax)
 }
+
+// errorPositionRe matches errors messages of the form <filename>:<line>:<col>,
+// where the <col> is optional.
+var errorPositionRe = regexp.MustCompile(`(?P<pos>.*:([\d]+)(:([\d]+))?): (?P<msg>.+)`)
+
+// extractErrorWithPosition returns a structured error with position
+// information for the given unstructured error. If a file handle is provided,
+// the error position will be on that file. This is useful for parse errors,
+// where we already know the file with the error.
+func extractErrorWithPosition(ctx context.Context, goCmdError string, src source.FileSource) (*source.Error, error) {
+	matches := errorPositionRe.FindStringSubmatch(strings.TrimSpace(goCmdError))
+	if len(matches) == 0 {
+		return nil, fmt.Errorf("error message doesn't contain a position")
+	}
+	var pos, msg string
+	for i, name := range errorPositionRe.SubexpNames() {
+		if name == "pos" {
+			pos = matches[i]
+		}
+		if name == "msg" {
+			msg = matches[i]
+		}
+	}
+	spn := span.Parse(pos)
+	fh, err := src.GetFile(ctx, spn.URI())
+	if err != nil {
+		return nil, err
+	}
+	content, err := fh.Read()
+	if err != nil {
+		return nil, err
+	}
+	m := &protocol.ColumnMapper{
+		URI:       spn.URI(),
+		Converter: span.NewContentConverter(spn.URI().Filename(), content),
+		Content:   content,
+	}
+	rng, err := m.Range(spn)
+	if err != nil {
+		return nil, err
+	}
+	category := GoCommandError
+	if fh != nil {
+		category = SyntaxError
+	}
+	return &source.Error{
+		Category: category,
+		Message:  msg,
+		Range:    rng,
+		URI:      spn.URI(),
+	}, nil
+}

internal/lsp/cache/view_test.go

@@ -8,9 +8,13 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
+	"golang.org/x/mod/modfile"
 	"golang.org/x/tools/internal/lsp/fake"
+	"golang.org/x/tools/internal/lsp/protocol"
+	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
 )
 
@@ -101,6 +105,88 @@ module fg
 	}
 }
 
+// This tests the logic used to extract positions from parse and other Go
+// command errors.
+func TestExtractPositionFromError(t *testing.T) {
+	workspace := `
+-- a/go.mod --
+modul a.com
+-- b/go.mod --
+module b.com
+
+go 1.12.hello
+-- c/go.mod --
+module c.com
+
+require a.com master
+`
+	dir, err := fake.Tempdir(workspace)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+
+	tests := []struct {
+		filename string
+		wantRng  protocol.Range
+	}{
+		{
+			filename: "a/go.mod",
+			wantRng:  protocol.Range{},
+		},
+		{
+			filename: "b/go.mod",
+			wantRng: protocol.Range{
+				Start: protocol.Position{Line: 2},
+				End:   protocol.Position{Line: 2},
+			},
+		},
+		{
+			filename: "c/go.mod",
+			wantRng: protocol.Range{
+				Start: protocol.Position{Line: 2},
+				End:   protocol.Position{Line: 2},
+			},
+		},
+	}
+	for _, test := range tests {
+		ctx := context.Background()
+		rel := fake.RelativeTo(dir)
+		uri := span.URIFromPath(rel.AbsPath(test.filename))
+		if source.DetectLanguage("", uri.Filename()) != source.Mod {
+			t.Fatalf("expected only go.mod files")
+		}
+		// Try directly parsing the given, invalid go.mod file. Then, extract a
+		// position from the error message.
+		src := osFileSource{}
+		modFH, err := src.GetFile(ctx, uri)
+		if err != nil {
+			t.Fatal(err)
+		}
+		content, err := modFH.Read()
+		if err != nil {
+			t.Fatal(err)
+		}
+		_, parseErr := modfile.Parse(uri.Filename(), content, nil)
+		if parseErr == nil {
+			t.Fatalf("%s: expected an unparseable go.mod file", uri.Filename())
+		}
+		srcErr, err := extractErrorWithPosition(ctx, parseErr.Error(), src)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if srcErr.URI != uri {
+			t.Errorf("unexpected URI: got %s, wanted %s", srcErr.URI, uri)
+		}
+		if protocol.CompareRange(test.wantRng, srcErr.Range) != 0 {
+			t.Errorf("unexpected range: got %s, wanted %s", srcErr.Range, test.wantRng)
+		}
+		if !strings.HasSuffix(parseErr.Error(), srcErr.Message) {
+			t.Errorf("unexpected message: got %s, wanted %s", srcErr.Message, parseErr)
+		}
+	}
+}
+
 func TestInVendor(t *testing.T) {
 	for _, tt := range []struct {
 		path     string