internal/lsp/cache: create infra for caching go.mod diagnostics

This change creates the infrastructure for caching diagnostics. Right now it uses a naive key of the entire snapshot which does not really add any benefit from using the cache. It also moves some code from inside the internal/lsp/mod to internal/lsp/cache.

Updates golang/go#31999

Change-Id: I599ad281baa062448d33ca0ea88ae6e09e10d73e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215021
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/modfiles.go

@@ -24,22 +24,6 @@ func (v *view) modFiles(ctx context.Context) (span.URI, span.URI, error) {
 	if v.mod == nil {
 		return "", "", nil
 	}
-	// Copy the real go.mod file content into the temp go.mod file.
-	origFile, err := os.Open(v.mod.realMod.Filename())
-	if err != nil {
-		return "", "", err
-	}
-	defer origFile.Close()
-
-	tempFile, err := os.OpenFile(v.mod.tempMod.Filename(), os.O_WRONLY, os.ModePerm)
-	if err != nil {
-		return "", "", err
-	}
-	defer tempFile.Close()
-
-	if _, err := io.Copy(tempFile, origFile); err != nil {
-		return "", "", err
-	}
 	return v.mod.realMod, v.mod.tempMod, nil
 }
 

internal/lsp/cache/parse_mod.go

@@ -6,11 +6,15 @@ package cache
 
 import (
 	"context"
+	"fmt"
+	"io/ioutil"
+	"os"
 	"regexp"
 	"strconv"
 	"strings"
 
 	"golang.org/x/mod/modfile"
+	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/lsp/telemetry"
@@ -21,67 +25,27 @@ import (
 	errors "golang.org/x/xerrors"
 )
 
+const ModTidyError = "go mod tidy"
+const SyntaxError = "syntax"
+
+type parseModKey struct {
+	snapshot source.Snapshot
+	cfg      string
+}
+
 type parseModHandle struct {
 	handle *memoize.Handle
 	file   source.FileHandle
+	cfg    *packages.Config
 }
 
 type parseModData struct {
 	memoize.NoCopy
 
-	modfile *modfile.File
+	modfile     *modfile.File
-	mapper  *protocol.ColumnMapper
+	mapper      *protocol.ColumnMapper
-	err     error
+	parseErrors []source.Error
-}
+	err         error
-
-func (c *cache) ParseModHandle(fh source.FileHandle) source.ParseModHandle {
-	h := c.store.Bind(fh.Identity(), func(ctx context.Context) interface{} {
-		data := &parseModData{}
-		data.modfile, data.mapper, data.err = parseMod(ctx, fh)
-		return data
-	})
-	return &parseModHandle{
-		handle: h,
-		file:   fh,
-	}
-}
-
-func parseMod(ctx context.Context, fh source.FileHandle) (*modfile.File, *protocol.ColumnMapper, error) {
-	ctx, done := trace.StartSpan(ctx, "cache.parseMod", telemetry.File.Of(fh.Identity().URI.Filename()))
-	defer done()
-
-	buf, _, err := fh.Read(ctx)
-	if err != nil {
-		return nil, nil, err
-	}
-	parsed, err := modfile.Parse(fh.Identity().URI.Filename(), buf, nil)
-	if err != nil {
-		// 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, nil, err
-		}
-		line, e := strconv.Atoi(matches[1])
-		if e != nil {
-			return nil, nil, err
-		}
-		contents := strings.Split(string(buf), "\n")[line-1]
-		return nil, 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))},
-			},
-		}
-	}
-	m := &protocol.ColumnMapper{
-		URI:       fh.Identity().URI,
-		Converter: span.NewContentConverter(fh.Identity().URI.Filename(), buf),
-		Content:   buf,
-	}
-	return parsed, m, nil
 }
 
 func (pgh *parseModHandle) String() string {
@@ -92,11 +56,235 @@ func (pgh *parseModHandle) File() source.FileHandle {
 	return pgh.file
 }
 
-func (pgh *parseModHandle) Parse(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, error) {
+func (pgh *parseModHandle) Parse(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, []source.Error, error) {
 	v := pgh.handle.Get(ctx)
 	if v == nil {
-		return nil, nil, errors.Errorf("no parsed file for %s", pgh.File().Identity().URI)
+		return nil, nil, nil, errors.Errorf("no parsed file for %s", pgh.File().Identity().URI)
 	}
 	data := v.(*parseModData)
-	return data.modfile, data.mapper, data.err
+	return data.modfile, data.mapper, data.parseErrors, data.err
+}
+
+func (s *snapshot) ParseModHandle(ctx context.Context, fh source.FileHandle) source.ParseModHandle {
+	realfh, tempfh, err := s.ModFiles(ctx)
+	cfg := s.View().Config(ctx)
+	folder := s.View().Folder().Filename()
+
+	key := parseModKey{
+		snapshot: s,
+		cfg:      hashConfig(cfg),
+	}
+	h := s.view.session.cache.store.Bind(key, func(ctx context.Context) interface{} {
+		data := &parseModData{}
+		if err != nil {
+			data.err = err
+			return data
+		}
+		// Check the case when the tempModfile flag is turned off.
+		if realfh == nil || tempfh == nil {
+			return data
+		}
+		data.modfile, data.mapper, data.parseErrors, data.err = goModFileDiagnostics(ctx, realfh, tempfh, cfg, folder)
+		return data
+	})
+	return &parseModHandle{
+		handle: h,
+		file:   fh,
+		cfg:    cfg,
+	}
+}
+
+func goModFileDiagnostics(ctx context.Context, realfh, tempfh source.FileHandle, cfg *packages.Config, folder string) (*modfile.File, *protocol.ColumnMapper, []source.Error, error) {
+	ctx, done := trace.StartSpan(ctx, "cache.parseMod", telemetry.File.Of(realfh.Identity().URI.Filename()))
+	defer done()
+
+	// Copy the real go.mod file content into the temp go.mod file.
+	contents, err := ioutil.ReadFile(realfh.Identity().URI.Filename())
+	if err != nil {
+		return nil, nil, nil, err
+	}
+	if err := ioutil.WriteFile(tempfh.Identity().URI.Filename(), contents, os.ModePerm); err != nil {
+		return nil, nil, nil, err
+	}
+
+	// We want to run "go mod tidy" to be able to diff between the real and the temp files.
+	args := append([]string{"mod", "tidy"}, cfg.BuildFlags...)
+	if _, err := source.InvokeGo(ctx, folder, cfg.Env, args...); err != nil {
+		// Ignore parse errors here. They'll be handled below.
+		if !strings.Contains(err.Error(), "errors parsing go.mod") {
+			return nil, nil, nil, err
+		}
+	}
+
+	realMod, m, parseErr, err := parseModFile(ctx, realfh)
+	if parseErr != nil {
+		return nil, nil, []source.Error{*parseErr}, nil
+	}
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	tempMod, _, _, err := parseModFile(ctx, tempfh)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	errors, err := modRequireErrors(realfh, m, realMod, tempMod)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+	return realMod, m, errors, nil
+}
+
+func modParseErrors(ctx context.Context, uri span.URI, m *protocol.ColumnMapper, modTidyErr error, buf []byte) (source.Error, error) {
+	re := regexp.MustCompile(`.*:([\d]+): (.+)`)
+	matches := re.FindStringSubmatch(strings.TrimSpace(modTidyErr.Error()))
+	if len(matches) < 3 {
+		log.Error(ctx, "could not parse golang/x/mod error message", modTidyErr)
+		return source.Error{}, modTidyErr
+	}
+	line, err := strconv.Atoi(matches[1])
+	if err != nil {
+		return source.Error{}, modTidyErr
+	}
+	lines := strings.Split(string(buf), "\n")
+	if len(lines) <= line {
+		return source.Error{}, errors.Errorf("could not parse goland/x/mod error message, line number out of range")
+	}
+	// Get the length of the line that the error is present on.
+	endOfLine := len(lines[line-1])
+	sOffset, err := m.Converter.ToOffset(line, 0)
+	if err != nil {
+		return source.Error{}, err
+	}
+	eOffset, err := m.Converter.ToOffset(line, endOfLine)
+	if err != nil {
+		return source.Error{}, err
+	}
+	spn := span.New(uri, span.NewPoint(line, 0, sOffset), span.NewPoint(line, endOfLine, eOffset))
+	rng, err := m.Range(spn)
+	if err != nil {
+		return source.Error{}, err
+	}
+	return source.Error{
+		Category: SyntaxError,
+		Message:  matches[2],
+		Range:    rng,
+		URI:      uri,
+	}, nil
+}
+
+func modRequireErrors(realfh source.FileHandle, m *protocol.ColumnMapper, realMod, tempMod *modfile.File) ([]source.Error, error) {
+	realReqs := make(map[string]*modfile.Require, len(realMod.Require))
+	tempReqs := make(map[string]*modfile.Require, len(tempMod.Require))
+	for _, req := range realMod.Require {
+		realReqs[req.Mod.Path] = req
+	}
+	for _, req := range tempMod.Require {
+		realReq := realReqs[req.Mod.Path]
+		if realReq != nil && realReq.Indirect == req.Indirect {
+			delete(realReqs, req.Mod.Path)
+		}
+		tempReqs[req.Mod.Path] = req
+	}
+
+	var errors []source.Error
+	for _, req := range realReqs {
+		if req.Syntax == nil {
+			continue
+		}
+		dep := req.Mod.Path
+		// Handle dependencies that are incorrectly labeled indirect and vice versa.
+		if tempReqs[dep] != nil && req.Indirect != tempReqs[dep].Indirect {
+			directErr, err := modDirectnessErrors(realfh, m, req)
+			if err != nil {
+				return nil, err
+			}
+			errors = append(errors, directErr)
+		}
+		// Handle unused dependencies.
+		if tempReqs[dep] == nil {
+			rng, err := rangeFromPositions(realfh.Identity().URI, m, req.Syntax.Start, req.Syntax.End)
+			if err != nil {
+				return nil, err
+			}
+			errors = append(errors, source.Error{
+				Category: ModTidyError,
+				Message:  fmt.Sprintf("%s is not used in this module.", dep),
+				Range:    rng,
+				URI:      realfh.Identity().URI,
+			})
+		}
+	}
+	return errors, nil
+}
+
+func modDirectnessErrors(fh source.FileHandle, m *protocol.ColumnMapper, req *modfile.Require) (source.Error, error) {
+	rng, err := rangeFromPositions(fh.Identity().URI, m, req.Syntax.Start, req.Syntax.End)
+	if err != nil {
+		return source.Error{}, err
+	}
+	if req.Indirect {
+		// If the dependency should be direct, just highlight the // indirect.
+		if comments := req.Syntax.Comment(); comments != nil && len(comments.Suffix) > 0 {
+			end := comments.Suffix[0].Start
+			end.LineRune += len(comments.Suffix[0].Token)
+			end.Byte += len([]byte(comments.Suffix[0].Token))
+			rng, err = rangeFromPositions(fh.Identity().URI, m, comments.Suffix[0].Start, end)
+			if err != nil {
+				return source.Error{}, err
+			}
+		}
+		return source.Error{
+			Category: ModTidyError,
+			Message:  fmt.Sprintf("%s should be a direct dependency.", req.Mod.Path),
+			Range:    rng,
+			URI:      fh.Identity().URI,
+		}, nil
+	}
+	return source.Error{
+		Category: ModTidyError,
+		Message:  fmt.Sprintf("%s should be an indirect dependency.", req.Mod.Path),
+		Range:    rng,
+		URI:      fh.Identity().URI,
+	}, nil
+}
+
+func parseModFile(ctx context.Context, fh source.FileHandle) (*modfile.File, *protocol.ColumnMapper, *source.Error, error) {
+	contents, _, err := fh.Read(ctx)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+	m := &protocol.ColumnMapper{
+		URI:       fh.Identity().URI,
+		Converter: span.NewContentConverter(fh.Identity().URI.Filename(), contents),
+		Content:   contents,
+	}
+	parsed, err := modfile.Parse(fh.Identity().URI.Filename(), contents, nil)
+	if err != nil {
+		parseErr, err := modParseErrors(ctx, fh.Identity().URI, m, err, contents)
+		return nil, nil, &parseErr, err
+	}
+	return parsed, m, nil, nil
+}
+
+func rangeFromPositions(uri span.URI, m *protocol.ColumnMapper, s, e modfile.Position) (protocol.Range, error) {
+	line, col, err := m.Converter.ToPosition(s.Byte)
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	start := span.NewPoint(line, col, s.Byte)
+
+	line, col, err = m.Converter.ToPosition(e.Byte)
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	end := span.NewPoint(line, col, e.Byte)
+
+	spn := span.New(uri, start, end)
+	rng, err := m.Range(spn)
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	return rng, nil
 }

internal/lsp/mod/diagnostics.go

@@ -11,11 +11,9 @@ import (
 	"fmt"
 	"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/span"
 	"golang.org/x/tools/internal/telemetry/trace"
 )
 
@@ -29,132 +27,30 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.File
 	if realfh == nil || tempfh == nil {
 		return nil, nil
 	}
-
 	ctx, done := trace.StartSpan(ctx, "modfiles.Diagnostics", telemetry.File.Of(realfh.Identity().URI))
 	defer done()
 
-	// If the view has a temporary go.mod file, we want to run "go mod tidy" to be able to
+	_, _, parseErrors, err := snapshot.ParseModHandle(ctx, realfh).Parse(ctx)
-	// diff between the real and the temp files.
-	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 {
-		// Ignore parse errors here. They'll be handled below.
-		if !strings.Contains(err.Error(), "errors parsing go.mod") {
-			return nil, err
-		}
-	}
-
-	realMod, m, 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 map[source.FileIdentity][]source.Diagnostic{
-			realfh.Identity(): []source.Diagnostic{{
-				Message:  err.Message,
-				Source:   "syntax",
-				Range:    err.Range,
-				Severity: protocol.SeverityError,
-			}},
-		}, nil
-	}
-	if err != nil {
-		return nil, err
-	}
-	tempMod, _, err := snapshot.View().Session().Cache().ParseModHandle(tempfh).Parse(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	// Check indirect vs direct, and removal of dependencies.
 	reports := map[source.FileIdentity][]source.Diagnostic{
 		realfh.Identity(): []source.Diagnostic{},
 	}
-	realReqs := make(map[string]*modfile.Require, len(realMod.Require))
+	for _, e := range parseErrors {
-	tempReqs := make(map[string]*modfile.Require, len(tempMod.Require))
+		diag := source.Diagnostic{
-	for _, req := range realMod.Require {
+			Message:        e.Message,
-		realReqs[req.Mod.Path] = req
+			Range:          e.Range,
-	}
+			SuggestedFixes: e.SuggestedFixes,
-	for _, req := range tempMod.Require {
+			Source:         e.Category,
-		realReq := realReqs[req.Mod.Path]
-		if realReq != nil && realReq.Indirect == req.Indirect {
-			delete(realReqs, req.Mod.Path)
 		}
-		tempReqs[req.Mod.Path] = req
+		if e.Category == "syntax" {
-	}
+			diag.Severity = protocol.SeverityError
-	for _, req := range realReqs {
+		} else {
-		if req.Syntax == nil {
+			diag.Severity = protocol.SeverityWarning
-			continue
 		}
-		dep := req.Mod.Path
+		reports[realfh.Identity()] = append(reports[realfh.Identity()], diag)
-
-		rng, err := getRangeFromPositions(m, realfh, req.Syntax.Start, req.Syntax.End)
-		if err != nil {
-			return nil, err
-		}
-		var diag *source.Diagnostic
-
-		if tempReqs[dep] != nil && req.Indirect != tempReqs[dep].Indirect {
-			// Show diagnostics for dependencies that are incorrectly labeled indirect.
-			if req.Indirect {
-				var fix []source.SuggestedFix
-				// If the dependency should not be indirect, just highlight the // indirect.
-				if comments := req.Syntax.Comment(); comments != nil && len(comments.Suffix) > 0 {
-					end := comments.Suffix[0].Start
-					end.LineRune += len(comments.Suffix[0].Token)
-					end.Byte += len([]byte(comments.Suffix[0].Token))
-
-					rng, err = getRangeFromPositions(m, realfh, comments.Suffix[0].Start, end)
-					if err != nil {
-						return nil, err
-					}
-					fix = []source.SuggestedFix{
-						{
-							Title: "Remove indirect",
-							Edits: map[span.URI][]protocol.TextEdit{realfh.Identity().URI: []protocol.TextEdit{
-								{
-									Range:   rng,
-									NewText: "",
-								},
-							}},
-						},
-					}
-				}
-				diag = &source.Diagnostic{
-					Message:        fmt.Sprintf("%s should be a direct dependency.", dep),
-					Range:          rng,
-					SuggestedFixes: fix,
-					Source:         "go mod tidy",
-					Severity:       protocol.SeverityWarning,
-				}
-			} else {
-				diag = &source.Diagnostic{
-					Message:  fmt.Sprintf("%s should be an indirect dependency.", dep),
-					Range:    rng,
-					Source:   "go mod tidy",
-					Severity: protocol.SeverityWarning,
-				}
-			}
-		}
-		// Handle unused dependencies.
-		if tempReqs[dep] == nil {
-			diag = &source.Diagnostic{
-				Message: fmt.Sprintf("%s is not used in this module.", dep),
-				Range:   rng,
-				SuggestedFixes: []source.SuggestedFix{
-					{
-						Title: fmt.Sprintf("Remove %s.", dep),
-						Edits: map[span.URI][]protocol.TextEdit{realfh.Identity().URI: []protocol.TextEdit{
-							{
-								Range:   rng,
-								NewText: "",
-							},
-						}},
-					},
-				},
-				Source:   "go mod tidy",
-				Severity: protocol.SeverityWarning,
-			}
-		}
-		reports[realfh.Identity()] = append(reports[realfh.Identity()], *diag)
 	}
 	return reports, nil
 }
@@ -199,42 +95,3 @@ func SuggestedFixes(fh source.FileHandle, diags []protocol.Diagnostic) []protoco
 	}
 	return actions
 }
-
-func getEndOfLine(req *modfile.Require, m *protocol.ColumnMapper) (span.Point, error) {
-	comments := req.Syntax.Comment()
-	if comments == nil {
-		return positionToPoint(m, req.Syntax.End)
-	}
-	suffix := comments.Suffix
-	if len(suffix) == 0 {
-		return positionToPoint(m, req.Syntax.End)
-	}
-	end := suffix[0].Start
-	end.LineRune += len(suffix[0].Token)
-	return positionToPoint(m, end)
-}
-
-func getRangeFromPositions(m *protocol.ColumnMapper, fh source.FileHandle, s, e modfile.Position) (protocol.Range, error) {
-	start, err := positionToPoint(m, s)
-	if err != nil {
-		return protocol.Range{}, err
-	}
-	end, err := positionToPoint(m, e)
-	if err != nil {
-		return protocol.Range{}, err
-	}
-	spn := span.New(fh.Identity().URI, start, end)
-	rng, err := m.Range(spn)
-	if err != nil {
-		return protocol.Range{}, err
-	}
-	return rng, nil
-}
-
-func positionToPoint(m *protocol.ColumnMapper, pos modfile.Position) (span.Point, error) {
-	line, col, err := m.Converter.ToPosition(pos.Byte)
-	if err != nil {
-		return span.Point{}, err
-	}
-	return span.NewPoint(line, col, pos.Byte), nil
-}

internal/lsp/mod/mod_test.go

@@ -106,7 +106,7 @@ func TestDiagnostics(t *testing.T) {
 				{
 					Message:  "usage: require module/path v1.2.3",
 					Source:   "syntax",
-					Range:    protocol.Range{Start: getRawPos(4, 0), End: getRawPos(4, 17)},
+					Range:    protocol.Range{Start: getRawPos(4, 0), End: getRawPos(4, 16)},
 					Severity: protocol.SeverityError,
 				},
 			},
@@ -117,7 +117,7 @@ func TestDiagnostics(t *testing.T) {
 				{
 					Message:  "usage: go 1.23",
 					Source:   "syntax",
-					Range:    protocol.Range{Start: getRawPos(2, 0), End: getRawPos(2, 4)},
+					Range:    protocol.Range{Start: getRawPos(2, 0), End: getRawPos(2, 3)},
 					Severity: protocol.SeverityError,
 				},
 			},
@@ -128,7 +128,7 @@ func TestDiagnostics(t *testing.T) {
 				{
 					Message:  "unknown directive: yo",
 					Source:   "syntax",
-					Range:    protocol.Range{Start: getRawPos(6, 0), End: getRawPos(6, 2)},
+					Range:    protocol.Range{Start: getRawPos(6, 0), End: getRawPos(6, 1)},
 					Severity: protocol.SeverityError,
 				},
 			},

internal/lsp/source/view.go

@@ -40,6 +40,10 @@ type Snapshot interface {
 	// ModFiles returns the FileHandles of the go.mod files attached to the view associated with this snapshot.
 	ModFiles(ctx context.Context) (FileHandle, FileHandle, error)
 
+	// ParseModHandle returns a ParseModHandle for the given go.mod file handle.
+	// This function can have no data or error if there is no modfile detected.
+	ParseModHandle(ctx context.Context, fh FileHandle) ParseModHandle
+
 	// PackageHandles returns the PackageHandles for the packages that this file
 	// belongs to.
 	PackageHandles(ctx context.Context, fh FileHandle) ([]PackageHandle, error)
@@ -216,9 +220,6 @@ type Cache interface {
 	// FileSet returns the shared fileset used by all files in the system.
 	FileSet() *token.FileSet
 
-	// ParseGoHandle returns a go.mod ParseGoHandle for the given file handle.
-	ParseModHandle(fh FileHandle) ParseModHandle
-
 	// ParseGoHandle returns a ParseGoHandle for the given file handle.
 	ParseGoHandle(fh FileHandle, mode ParseMode) ParseGoHandle
 }
@@ -252,7 +253,7 @@ type ParseModHandle interface {
 
 	// Parse returns the parsed modifle for the go.mod file.
 	// If the file is not available, returns nil and an error.
-	Parse(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, error)
+	Parse(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, []Error, error)
 }
 
 // ParseMode controls the content of the AST produced when parsing a source file.