internal/lsp: show dependency quick fixes for go.mod diagnostics
This change adds quickfixes for unused dependencies and dependencies that should not be marked as indirect. It also updates the positions for the diagnostics to make more sense relative to the warning message. There is a testing harness now for suggested fixes.
Updates golang/go#31999
Change-Id: I0db3382bf892fcc2d9ab3b633020d9167a0ad09b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/213917
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go
index bdf6e46..15cdae8 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -11,6 +11,7 @@
"strings"
"golang.org/x/tools/internal/imports"
+ "golang.org/x/tools/internal/lsp/mod"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/lsp/telemetry"
@@ -56,17 +57,19 @@
switch fh.Identity().Kind {
case source.Mod:
if !wanted[protocol.SourceOrganizeImports] {
- return nil, nil
+ codeActions = append(codeActions, protocol.CodeAction{
+ Title: "Tidy",
+ Kind: protocol.SourceOrganizeImports,
+ Command: &protocol.Command{
+ Title: "Tidy",
+ Command: "tidy",
+ Arguments: []interface{}{fh.Identity().URI},
+ },
+ })
}
- codeActions = append(codeActions, protocol.CodeAction{
- Title: "Tidy",
- Kind: protocol.SourceOrganizeImports,
- Command: &protocol.Command{
- Title: "Tidy",
- Command: "tidy",
- Arguments: []interface{}{fh.Identity().URI},
- },
- })
+ if diagnostics := params.Context.Diagnostics; len(diagnostics) > 0 {
+ codeActions = append(codeActions, mod.SuggestedFixes(fh, diagnostics)...)
+ }
case source.Go:
edits, editsPerFix, err := source.AllImportsFixes(ctx, snapshot, fh)
if err != nil {
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 78a0d8f..f2b5008 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -9,9 +9,11 @@
"context"
"fmt"
"go/token"
+ "io/ioutil"
"os"
"os/exec"
"path/filepath"
+ "runtime"
"sort"
"strings"
"testing"
@@ -19,6 +21,7 @@
"golang.org/x/tools/go/packages/packagestest"
"golang.org/x/tools/internal/lsp/cache"
"golang.org/x/tools/internal/lsp/diff"
+ "golang.org/x/tools/internal/lsp/mod"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/lsp/tests"
@@ -926,3 +929,99 @@
}
}
}
+
+// TODO(golang/go#36091): This function can be refactored to look like the rest of this file
+// 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")
+ }
+
+ ctx := tests.Context(t)
+ cache := cache.New(nil)
+ session := cache.NewSession(ctx)
+ options := tests.DefaultOptions()
+ options.TempModfile = true
+ options.Env = append(os.Environ(), "GOPACKAGESDRIVER=off", "GOROOT=")
+
+ server := Server{
+ session: session,
+ delivered: map[span.URI]sentDiagnostics{},
+ }
+
+ for _, tt := range []string{"indirect", "unused"} {
+ t.Run(tt, func(t *testing.T) {
+ folder, err := tests.CopyFolderToTempDir(filepath.Join("mod", "testdata", tt))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer os.RemoveAll(folder)
+
+ _, snapshot, err := session.NewView(ctx, "suggested_fix_test", span.FileURI(folder), options)
+ if err != nil {
+ t.Fatal(err)
+ }
+ // TODO: Add testing for when the -modfile flag is turned off and we still get diagnostics.
+ if _, t, _ := snapshot.ModFiles(ctx); t == nil {
+ return
+ }
+ reports, err := mod.Diagnostics(ctx, snapshot)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(reports) != 1 {
+ t.Errorf("expected 1 fileHandle, got %d", len(reports))
+ }
+ for fh, diags := range reports {
+ actions, err := server.CodeAction(ctx, &protocol.CodeActionParams{
+ TextDocument: protocol.TextDocumentIdentifier{
+ URI: protocol.NewURI(fh.URI),
+ },
+ Context: protocol.CodeActionContext{
+ Only: []protocol.CodeActionKind{protocol.SourceOrganizeImports},
+ Diagnostics: toProtocolDiagnostics(ctx, diags),
+ },
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(actions) == 0 {
+ t.Fatal("no code actions returned")
+ }
+ 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)
+ content, err := ioutil.ReadFile(uri.Filename())
+ if err != nil {
+ 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):]
+ }
+ res[uri] = strings.Join(split, "\n")
+ }
+ got := res[fh.URI]
+ golden := filepath.Join(folder, "go.mod.golden")
+ contents, err := ioutil.ReadFile(golden)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := string(contents)
+ if want != got {
+ t.Errorf("suggested fixes failed for %s, expected:\n%s\ngot:\n%s", fh.URI.Filename(), want, got)
+ }
+ }
+ })
+ }
+}
diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go
index 2ef87f9..b7cd840 100644
--- a/internal/lsp/mod/diagnostics.go
+++ b/internal/lsp/mod/diagnostics.go
@@ -86,30 +86,72 @@
}
dep := req.Mod.Path
- start, err := positionToPoint(m, req.Syntax.Start)
+ rng, err := getRangeFromPositions(m, realfh, req.Syntax.Start, req.Syntax.End)
if err != nil {
return nil, err
}
- end, err := positionToPoint(m, req.Syntax.End)
- if err != nil {
- return nil, err
- }
- spn := span.New(realfh.Identity().URI, start, end)
- rng, err := m.Range(spn)
- if err != nil {
- return nil, err
- }
+ var diag *source.Diagnostic
- diag := &source.Diagnostic{
- Message: fmt.Sprintf("%s is not used in this module.", dep),
- Source: "go mod tidy",
- Range: rng,
- Severity: protocol.SeverityWarning,
- }
if tempReqs[dep] != nil && req.Indirect != tempReqs[dep].Indirect {
- diag.Message = fmt.Sprintf("%s should be an indirect dependency.", dep)
+ // Show diagnostics for dependencies that are incorrectly labeled indirect.
if req.Indirect {
- diag.Message = fmt.Sprintf("%s should not be an indirect dependency.", dep)
+ 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)
@@ -117,6 +159,78 @@
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 {
+ 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 {
+ 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{
+ {
+ 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,
+ })
+ }
+ 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 {
diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go
index 76ed109..a720546 100644
--- a/internal/lsp/mod/mod_test.go
+++ b/internal/lsp/mod/mod_test.go
@@ -2,19 +2,16 @@
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
-package mod_test
+package mod
import (
"context"
- "fmt"
"io/ioutil"
"os"
- "path"
"path/filepath"
"testing"
"golang.org/x/tools/internal/lsp/cache"
- "golang.org/x/tools/internal/lsp/mod"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/lsp/tests"
@@ -39,7 +36,7 @@
// Make sure to copy the test directory to a temporary directory so we do not
// modify the test code or add go.sum files when we run the tests.
- folder, err := copyToTempDir(filepath.Join("testdata", "unchanged"))
+ folder, err := tests.CopyFolderToTempDir(filepath.Join("testdata", "unchanged"))
if err != nil {
t.Fatal(err)
}
@@ -83,11 +80,11 @@
testdir: "indirect",
want: []source.Diagnostic{
{
- Message: "golang.org/x/tools should not be an indirect dependency.",
+ Message: "golang.org/x/tools should be a direct dependency.",
Source: "go mod tidy",
// TODO(golang/go#36091): When marker support gets added for go.mod files, we
// can remove these hard coded positions.
- Range: protocol.Range{Start: getPos(4, 0), End: getPos(4, 61)},
+ Range: protocol.Range{Start: getRawPos(4, 62), End: getRawPos(4, 73)},
Severity: protocol.SeverityWarning,
},
},
@@ -98,7 +95,7 @@
{
Message: "golang.org/x/tools is not used in this module.",
Source: "go mod tidy",
- Range: protocol.Range{Start: getPos(4, 0), End: getPos(4, 61)},
+ Range: protocol.Range{Start: getRawPos(4, 0), End: getRawPos(4, 61)},
Severity: protocol.SeverityWarning,
},
},
@@ -109,7 +106,7 @@
{
Message: "usage: require module/path v1.2.3",
Source: "syntax",
- Range: protocol.Range{Start: getPos(4, 0), End: getPos(4, 17)},
+ Range: protocol.Range{Start: getRawPos(4, 0), End: getRawPos(4, 17)},
Severity: protocol.SeverityError,
},
},
@@ -120,7 +117,7 @@
{
Message: "usage: go 1.23",
Source: "syntax",
- Range: protocol.Range{Start: getPos(2, 0), End: getPos(2, 4)},
+ Range: protocol.Range{Start: getRawPos(2, 0), End: getRawPos(2, 4)},
Severity: protocol.SeverityError,
},
},
@@ -131,16 +128,16 @@
{
Message: "unknown directive: yo",
Source: "syntax",
- Range: protocol.Range{Start: getPos(6, 0), End: getPos(6, 2)},
+ Range: protocol.Range{Start: getRawPos(6, 0), End: getRawPos(6, 2)},
Severity: protocol.SeverityError,
},
},
},
} {
t.Run(tt.testdir, func(t *testing.T) {
- // Make sure to copy the test directory to a temporary directory so we do not
- // modify the test code or add go.sum files when we run the tests.
- folder, err := copyToTempDir(filepath.Join("testdata", tt.testdir))
+ // TODO: Once we refactor this to work with go/packages/packagestest. We do not
+ // need to copy to a temporary directory.
+ folder, err := tests.CopyFolderToTempDir(filepath.Join("testdata", tt.testdir))
if err != nil {
t.Fatal(err)
}
@@ -153,7 +150,7 @@
if !hasTempModfile(ctx, snapshot) {
return
}
- reports, err := mod.Diagnostics(ctx, snapshot)
+ reports, err := Diagnostics(ctx, snapshot)
if err != nil {
t.Fatal(err)
}
@@ -174,38 +171,7 @@
return t != nil
}
-func copyToTempDir(folder string) (string, error) {
- if _, err := os.Stat(folder); err != nil {
- return "", err
- }
- dst, err := ioutil.TempDir("", "modfile_test")
- if err != nil {
- return "", err
- }
- fds, err := ioutil.ReadDir(folder)
- if err != nil {
- return "", err
- }
- for _, fd := range fds {
- srcfp := path.Join(folder, fd.Name())
- dstfp := path.Join(dst, fd.Name())
- stat, err := os.Stat(srcfp)
- if err != nil {
- return "", err
- }
- if !stat.Mode().IsRegular() {
- return "", fmt.Errorf("cannot copy non regular file %s", srcfp)
- }
- contents, err := ioutil.ReadFile(srcfp)
- if err != nil {
- return "", err
- }
- ioutil.WriteFile(dstfp, contents, stat.Mode())
- }
- return dst, nil
-}
-
-func getPos(line, character int) protocol.Position {
+func getRawPos(line, character int) protocol.Position {
return protocol.Position{
Line: float64(line),
Character: float64(character),
diff --git a/internal/lsp/mod/testdata/indirect/go.mod b/internal/lsp/mod/testdata/indirect/go.mod
index 84e1b53..2e5dc13 100644
--- a/internal/lsp/mod/testdata/indirect/go.mod
+++ b/internal/lsp/mod/testdata/indirect/go.mod
@@ -1,4 +1,4 @@
-module invalidrequire
+module indirect
go 1.12
diff --git a/internal/lsp/mod/testdata/indirect/go.mod.golden b/internal/lsp/mod/testdata/indirect/go.mod.golden
new file mode 100644
index 0000000..76e0dfd
--- /dev/null
+++ b/internal/lsp/mod/testdata/indirect/go.mod.golden
@@ -0,0 +1,5 @@
+module indirect
+
+go 1.12
+
+require golang.org/x/tools v0.0.0-20191219192050-56b0b28a00f7
diff --git a/internal/lsp/mod/testdata/invalidrequire/go.mod b/internal/lsp/mod/testdata/invalidrequire/go.mod
index 6829b9c..98c5b05 100644
--- a/internal/lsp/mod/testdata/invalidrequire/go.mod
+++ b/internal/lsp/mod/testdata/invalidrequire/go.mod
@@ -1,4 +1,4 @@
-module indirect
+module invalidrequire
go 1.12
diff --git a/internal/lsp/mod/testdata/unused/go.mod.golden b/internal/lsp/mod/testdata/unused/go.mod.golden
new file mode 100644
index 0000000..da1c5a3
--- /dev/null
+++ b/internal/lsp/mod/testdata/unused/go.mod.golden
@@ -0,0 +1,5 @@
+module unused
+
+go 1.12
+
+
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index db63e83..f4331da 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -13,6 +13,7 @@
"go/ast"
"go/token"
"io/ioutil"
+ "os"
"path/filepath"
"runtime"
"sort"
@@ -186,7 +187,9 @@
protocol.SourceOrganizeImports: true,
protocol.QuickFix: true,
},
- source.Mod: {},
+ source.Mod: {
+ protocol.SourceOrganizeImports: true,
+ },
source.Sum: {},
}
o.HoverKind = source.SynopsisDocumentation
@@ -920,3 +923,35 @@
func spanName(spn span.Span) string {
return fmt.Sprintf("%v_%v_%v", uriName(spn.URI()), spn.Start().Line(), spn.Start().Column())
}
+
+func CopyFolderToTempDir(folder string) (string, error) {
+ if _, err := os.Stat(folder); err != nil {
+ return "", err
+ }
+ dst, err := ioutil.TempDir("", "modfile_test")
+ if err != nil {
+ return "", err
+ }
+ fds, err := ioutil.ReadDir(folder)
+ if err != nil {
+ return "", err
+ }
+ for _, fd := range fds {
+ srcfp := filepath.Join(folder, fd.Name())
+ stat, err := os.Stat(srcfp)
+ if err != nil {
+ return "", err
+ }
+ if !stat.Mode().IsRegular() {
+ return "", fmt.Errorf("cannot copy non regular file %s", srcfp)
+ }
+ contents, err := ioutil.ReadFile(srcfp)
+ if err != nil {
+ return "", err
+ }
+ if err := ioutil.WriteFile(filepath.Join(dst, fd.Name()), contents, stat.Mode()); err != nil {
+ return "", err
+ }
+ }
+ return dst, nil
+}
