From bd79bb77d62b3c5b416a5122d26397956f4651fb Mon Sep 17 00:00:00 2001 From: Rohan Challa Date: Thu, 16 Jan 2020 14:32:09 -0500 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/modfiles.go | 16 -- internal/lsp/cache/parse_mod.go | 300 ++++++++++++++++++++++++++------ internal/lsp/mod/diagnostics.go | 167 ++---------------- internal/lsp/mod/mod_test.go | 6 +- internal/lsp/source/view.go | 9 +- 5 files changed, 264 insertions(+), 234 deletions(-) diff --git a/internal/lsp/cache/modfiles.go b/internal/lsp/cache/modfiles.go index 4797ba34b0..9725358bf6 100644 --- a/internal/lsp/cache/modfiles.go +++ b/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 } diff --git a/internal/lsp/cache/parse_mod.go b/internal/lsp/cache/parse_mod.go index cddf09bee8..b0f44c9346 100644 --- a/internal/lsp/cache/parse_mod.go +++ b/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 - mapper *protocol.ColumnMapper - 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 + modfile *modfile.File + mapper *protocol.ColumnMapper + parseErrors []source.Error + err error } 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 } diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index b7cd8407dd..5b40fd612e 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/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 - // 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) + _, _, parseErrors, err := snapshot.ParseModHandle(ctx, realfh).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)) - 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) + for _, e := range parseErrors { + diag := source.Diagnostic{ + Message: e.Message, + Range: e.Range, + SuggestedFixes: e.SuggestedFixes, + Source: e.Category, } - tempReqs[req.Mod.Path] = req - } - for _, req := range realReqs { - if req.Syntax == nil { - continue + if e.Category == "syntax" { + diag.Severity = protocol.SeverityError + } else { + diag.Severity = protocol.SeverityWarning } - dep := req.Mod.Path - - 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) + 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 -} diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go index 05c6169464..6876f7d423 100644 --- a/internal/lsp/mod/mod_test.go +++ b/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, }, }, diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 0a304ed53f..ad5676b40c 100644 --- a/internal/lsp/source/view.go +++ b/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.