From 0043dadf92a14f37520160032d0fd180a383ec9a Mon Sep 17 00:00:00 2001 From: Rohan Challa Date: Thu, 16 Jan 2020 14:32:09 -0500 Subject: [PATCH] internal/lsp: use x/mod to get edits for go.mod quick fixes This change uses the wonderful functions from x/mod to get the proper edits for the quick fixes on a go.mod diagnostic. It also creates a goModData structure to hold the data thats gets passed into the various parse functions, this will help reduce the large function prototypes that can occur when we decompose the logic. It also refactors the Modfiles() function to return span.URIs vs FileHandles. Change-Id: Ifa0896442650f2ddbd8fe98d8f231a9e94c3d042 Reviewed-on: https://go-review.googlesource.com/c/tools/+/215097 Run-TryBot: Rohan Challa Reviewed-by: Rebecca Stambler TryBot-Result: Gobot Gobot --- internal/lsp/cache/modfiles.go | 2 +- internal/lsp/cache/parse_mod.go | 313 +++++++++++++----- internal/lsp/cache/snapshot.go | 23 -- internal/lsp/code_action.go | 2 +- internal/lsp/lsp_test.go | 34 +- internal/lsp/mod/diagnostics.go | 83 +++-- internal/lsp/mod/mod_test.go | 4 +- .../lsp/mod/testdata/indirect/go.mod.golden | 2 +- .../lsp/mod/testdata/unused/go.mod.golden | 2 - internal/lsp/source/view.go | 20 +- 10 files changed, 308 insertions(+), 177 deletions(-) diff --git a/internal/lsp/cache/modfiles.go b/internal/lsp/cache/modfiles.go index 9725358bf6..44ce3f6dbf 100644 --- a/internal/lsp/cache/modfiles.go +++ b/internal/lsp/cache/modfiles.go @@ -19,7 +19,7 @@ import ( errors "golang.org/x/xerrors" ) -func (v *view) modFiles(ctx context.Context) (span.URI, span.URI, error) { +func (v *view) ModFiles() (span.URI, span.URI, error) { // Don't return errors if the view is not a module. if v.mod == nil { return "", "", nil diff --git a/internal/lsp/cache/parse_mod.go b/internal/lsp/cache/parse_mod.go index b0f44c9346..b66d6f5fb1 100644 --- a/internal/lsp/cache/parse_mod.go +++ b/internal/lsp/cache/parse_mod.go @@ -33,7 +33,7 @@ type parseModKey struct { cfg string } -type parseModHandle struct { +type modTidyHandle struct { handle *memoize.Handle file source.FileHandle cfg *packages.Config @@ -48,26 +48,43 @@ type parseModData struct { err error } -func (pgh *parseModHandle) String() string { - return pgh.File().Identity().URI.Filename() +type goModData struct { + // origfh is the file handle for the original go.mod file. + origfh source.FileHandle + + // origParsedFile contains the parsed contents that are used to diff with + // the ideal contents. + origParsedFile *modfile.File + + // origMapper is the column mapper for the original go.mod file. + origMapper *protocol.ColumnMapper + + // idealParsedFile contains the parsed contents for the go.mod file + // after it has been "tidied". + idealParsedFile *modfile.File } -func (pgh *parseModHandle) File() source.FileHandle { - return pgh.file +func (mth *modTidyHandle) String() string { + return mth.File().Identity().URI.Filename() } -func (pgh *parseModHandle) Parse(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, []source.Error, error) { - v := pgh.handle.Get(ctx) +func (mth *modTidyHandle) File() source.FileHandle { + return mth.file +} + +func (mth *modTidyHandle) Tidy(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, []source.Error, error) { + v := mth.handle.Get(ctx) if v == nil { - return nil, nil, nil, errors.Errorf("no parsed file for %s", pgh.File().Identity().URI) + return nil, nil, nil, errors.Errorf("no parsed file for %s", mth.File().Identity().URI) } data := v.(*parseModData) 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) +func (s *snapshot) ModTidyHandle(ctx context.Context, realfh source.FileHandle) source.ModTidyHandle { + realURI, tempURI, err := s.View().ModFiles() cfg := s.View().Config(ctx) + options := s.View().Options() folder := s.View().Folder().Filename() key := parseModKey{ @@ -81,77 +98,102 @@ func (s *snapshot) ParseModHandle(ctx context.Context, fh source.FileHandle) sou return data } // Check the case when the tempModfile flag is turned off. - if realfh == nil || tempfh == nil { + if realURI == "" || tempURI == "" { return data } - data.modfile, data.mapper, data.parseErrors, data.err = goModFileDiagnostics(ctx, realfh, tempfh, cfg, folder) + + ctx, done := trace.StartSpan(ctx, "cache.ModTidyHandle", telemetry.File.Of(realURI)) + defer done() + + // Copy the real go.mod file content into the temp go.mod file. + realContents, _, err := realfh.Read(ctx) + if err != nil { + data.err = err + return data + } + if err := ioutil.WriteFile(tempURI.Filename(), realContents, os.ModePerm); err != nil { + data.err = err + return data + } + + // 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") { + data.err = err + return data + } + } + + realMapper := &protocol.ColumnMapper{ + URI: realURI, + Converter: span.NewContentConverter(realURI.Filename(), realContents), + Content: realContents, + } + origParsedFile, err := modfile.Parse(realURI.Filename(), realContents, nil) + if err != nil { + if parseErr, err := extractModParseErrors(ctx, realURI, realMapper, err, realContents); err == nil { + data.parseErrors = []source.Error{parseErr} + return data + } + data.err = err + return data + } + + // Go directly to disk to get the temporary mod file, since it is always on disk. + tempContents, err := ioutil.ReadFile(tempURI.Filename()) + if err != nil { + data.err = err + return data + } + idealParsedFile, err := modfile.Parse(tempURI.Filename(), tempContents, nil) + if err != nil { + // We do not need to worry about the temporary file's parse errors since it has been "tidied". + data.err = err + return data + } + + modData := goModData{ + origfh: realfh, + origParsedFile: origParsedFile, + origMapper: realMapper, + idealParsedFile: idealParsedFile, + } + errors, err := modRequireErrors(ctx, options, modData) + if err != nil { + data.err = err + return data + } + data.modfile, data.mapper, data.parseErrors, data.err = origParsedFile, realMapper, errors, nil return data }) - return &parseModHandle{ + return &modTidyHandle{ handle: h, - file: fh, + file: realfh, 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) { +// extractModParseErrors processes the raw errors returned by modfile.Parse, +// extracting the filenames and line numbers that correspond to the errors. +func extractModParseErrors(ctx context.Context, uri span.URI, m *protocol.ColumnMapper, parseErr error, content []byte) (source.Error, error) { re := regexp.MustCompile(`.*:([\d]+): (.+)`) - matches := re.FindStringSubmatch(strings.TrimSpace(modTidyErr.Error())) + matches := re.FindStringSubmatch(strings.TrimSpace(parseErr.Error())) if len(matches) < 3 { - log.Error(ctx, "could not parse golang/x/mod error message", modTidyErr) - return source.Error{}, modTidyErr + log.Error(ctx, "could not parse golang/x/mod error message", parseErr) + return source.Error{}, parseErr } line, err := strconv.Atoi(matches[1]) if err != nil { - return source.Error{}, modTidyErr + return source.Error{}, parseErr } - lines := strings.Split(string(buf), "\n") + lines := strings.Split(string(content), "\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. + // 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, 0) if err != nil { @@ -174,13 +216,15 @@ func modParseErrors(ctx context.Context, uri span.URI, m *protocol.ColumnMapper, }, 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 { +// modRequireErrors extracts the errors that occur on the require directives. +// It checks for directness issues and unused dependencies. +func modRequireErrors(ctx context.Context, options source.Options, modData goModData) ([]source.Error, error) { + realReqs := make(map[string]*modfile.Require, len(modData.origParsedFile.Require)) + tempReqs := make(map[string]*modfile.Require, len(modData.idealParsedFile.Require)) + for _, req := range modData.origParsedFile.Require { realReqs[req.Mod.Path] = req } - for _, req := range tempMod.Require { + for _, req := range modData.idealParsedFile.Require { realReq := realReqs[req.Mod.Path] if realReq != nil && realReq.Indirect == req.Indirect { delete(realReqs, req.Mod.Path) @@ -189,14 +233,13 @@ func modRequireErrors(realfh source.FileHandle, m *protocol.ColumnMapper, realMo } var errors []source.Error - for _, req := range realReqs { + for dep, 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) + directErr, err := modDirectnessErrors(ctx, options, modData, req) if err != nil { return nil, err } @@ -204,7 +247,11 @@ func modRequireErrors(realfh source.FileHandle, m *protocol.ColumnMapper, realMo } // Handle unused dependencies. if tempReqs[dep] == nil { - rng, err := rangeFromPositions(realfh.Identity().URI, m, req.Syntax.Start, req.Syntax.End) + rng, err := rangeFromPositions(modData.origfh.Identity().URI, modData.origMapper, req.Syntax.Start, req.Syntax.End) + if err != nil { + return nil, err + } + edits, err := dropDependencyEdits(ctx, options, modData, req) if err != nil { return nil, err } @@ -212,15 +259,22 @@ func modRequireErrors(realfh source.FileHandle, m *protocol.ColumnMapper, realMo Category: ModTidyError, Message: fmt.Sprintf("%s is not used in this module.", dep), Range: rng, - URI: realfh.Identity().URI, + URI: modData.origfh.Identity().URI, + SuggestedFixes: []source.SuggestedFix{ + { + Title: fmt.Sprintf("Remove dependency: %s", dep), + Edits: map[span.URI][]protocol.TextEdit{modData.origfh.Identity().URI: edits}, + }, + }, }) } } 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) +// modDirectnessErrors extracts errors when a dependency is labeled indirect when it should be direct and vice versa. +func modDirectnessErrors(ctx context.Context, options source.Options, modData goModData, req *modfile.Require) (source.Error, error) { + rng, err := rangeFromPositions(modData.origfh.Identity().URI, modData.origMapper, req.Syntax.Start, req.Syntax.End) if err != nil { return source.Error{}, err } @@ -230,42 +284,121 @@ func modDirectnessErrors(fh source.FileHandle, m *protocol.ColumnMapper, req *mo 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) + rng, err = rangeFromPositions(modData.origfh.Identity().URI, modData.origMapper, comments.Suffix[0].Start, end) if err != nil { return source.Error{}, err } } + edits, err := changeDirectnessEdits(ctx, options, modData, req, false) + 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, + URI: modData.origfh.Identity().URI, + SuggestedFixes: []source.SuggestedFix{{ + Title: fmt.Sprintf("Make %s direct", req.Mod.Path), + Edits: map[span.URI][]protocol.TextEdit{modData.origfh.Identity().URI: edits}, + }}, }, nil } + // If the dependency should be indirect, add the // indirect. + edits, err := changeDirectnessEdits(ctx, options, modData, req, true) + if err != nil { + return source.Error{}, err + } return source.Error{ Category: ModTidyError, Message: fmt.Sprintf("%s should be an indirect dependency.", req.Mod.Path), Range: rng, - URI: fh.Identity().URI, + URI: modData.origfh.Identity().URI, + SuggestedFixes: []source.SuggestedFix{{ + Title: fmt.Sprintf("Make %s indirect", req.Mod.Path), + Edits: map[span.URI][]protocol.TextEdit{modData.origfh.Identity().URI: edits}, + }}, }, nil } -func parseModFile(ctx context.Context, fh source.FileHandle) (*modfile.File, *protocol.ColumnMapper, *source.Error, error) { - contents, _, err := fh.Read(ctx) +// dropDependencyEdits gets the edits needed to remove the dependency from the go.mod file. +// As an example, this function will codify the edits needed to convert the before go.mod file to the after. +// Before: +// module t +// +// go 1.11 +// +// require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee +// After: +// module t +// +// go 1.11 +func dropDependencyEdits(ctx context.Context, options source.Options, modData goModData, req *modfile.Require) ([]protocol.TextEdit, error) { + if err := modData.origParsedFile.DropRequire(req.Mod.Path); err != nil { + return nil, err + } + modData.origParsedFile.Cleanup() + newContents, err := modData.origParsedFile.Format() if err != nil { - return nil, nil, nil, err + return 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) + // Reset the *modfile.File back to before we dropped the dependency. + modData.origParsedFile.AddNewRequire(req.Mod.Path, req.Mod.Version, req.Indirect) + // Calculate the edits to be made due to the change. + diff := options.ComputeEdits(modData.origfh.Identity().URI, string(modData.origMapper.Content), string(newContents)) + edits, err := source.ToProtocolEdits(modData.origMapper, diff) if err != nil { - parseErr, err := modParseErrors(ctx, fh.Identity().URI, m, err, contents) - return nil, nil, &parseErr, err + return nil, err } - return parsed, m, nil, nil + return edits, nil +} + +// changeDirectnessEdits gets the edits needed to change an indirect dependency to direct and vice versa. +// As an example, this function will codify the edits needed to convert the before go.mod file to the after. +// Before: +// module t +// +// go 1.11 +// +// require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee +// After: +// module t +// +// go 1.11 +// +// require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee // indirect +func changeDirectnessEdits(ctx context.Context, options source.Options, modData goModData, req *modfile.Require, indirect bool) ([]protocol.TextEdit, error) { + var newReq []*modfile.Require + prevIndirect := false + // Change the directness in the matching require statement. + for _, r := range modData.origParsedFile.Require { + if req.Mod.Path == r.Mod.Path { + prevIndirect = req.Indirect + req.Indirect = indirect + } + newReq = append(newReq, r) + } + modData.origParsedFile.SetRequire(newReq) + modData.origParsedFile.Cleanup() + newContents, err := modData.origParsedFile.Format() + if err != nil { + return nil, err + } + // Change the dependency back to the way it was before we got the newContents. + for _, r := range modData.origParsedFile.Require { + if req.Mod.Path == r.Mod.Path { + req.Indirect = prevIndirect + } + newReq = append(newReq, r) + } + modData.origParsedFile.SetRequire(newReq) + // Calculate the edits to be made due to the change. + diff := options.ComputeEdits(modData.origfh.Identity().URI, string(modData.origMapper.Content), string(newContents)) + edits, err := source.ToProtocolEdits(modData.origMapper, diff) + if err != nil { + return nil, err + } + return edits, nil } func rangeFromPositions(uri span.URI, m *protocol.ColumnMapper, s, e modfile.Position) (protocol.Range, error) { diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index bd31dfff06..7695025c5c 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -65,29 +65,6 @@ func (s *snapshot) View() source.View { return s.view } -func (s *snapshot) ModFiles(ctx context.Context) (source.FileHandle, source.FileHandle, error) { - r, t, err := s.view.modFiles(ctx) - if err != nil { - return nil, nil, err - } - if r == "" || t == "" { - return nil, nil, nil - } - // Get the real mod file's content through the snapshot, - // as it may be open in an overlay. - realfh, err := s.GetFile(r) - if err != nil { - return nil, nil, err - } - // Go directly to disk to get the temporary mod file, - // since it is always on disk. - tempfh := s.view.session.cache.GetFile(t) - if tempfh == nil { - return nil, nil, errors.Errorf("temporary go.mod filehandle is nil") - } - return realfh, tempfh, nil -} - func (s *snapshot) PackageHandles(ctx context.Context, fh source.FileHandle) ([]source.PackageHandle, error) { // If the file is a go.mod file, go.Packages.Load will always return 0 packages. if fh.Identity().Kind == source.Mod { diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 15cdae883e..c7f99cea40 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -68,7 +68,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara }) } if diagnostics := params.Context.Diagnostics; len(diagnostics) > 0 { - codeActions = append(codeActions, mod.SuggestedFixes(fh, diagnostics)...) + codeActions = append(codeActions, mod.SuggestedFixes(ctx, snapshot, fh, diagnostics)...) } case source.Go: edits, editsPerFix, err := source.AllImportsFixes(ctx, snapshot, fh) diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index a47bfdda7d..6384e0c9d5 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -932,7 +932,7 @@ func TestBytesOffset(t *testing.T) { // when marker support gets added for go.mod files. func TestModfileSuggestedFixes(t *testing.T) { if runtime.GOOS == "android" { - t.Skipf("this test cannot find mod/testdata files") + t.Skip("this test cannot find mod/testdata files") } ctx := tests.Context(t) @@ -959,10 +959,17 @@ func TestModfileSuggestedFixes(t *testing.T) { if err != nil { t.Fatal(err) } + + realURI, tempURI, _ := snapshot.View().ModFiles() // TODO: Add testing for when the -modfile flag is turned off and we still get diagnostics. - if _, t, _ := snapshot.ModFiles(ctx); t == nil { + if tempURI == "" { return } + realfh, err := snapshot.GetFile(realURI) + if err != nil { + t.Fatal(err) + } + reports, err := mod.Diagnostics(ctx, snapshot) if err != nil { t.Fatal(err) @@ -970,6 +977,12 @@ func TestModfileSuggestedFixes(t *testing.T) { if len(reports) != 1 { t.Errorf("expected 1 fileHandle, got %d", len(reports)) } + + _, m, _, err := snapshot.ModTidyHandle(ctx, realfh).Tidy(ctx) + if err != nil { + t.Fatal(err) + } + for fh, diags := range reports { actions, err := server.CodeAction(ctx, &protocol.CodeActionParams{ TextDocument: protocol.TextDocumentIdentifier{ @@ -989,7 +1002,6 @@ func TestModfileSuggestedFixes(t *testing.T) { if len(actions) > 1 { t.Fatal("expected only 1 code action") } - res := map[span.URI]string{} for _, docEdits := range actions[0].Edit.DocumentChanges { uri := span.URI(docEdits.TextDocument.URI) @@ -998,20 +1010,14 @@ func TestModfileSuggestedFixes(t *testing.T) { t.Fatal(err) } res[uri] = string(content) - - split := strings.Split(res[uri], "\n") - for i := len(docEdits.Edits) - 1; i >= 0; i-- { - edit := docEdits.Edits[i] - start := edit.Range.Start - end := edit.Range.End - tmp := split[int(start.Line)][0:int(start.Character)] + edit.NewText - split[int(end.Line)] = tmp + split[int(end.Line)][int(end.Character):] + sedits, err := source.FromProtocolEdits(m, docEdits.Edits) + if err != nil { + t.Fatal(err) } - res[uri] = strings.Join(split, "\n") + res[uri] = applyEdits(res[uri], sedits) } got := res[fh.URI] - golden := filepath.Join(folder, "go.mod.golden") - contents, err := ioutil.ReadFile(golden) + contents, err := ioutil.ReadFile(filepath.Join(folder, "go.mod.golden")) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index 5b40fd612e..5ec21454b6 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -8,29 +8,33 @@ package mod import ( "context" - "fmt" - "strings" "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/telemetry/log" "golang.org/x/tools/internal/telemetry/trace" ) 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) + realURI, tempURI, err := snapshot.View().ModFiles() if err != nil { return nil, err } // Check the case when the tempModfile flag is turned off. - if realfh == nil || tempfh == nil { + if realURI == "" || tempURI == "" { return nil, nil } - ctx, done := trace.StartSpan(ctx, "modfiles.Diagnostics", telemetry.File.Of(realfh.Identity().URI)) + + ctx, done := trace.StartSpan(ctx, "mod.Diagnostics", telemetry.File.Of(realURI)) defer done() - _, _, parseErrors, err := snapshot.ParseModHandle(ctx, realfh).Parse(ctx) + realfh, err := snapshot.GetFile(realURI) + if err != nil { + return nil, err + } + _, _, parseErrors, err := snapshot.ModTidyHandle(ctx, realfh).Tidy(ctx) if err != nil { return nil, err } @@ -55,43 +59,56 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.File return reports, nil } -// TODO: Add caching for go.mod diagnostics to be able to map them back to source.Diagnostics -// and reuse the cached suggested fixes. -func SuggestedFixes(fh source.FileHandle, diags []protocol.Diagnostic) []protocol.CodeAction { +func SuggestedFixes(ctx context.Context, snapshot source.Snapshot, f source.FileHandle, diags []protocol.Diagnostic) []protocol.CodeAction { + _, _, parseErrors, err := snapshot.ModTidyHandle(ctx, f).Tidy(ctx) + if err != nil { + return nil + } + + errorsMap := make(map[string][]source.Error) + for _, e := range parseErrors { + if errorsMap[e.Message] == nil { + errorsMap[e.Message] = []source.Error{} + } + errorsMap[e.Message] = append(errorsMap[e.Message], e) + } + var actions []protocol.CodeAction for _, diag := range diags { - var title string - if strings.Contains(diag.Message, "is not used in this module") { - split := strings.Split(diag.Message, " ") - if len(split) < 1 { + for _, e := range errorsMap[diag.Message] { + if !sameDiagnostic(diag, e) { continue } - title = fmt.Sprintf("Remove dependency: %s", split[0]) - } - if strings.Contains(diag.Message, "should be a direct dependency.") { - title = "Remove indirect" - } - if title == "" { - continue - } - actions = append(actions, protocol.CodeAction{ - Title: title, - Kind: protocol.QuickFix, - Edit: protocol.WorkspaceEdit{ - DocumentChanges: []protocol.TextDocumentEdit{ - { + for _, fix := range e.SuggestedFixes { + action := protocol.CodeAction{ + Title: fix.Title, + Kind: protocol.QuickFix, + Diagnostics: []protocol.Diagnostic{diag}, + Edit: protocol.WorkspaceEdit{}, + } + for uri, edits := range fix.Edits { + fh, err := snapshot.GetFile(uri) + if err != nil { + log.Error(ctx, "no file", err, telemetry.URI.Of(uri)) + continue + } + action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, protocol.TextDocumentEdit{ TextDocument: protocol.VersionedTextDocumentIdentifier{ Version: fh.Identity().Version, TextDocumentIdentifier: protocol.TextDocumentIdentifier{ URI: protocol.NewURI(fh.Identity().URI), }, }, - Edits: []protocol.TextEdit{protocol.TextEdit{Range: diag.Range, NewText: ""}}, - }, - }, - }, - Diagnostics: diags, - }) + Edits: edits, + }) + } + actions = append(actions, action) + } + } } return actions } + +func sameDiagnostic(d protocol.Diagnostic, e source.Error) bool { + return d.Message == e.Message && protocol.CompareRange(d.Range, e.Range) == 0 && d.Source == e.Category +} diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go index 29d0d1b928..a498303546 100644 --- a/internal/lsp/mod/mod_test.go +++ b/internal/lsp/mod/mod_test.go @@ -167,8 +167,8 @@ func TestDiagnostics(t *testing.T) { } func hasTempModfile(ctx context.Context, snapshot source.Snapshot) bool { - _, t, _ := snapshot.ModFiles(ctx) - return t != nil + _, t, _ := snapshot.View().ModFiles() + return t != "" } func getRawPos(line, character int) protocol.Position { diff --git a/internal/lsp/mod/testdata/indirect/go.mod.golden b/internal/lsp/mod/testdata/indirect/go.mod.golden index 76e0dfd3c7..7e4be77c96 100644 --- a/internal/lsp/mod/testdata/indirect/go.mod.golden +++ b/internal/lsp/mod/testdata/indirect/go.mod.golden @@ -2,4 +2,4 @@ module indirect go 1.12 -require golang.org/x/tools v0.0.0-20191219192050-56b0b28a00f7 +require golang.org/x/tools v0.0.0-20191219192050-56b0b28a00f7 diff --git a/internal/lsp/mod/testdata/unused/go.mod.golden b/internal/lsp/mod/testdata/unused/go.mod.golden index da1c5a3428..34ca63ab62 100644 --- a/internal/lsp/mod/testdata/unused/go.mod.golden +++ b/internal/lsp/mod/testdata/unused/go.mod.golden @@ -1,5 +1,3 @@ module unused go 1.12 - - diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 8b826362e7..ed4ffef0bc 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -37,12 +37,9 @@ type Snapshot interface { // This is used to get the SuggestedFixes associated with that error. FindAnalysisError(ctx context.Context, pkgID, analyzerName, msg string, rng protocol.Range) (*Error, error) - // 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. + // ModTidyHandle returns a ModTidyHandle 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 + ModTidyHandle(ctx context.Context, fh FileHandle) ModTidyHandle // PackageHandles returns the PackageHandles for the packages that this file // belongs to. @@ -99,6 +96,9 @@ type View interface { // ModFile is the path to the go.mod file for the view, if any. ModFile() string + // ModFiles returns the URIs of the go.mod files attached to the view associated with this snapshot. + ModFiles() (span.URI, span.URI, error) + // LookupBuiltin returns the go/ast.Object for the given name in the builtin package. LookupBuiltin(ctx context.Context, name string) (*ast.Object, error) @@ -246,14 +246,14 @@ type ParseGoHandle interface { Cached() (*ast.File, *protocol.ColumnMapper, error, error) } -// ParseModHandle represents a handle to the modfile for a go.mod. -type ParseModHandle interface { +// ModTidyHandle represents a handle to the modfile for a go.mod. +type ModTidyHandle interface { // File returns a file handle for which to get the modfile. File() FileHandle - // 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, error) + // Tidy returns the parsed modfile, a mapper, and "go mod tidy" errors + // for the go.mod file. If the file is not available, returns nil and an error. + Tidy(ctx context.Context) (*modfile.File, *protocol.ColumnMapper, []Error, error) } // ParseMode controls the content of the AST produced when parsing a source file.