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 <rohan@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

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

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

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 {

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)

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

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

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 {

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

internal/lsp/mod/testdata/unused/go.mod.golden

@@ -1,5 +1,3 @@
 module unused
 
 go 1.12
-
-

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.