[tools] internal/lsp: add knownPackages and addImport as nonStandardRequests
Marwan Sulaiman (Gerrit)
Marwan Sulaiman has uploaded this change for review.
internal/lsp: add knownPackages and addImport as nonStandardRequests
This CL adds two new non-standard methods that lets a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/lsp_test.go
M internal/lsp/server.go
A internal/lsp/source/add_import.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
9 files changed, 273 insertions(+), 0 deletions(-)
diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go
index b8abe6d..4521504 100644
--- a/internal/lsp/cmd/test/cmdtest.go
+++ b/internal/lsp/cmd/test/cmdtest.go
@@ -100,6 +100,10 @@
//TODO: function extraction not supported on command line
}
+func (r *runner) AddImport(t *testing.T, uri span.URI, expectedImport string) {
+ //TODO: function extraction not supported on command line
+}
+
func (r *runner) runGoplsCmd(t testing.TB, args ...string) (string, string) {
rStdout, wStdout, err := os.Pipe()
if err != nil {
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index fd8b4ad..e8c8eee 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -1122,6 +1122,51 @@
}
}
+func (r *runner) AddImport(t *testing.T, uri span.URI, expectedImport string) {
+ resp, err := r.server.nonstandardRequest(r.ctx, "gopls/knownPackages", map[string]interface{}{
+ "textDocument": map[string]interface{}{
+ "uri": string(uri),
+ },
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ pkgs, _ := resp.([]string)
+ var hasPkg bool
+ for _, p := range pkgs {
+ if p == expectedImport {
+ hasPkg = true
+ break
+ }
+ }
+ if !hasPkg {
+ t.Fatalf("expected %q to be in list of knownPackages of length %d", expectedImport, len(pkgs))
+ }
+ edits, err := r.server.addImport(r.ctx, map[string]interface{}{
+ "textDocument": map[string]interface{}{
+ "uri": string(uri),
+ },
+ "importPath": expectedImport,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ res, err := applyTextDocumentEdits(r, edits)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := res[uri]
+ got := r.data.Golden("addimport", uri.Filename(), func() ([]byte, error) {
+ return []byte(want), nil
+ })
+ if got == nil {
+ t.Fatalf("golden file %q not found", uri.Filename())
+ }
+ if diff := tests.Diff(t, want, string(got)); diff != "" {
+ t.Error(diff)
+ }
+}
+
func TestBytesOffset(t *testing.T) {
tests := []struct {
text string
diff --git a/internal/lsp/server.go b/internal/lsp/server.go
index df1da8c..7ff292f 100644
--- a/internal/lsp/server.go
+++ b/internal/lsp/server.go
@@ -143,10 +143,85 @@
return nil, err
}
return struct{}{}, nil
+ case "gopls/knownPackages":
+ uri, err := knownPackagesRequest(params)
+ if err != nil {
+ return nil, err
+ }
+ snapshot, fh, ok, release, err := s.beginFileRequest(ctx, protocol.DocumentURI(uri), source.UnknownKind)
+ defer release()
+ if !ok {
+ return nil, err
+ }
+ return source.KnownPackages(ctx, snapshot, fh)
+ case "gopls/addImport":
+ edits, err := s.addImport(ctx, params)
+ if err != nil {
+ return nil, err
+ }
+ _, err = s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{
+ Edit: protocol.WorkspaceEdit{
+ DocumentChanges: edits,
+ },
+ })
+ return nil, err
}
return nil, notImplemented(method)
}
+func (s *Server) addImport(ctx context.Context, params interface{}) ([]protocol.TextDocumentEdit, error) {
+ importPath, uri, err := addImportRequest(params)
+ if err != nil {
+ return nil, err
+ }
+ snapshot, fh, ok, release, err := s.beginFileRequest(ctx, protocol.DocumentURI(uri), source.UnknownKind)
+ defer release()
+ if !ok {
+ return nil, err
+ }
+ edits, err := source.AddImport(ctx, snapshot, fh, importPath)
+ if err != nil {
+ return nil, err
+ }
+ return documentChanges(fh, edits), nil
+}
+
+func addImportRequest(params interface{}) (importPath string, uri string, err error) {
+ paramMap, ok := params.(map[string]interface{})
+ if !ok {
+ return "", "", fmt.Errorf("invalid gopls/addImport request type: %T", params)
+ }
+ td, ok := paramMap["textDocument"].(map[string]interface{})
+ if !ok {
+ return "", "", fmt.Errorf("invalid gopls/addImport textDocument type: %T", params)
+ }
+ uri, ok = td["uri"].(string)
+ if !ok || uri == "" {
+ return "", "", fmt.Errorf("gopls/addImport uri must be a string")
+ }
+ importPath, ok = paramMap["importPath"].(string)
+ if !ok || importPath == "" {
+ return "", "", fmt.Errorf("gopls/addImport importPath must be a string")
+ }
+ return importPath, uri, nil
+}
+
+func knownPackagesRequest(params interface{}) (string, error) {
+ paramMap, ok := params.(map[string]interface{})
+ if !ok {
+ return "", fmt.Errorf("invalid gopls/golist request type: %T", params)
+ }
+ td, ok := paramMap["textDocument"].(map[string]interface{})
+ if !ok {
+ return "", fmt.Errorf("invalid gopls/golist textDocument type: %T", params)
+ }
+ uri, ok := td["uri"].(string)
+ if !ok || uri == "" {
+ return "", fmt.Errorf("gopls/golist uri must be a string")
+ }
+ return uri, nil
+}
+
func notImplemented(method string) error {
return errors.Errorf("%w: %q not yet implemented", jsonrpc2.ErrMethodNotFound, method)
}
diff --git a/internal/lsp/source/add_import.go b/internal/lsp/source/add_import.go
new file mode 100644
index 0000000..7fa6bec
--- /dev/null
+++ b/internal/lsp/source/add_import.go
@@ -0,0 +1,22 @@
+package source
+
+import (
+ "context"
+
+ "golang.org/x/tools/internal/imports"
+ "golang.org/x/tools/internal/lsp/protocol"
+)
+
+// AddImport adds a single import statement to the given file
+func AddImport(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle, importPath string) ([]protocol.TextEdit, error) {
+ _, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
+ if err != nil {
+ return nil, err
+ }
+ return ComputeOneImportFixEdits(snapshot, pgf, &imports.ImportFix{
+ StmtInfo: imports.ImportInfo{
+ ImportPath: importPath,
+ },
+ FixType: imports.AddImport,
+ })
+}
diff --git a/internal/lsp/source/known_packages.go b/internal/lsp/source/known_packages.go
new file mode 100644
index 0000000..53f8195
--- /dev/null
+++ b/internal/lsp/source/known_packages.go
@@ -0,0 +1,98 @@
+package source
+
+import (
+ "context"
+ "fmt"
+ "sort"
+ "strings"
+)
+
+// KnownPackages returns a list of all known packages
+// in this workspace that are not imported by the
+// given file.
+func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle) ([]string, error) {
+ pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
+ if err != nil {
+ return nil, fmt.Errorf("GetParsedFile: %w", err)
+ }
+ alreadyImported := map[string]struct{}{}
+ for _, imp := range pgf.File.Imports {
+ alreadyImported[imp.Path.Value] = struct{}{}
+ }
+ pkgs, err := snapshot.KnownPackages(ctx)
+ if err != nil {
+ return nil, err
+ }
+ visited := map[string]struct{}{}
+ var resp []string
+ for _, knownPkg := range pkgs {
+ path := knownPkg.PkgPath()
+ gofiles := knownPkg.CompiledGoFiles()
+ if len(gofiles) == 0 || gofiles[0].File.Name == nil {
+ continue
+ }
+ pkgName := gofiles[0].File.Name.Name
+ // package main cannot be imported
+ if pkgName == "main" {
+ continue
+ }
+ // no need to import what the file already imports
+ if _, ok := alreadyImported[path]; ok {
+ continue
+ }
+ // snapshot.KnownPackages could have multiple versions of a pkg
+ if _, ok := visited[path]; ok {
+ continue
+ }
+ visited[path] = struct{}{}
+ // make sure internal packages are importable by the file
+ if !isValidImport(pkg.PkgPath(), path) {
+ continue
+ }
+ // naive check on cyclical imports
+ if isDirectlyCyclical(pkg, knownPkg) {
+ continue
+ }
+ resp = append(resp, path)
+ }
+ sort.Slice(resp, func(i, j int) bool {
+ importI, importJ := resp[i], resp[j]
+ iHasDot := strings.Contains(importI, ".")
+ jHasDot := strings.Contains(importJ, ".")
+ if iHasDot && !jHasDot {
+ return false
+ }
+ if jHasDot && !iHasDot {
+ return true
+ }
+ return importI < importJ
+ })
+ return resp, nil
+}
+
+// isDirectlyCyclical checks if improted directly imports pkg.
+// It does not (yet) offer a full cyclical check because showing a user
+// a list of importable packages already generates a very large list
+// and having a few false positives in there is worth the performance
+// snappiness.
+func isDirectlyCyclical(pkg, imported Package) bool {
+ for _, imp := range imported.Imports() {
+ if imp.PkgPath() == pkg.PkgPath() {
+ return true
+ }
+ }
+ return false
+}
+
+func isValidImport(pkgPath, importPkgPath string) bool {
+ if strings.HasPrefix(importPkgPath, "internal/") {
+ return false
+ }
+ if i := strings.LastIndex(importPkgPath, "/internal/"); i > -1 {
+ return strings.HasPrefix(pkgPath, importPkgPath[:i])
+ }
+ if i := strings.LastIndex(importPkgPath, "/internal"); i > -1 {
+ return strings.HasPrefix(pkgPath, importPkgPath[:i])
+ }
+ return true
+}
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index bc670db..0f6e96b 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -935,6 +935,7 @@
}
func (r *runner) FunctionExtraction(t *testing.T, start span.Span, end span.Span) {}
func (r *runner) CodeLens(t *testing.T, uri span.URI, want []protocol.CodeLens) {}
+func (r *runner) AddImport(t *testing.T, uri span.URI, expectedImport string) {}
func spanToRange(data *tests.Data, spn span.Span) (*protocol.ColumnMapper, protocol.Range, error) {
m, err := data.Mapper(spn.URI())
diff --git a/internal/lsp/testdata/addimport/addimport.go.golden b/internal/lsp/testdata/addimport/addimport.go.golden
new file mode 100644
index 0000000..9605aa6
--- /dev/null
+++ b/internal/lsp/testdata/addimport/addimport.go.golden
@@ -0,0 +1,7 @@
+-- addimport --
+package addimport //@addimport("", "bytes")
+
+import "bytes"
+
+func main() {}
+
diff --git a/internal/lsp/testdata/addimport/addimport.go.in b/internal/lsp/testdata/addimport/addimport.go.in
new file mode 100644
index 0000000..07b454f
--- /dev/null
+++ b/internal/lsp/testdata/addimport/addimport.go.in
@@ -0,0 +1,3 @@
+package addimport //@addimport("", "bytes")
+
+func main() {}
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index bc44f36..ba2f8af 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -73,6 +73,7 @@
type WorkspaceSymbols map[WorkspaceSymbolsTestType]map[span.URI][]string
type Signatures map[span.Span]*protocol.SignatureHelp
type Links map[span.URI][]Link
+type AddImport map[span.URI]string
type Data struct {
Config packages.Config
@@ -106,6 +107,7 @@
WorkspaceSymbols WorkspaceSymbols
Signatures Signatures
Links Links
+ AddImport AddImport
t testing.TB
fragments map[string]string
@@ -146,6 +148,7 @@
WorkspaceSymbols(*testing.T, span.URI, string, WorkspaceSymbolsTestType)
SignatureHelp(*testing.T, span.Span, *protocol.SignatureHelp)
Link(*testing.T, span.URI, []Link)
+ AddImport(*testing.T, span.URI, string)
}
type Definition struct {
@@ -291,6 +294,7 @@
WorkspaceSymbols: make(WorkspaceSymbols),
Signatures: make(Signatures),
Links: make(Links),
+ AddImport: make(AddImport),
t: t,
dir: dir,
@@ -445,6 +449,7 @@
"extractfunc": datum.collectFunctionExtractions,
"incomingcalls": datum.collectIncomingCalls,
"outgoingcalls": datum.collectOutgoingCalls,
+ "addimport": datum.collectAddImports,
}); err != nil {
t.Fatal(err)
}
@@ -784,6 +789,15 @@
}
})
+ t.Run("AddImport", func(t *testing.T) {
+ t.Helper()
+ for uri, exp := range data.AddImport {
+ t.Run(uriName(uri), func(t *testing.T) {
+ tests.AddImport(t, uri, exp)
+ })
+ }
+ })
+
if *UpdateGolden {
for _, golden := range data.golden {
if !golden.Modified {
@@ -1075,6 +1089,10 @@
data.Imports = append(data.Imports, spn)
}
+func (data *Data) collectAddImports(spn span.Span, imp string) {
+ data.AddImport[spn.URI()] = imp
+}
+
func (data *Data) collectSemanticTokens(spn span.Span) {
data.SemanticTokens = append(data.SemanticTokens, spn)
}
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Marwan Sulaiman uploaded patch set #2 to this change.
internal/lsp: add knownPackages and addImport as nonStandardRequests
This CL adds two new non-standard methods that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/lsp_test.go
M internal/lsp/server.go
A internal/lsp/source/add_import.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
9 files changed, 273 insertions(+), 0 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Marwan Sulaiman uploaded patch set #3 to this change.
internal/lsp: add knownPackages and addImport as nonStandardRequests
This CL adds two new non-standard methods that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/lsp_test.go
M internal/lsp/server.go
A internal/lsp/source/add_import.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
9 files changed, 273 insertions(+), 0 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Marwan Sulaiman uploaded patch set #4 to this change.
internal/lsp: add knownPackages and addImport as nonStandardRequests
This CL adds two new non-standard methods that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/lsp_test.go
M internal/lsp/server.go
A internal/lsp/source/add_import.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
9 files changed, 273 insertions(+), 0 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Rebecca Stambler.
Rebecca Stambler removed Ian Cottrell from this change.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
1 comment:
Patchset:
I believe that Rob mentioned on Slack that we'd prefer to do this via commands instead of non-standard requests, and Rob currently has a stack of CLs that will make commands easier to document (CL 289691). After that stack is merged, would you mind converting this to use commands?
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Marwan Sulaiman uploaded patch set #5 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/command.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
10 files changed, 352 insertions(+), 45 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Marwan Sulaiman uploaded patch set #6 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/command.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
10 files changed, 356 insertions(+), 49 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
Patchset:
I believe that Rob mentioned on Slack that we'd prefer to do this via commands instead of non-standa […]
I just converted it to commands. But happy to change things further if needed after Rob's refactors are in. I was just curious to see what switching from nonStandardRequest to workspace/executeCommand looked like, and it was pretty easy!
Thanks
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #7 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
11 files changed, 264 insertions(+), 29 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #8 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
13 files changed, 278 insertions(+), 38 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman.
11 comments:
File internal/lsp/lsp_test.go:
arg := fmt.Sprintf(`{"URI": "%s"}`, string(uri))
resp, err := r.server.executeCommand(r.ctx, &protocol.ExecuteCommandParams{
Command: "gopls.list_known_packages",
Arguments: []json.RawMessage{[]byte(arg)},
})Use command.NewListKnownPackagesCommand here (and then access Title and Arguments from the response). This will make this part type-safe and eliminate the coupling to the command name.
Patch Set #8, Line 1114:
res, ok :=This is fine, but FWIW I would just do res := resp.(command.ListKnownPackagesResult).
Patch Set #8, Line 1116:
expected response to be []string but got %TStale error message, but also we usually follow got-before-want style for test errors, as well as trying to identify the context. So maybe this could be
t.Fatalf("%s: got result of type T%, want []string", command.ListKnownPackages, resp)
But all this is moot if you skip the commaok and just panic, which is fine too :)
Patch Set #8, Line 1126:
knownPackagesnit: just 'known packages', since knownPackages isn't an identifier in this context?
Patch Set #8, Line 1126:
expected %q to be in list of knownPackages of length %dIf possible, I'd also restructure this to be got-before-want (assuming the list is not too long), i.e.
t.Fatalf("%s: got\n%v\nwant contains %q", command.ListKnownPackages, resp.Packages, expectedImport)
arg = fmt.Sprintf(
`{
"URI": "%s",
"ImportPath": "%s"
}`,
string(uri),
expectedImport,
)
_, err = r.server.executeCommand(r.ctx, &protocol.ExecuteCommandParams{
Command: "gopls.add_import",
Arguments: []json.RawMessage{[]byte(arg)},
})Use command.NewAddImportCommand here (and then access Title and Arguments from the result).
Patch Set #8, Line 1143:
(<-r.editRecv)[uri]I just added this ~hack to facilitate my refactoring. Seeing this now makes me realize that we really need to have a non-blocking helper that fails if no edits were received.
Just a note -- I'll do this in a follow-up.
Patch Set #8, Line 1151:
t.Error(diff)Absent context, this diff is likely to be of unclear meaning.
https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures(for example, you could identify that this is the result of command.AddImport)
File internal/lsp/source/known_packages.go:
Patch Set #8, Line 11:
in this workspaceTo be clear, this returns *all* loaded packages (meaning anything in the package graph), not just workspace packages. Is that what you want? (I'm not familiar with this functionality in vscode)
If so, this docstring could perhaps be clarified.
Patch Set #8, Line 13:
[]stringIs it really sufficient just to return import paths here? Don't you also need the package name to implement the feature correctly? (i.e., if you need to import q "example.com/p")
Patch Set #8, Line 73:
improted*imported
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Robert Findley.
10 comments:
Patchset:
Thank you for the review! Pushing CL updates in a bit.
File internal/lsp/lsp_test.go:
arg := fmt.Sprintf(`{"URI": "%s"}`, string(uri))
resp, err := r.server.executeCommand(r.ctx, &protocol.ExecuteCommandParams{
Command: "gopls.list_known_packages",
Arguments: []json.RawMessage{[]byte(arg)},
})
Use command. […]
Ack
Patch Set #8, Line 1116:
expected response to be []string but got %T
Stale error message, but also we usually follow got-before-want style for test errors, as well as tr […]
Ack
Patch Set #8, Line 1126:
expected %q to be in list of knownPackages of length %d
If possible, I'd also restructure this to be got-before-want (assuming the list is not too long), i. […]
Sounds good. ICYMI, printing the entire resp.Packages (as opposed to len(resp.Packages)) could make the test failure a little rough to read as it may have hundreds of elements. But I'll go with your suggestion to %v the whole list and I'm happy to switch it back to len(resp.Pacakges) if needed 👍
Patch Set #8, Line 1126:
knownPackagesnit: just 'known packages', since knownPackages isn't an identifier in this context?
Ack
arg = fmt.Sprintf(
`{
"URI": "%s",
"ImportPath": "%s"
}`,
string(uri),
expectedImport,
)
_, err = r.server.executeCommand(r.ctx, &protocol.ExecuteCommandParams{
Command: "gopls.add_import",
Arguments: []json.RawMessage{[]byte(arg)},
})Use command.NewAddImportCommand here (and then access Title and Arguments from the result).
Ack
Patch Set #8, Line 1151:
t.Error(diff)
Absent context, this diff is likely to be of unclear meaning. […]
Cool cool, just fyi this is the pattern in the rest of the file but I'm happy to add more context along with the diff 💯
File internal/lsp/source/known_packages.go:
Patch Set #8, Line 11:
in this workspace
To be clear, this returns *all* loaded packages (meaning anything in the package graph), not just wo […]
Ack
Patch Set #8, Line 13:
[]string
Is it really sufficient just to return import paths here? Don't you also need the package name to im […]
The VSCode implementation only returns a list of import paths without package names, which works for 98% of the time. The cases where you must specify a package name I imagine is when you have a conflict with another import of the same name, in which case we'll need the user to manually add the package name anyway. Another case is when the last path segment is not the same as the package name such as:
import hello "example.com/go-hello"in which case go-imports will add that for you the moment you pick the dependency as well which is a nice enhancement over the current VSCode implementation.
Patch Set #8, Line 73:
improted*imported
Ack
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Robert Findley.
Marwan Sulaiman uploaded patch set #9 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
13 files changed, 278 insertions(+), 38 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Robert Findley.
Marwan Sulaiman uploaded patch set #10 to this change.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
5 comments:
File internal/lsp/command.go:
Patch Set #9, Line 592:
showAsyncErrseems like this function only has one caller--maybe it can be inlined?
File internal/lsp/source/add_import.go:
Patch Set #9, Line 1:
package sourceneeds copyright notice
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 1:
package sourcethis file needs a copyright notice at the top (you can just copy it from another file and update the date)
// package main cannot be imported
if pkgName == "main" {
continue
}I think you may also need a case for test packages here? You can use the package's "ForTest" function to check if it's a test.
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {can you reuse or export isValidImport from the internal/lsp/cache package? https://github.com/golang/tools/blob/123adc86bcb6c2a95bc058e93df2f247c11a9629/internal/lsp/cache/check.go#L715
If you export it, you will have to move it into the source package.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Robert Findley (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Patch set 10:Run-TryBot +1Trust +1
kokoro (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/d817c4ec-5da3-4034-a127-93ac5c289594
Patch set 10:gopls-CI -1
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
6 comments:
File internal/lsp/command.go:
Patch Set #9, Line 592:
showAsyncErrseems like this function only has one caller--maybe it can be inlined?
Ack
File internal/lsp/source/add_import.go:
Patch Set #9, Line 1:
package sourceneeds copyright notice
Ack
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 1:
package source
this file needs a copyright notice at the top (you can just copy it from another file and update the […]
Ack
Patch Set #9, Line 1:
package source
this file needs a copyright notice at the top (you can just copy it from another file and update the […]
Ack
// package main cannot be imported
if pkgName == "main" {
continue
}
I think you may also need a case for test packages here? You can use the package's "ForTest" functio […]
Ack
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
can you reuse or export isValidImport from the internal/lsp/cache package? https://github. […]
I remember looking at that function and I believe its implementation is a little different as it allows internal Go libraries such as `internal/profile` and others to be valid. It also doesn't account for packages _ending_ with "internal". So I opted for creating this one specifically for "what can be imported" right under where it's being used.
Happy to merge the two and make it configurable. Or any other suggestions are very welcome 👍
Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
Marwan Sulaiman uploaded patch set #11 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
13 files changed, 286 insertions(+), 38 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackagesSo sorry about the confusion on this, but Heschi actually just pointed out that this logic is very similar to the gopls unimported completions code. He suggests starting with a call to GetAllCandidates (https://pkg.go.dev/golang.org/x/tools/internal/imports?utm_source=gopls#GetAllCandidates) and modeling the code off of (*completer).unimportedPackages (https://github.com/golang/tools/blob/123adc86bcb6c2a95bc058e93df2f247c11a9629/internal/lsp/source/completion/completion.go#L1407).
What do you think?
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
So sorry about the confusion on this, but Heschi actually just pointed out that this logic is very s […]
No problem! Happy to do whatever you all think is best.
That said, I'm looking at the implementation of GetAllCandidates and I noticed two issues:
1. It currently takes about 9 seconds to return the list of packages to the user (when running the command with this file as an argument). That's quite a long time compared to the 10.5 milliseconds it takes to just run `snapshot.KnownPackages`. The second time I run GetAllCandidates, it's much faster but still a pretty noticeable jitter (about 1-2 seconds).
2. GetAllCandidates seems to be returning a very large number of packages. Most of them unrelated to the module I'm operating within, so I imagine I'd need to do some extra logic to exclude those ones?
I'm gonna continue digging but just wanted to double check in case I'm missing anything. Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
No problem! Happy to do whatever you all think is best. […]
I've never used the add import feature in vscode, so I assumed that it was intended to work basically the same way as package completions in gopls. Looking at it now I guess it's more like import path completions; it might make sense to use populateImportCompletions as a reference instead.
The delay you're seeing is the time to scan your module cache, which can be costly, especially on Macs. The imports calls attempt to prioritize more relevant packages (i.e. packages in the main module, then directly required modules, etc.) and honor timeouts, so our strategy is to pick a responsiveness target (100ms) and use that as the timeout on an inner context. You should still get decent results.
If you still have responsiveness concerns, you can start with CachedImportPaths (don't use KnownPackages, package paths are not import paths) and merge in the results from unimported packages.
Unimported completions are unfortunately a little buggy right now; they don't enforce all the importability checks that they should on in-memory packages, only on on-disk packages. At some point I would like to unify the two codepaths so that in-memory results flow through the same checks as on-disk results do. I personally wouldn't worry about them for this feature. For example, you're missing vendor checks.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
I realized I wrote a little too much (mainly for my own knowledge curiosity). So here's a quick TL;DR:
- Yes, I'd love to start with CachedImportPaths as a first step and in a follow up CL I'll see if I can use imports.GetAllCandidates to provide better results? I'll push the small change in a minute :)
----
Looking at it now I guess it's more like import path completions;
The difference being is that the client asks for a list of all import paths before the user types anything. The client is then responsible for fuzzy searching based on the returned list. This is useful for when users aren't sure what they are looking for yet and haven't begun to type anything. It also makes the search experience snappier given that it doesn't need to communicate back and forth and holds all the import paths ahead of time client side. So far I've tested it with a list of ~500 import paths and there's no delay.
it might make sense to use populateImportCompletions as a reference instead.
I think the same reasons above applies here given the godoc for this method. We wanna return the whole list ahead of time and not be at the directory level, but rather the main module level.
don't use KnownPackages, package paths are not import paths
For my own education, I'd love to know how a package path might be different from na import path.
The imports calls attempt to prioritize more relevant packages (i.e. packages in the main module, then directly required modules, etc.) and honor timeouts, so our strategy is to pick a responsiveness target (100ms) and use that as the timeout on an inner context
I think that's exactly what I'm looking for. Does that mean I can just create a context.WithTimeout(ctx, 100ms) and pass it to imports.GetAllCandidates?
For example, you're missing vendor checks.
FWIW, I just tested the method above against a vendor'd module and it didn't show any vendor prefixed import paths.
Thanks again to you and everyone for adding their reviews here and helping me understand the codebase more 🙏
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #12 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
13 files changed, 286 insertions(+), 38 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman.
2 comments:
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
I realized I wrote a little too much (mainly for my own knowledge curiosity). […]
CachedImportPaths may not give you quite the user experience you want. It returns all the packages that gopls knows about, but it only knows about packages that are in the workspace or have already been loaded as dependencies. So if the user has imported example.com/a and now wants to import example.com/b for the first time, only a will appear in the list, which will probably be suprising. (KnownPackages has the same problem.) GetAllCandidates will return everything, but is subject to the latency problems discussed. That's why we merge the results together.
Unfortunately, the imports codebase relies on the assumption that there will be more requests to get good latency. That is, if the user types "a", we don't need to find all packages matching "a", just enough to fill up their results box. Then when they come back with "abc", we can look more precisely. If this feature has to return all its results at once, that's potentially a problem. But at least to start you should try just a context with a short timeout, yes.
Package paths differ from import paths when GOPATH vendoring is involved; there the import path "example.com/a" may actually map to the package "example.com/project/vendor/example.com/a". That's the same case that you're missing the checks for. (It's possible we no longer care about GOPATH mode, but still.)
File internal/lsp/source/known_packages.go:
Patch Set #12, Line 33:
path := knownPkg.PkgPath()You should use the map key here, that's the major difference between the two.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Heschi Kreinick.
2 comments:
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
CachedImportPaths may not give you quite the user experience you want. […]
Got it. So do you suggest that I also merge CachedImportPaths with GetAllCandidates or should I remove pretty much remove a lot of the code here and just replace it with GetAllCandidates+timeouts? Happy with either but I just wanted to double check 😊
As for imports assuming a user is typing, yes I agree it's a problem for this feature. I think ideally there should be a method that just says "give me every single package that this main-module can import, nothing more, nothing less". This way it will show example.com/a _and_ example.com/b and it will _not_ traverse the entire go modules cache directory and having to prioritize relative imports.
I'd be happy to try and create a function like this in a follow up CL if that's something you all think is worth having.
File internal/lsp/source/known_packages.go:
Patch Set #12, Line 33:
path := knownPkg.PkgPath()You should use the map key here, that's the major difference between the two.
Ack
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
Got it. […]
I think the requirements you're stating with are just tough to satisfy with the options on we have handy. In module mode, at least, the easiest answer to the precise question you asked is to run `go list all`, then filter for unusable packages. (I'm not sure how `go list all` works in GOPATH mode.) `go list all` should be relatively quick, though not instant. So that's one option.
Then, the other option is to tweak the imports package to allow a mode where you only scan in-scope packages and wait forever to do it. That will be a little tricky but all the data you need (the list of in-scope modules) is already available.
Other than that, yeah, merging the two data source is realistically the best bet. It's still not guaranteed given an arbitrary latency restriction, but it should be fine most of the time.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman.
Marwan Sulaiman uploaded patch set #13 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
13 files changed, 313 insertions(+), 38 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
I think the requirements you're stating with are just tough to satisfy with the options on we have h […]
Hi Heschi,
I tried 'go list all' and unfortunately it does not return packages whose module is in the go.mod file but not yet imported by a Go file. It's easily reproduced with the following commands:```
$ go mod init repro
go: creating new go.mod: module repro
$ go get github.com/pkg/errors
go get: added github.com/pkg/errors v0.9.1
$ go list all
repro
# no pkg/errors
```So in a way, 'go list all' works similar to CachedImportPaths?
I think ideally there would be an implementation that more or less answers the question: "Given a Go file, list all the importable packages to the module it belongs to". So maybe the implementation will just have to ask for the go.mod file and traverses each package within each module, and hopefully with some smart caching and multi-module mode considerations.
Let me know what you think. Of course in the meantime, I did what you suggested which is to merge CachedImportPaths with a timed-out GetAllCandidates. Would love another look there.
Thanks!
PS. I really appreciate the back and forth here to make sure we have a great UX. The current vscode 'Add Import' feature is far from what we're aiming for here and CachedImportPaths on its own is already miles better.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #14 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
13 files changed, 311 insertions(+), 38 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley.
8 comments:
Patchset:
Let me know if you're running out of steam and I can stop picking nits.
File internal/lsp/command/interface.go:
importable packages
// that the URIArg can choose to addpackages importable from the given URI?
Patch Set #14, Line 123:
Note that the method returns the edits but also calls applyEditOutdated?
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given fileThis is probably better tested with a regression test, I'm afraid. See gopls/internal/regtest. You should be able to use ExecuteCommand to invoke AddImport.
File internal/lsp/source/known_packages.go:
Patch Set #8, Line 13:
[]stringThe VSCode implementation only returns a list of import paths without package names, which works for […]
I don't think there are any real obstacles to returning the correct import name at this point: GetAllCandidates gives you an ImportFix and you can compute the correct fix for CachedImportPaths same as the completion code. Then return those and pass them (as you're already doing) to ComputeOneImportFixEdits.
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {I remember looking at that function and I believe its implementation is a little different as it all […]
Those are just bugs and they should be fixed, unless I'm missing something?
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
Hi Heschi, […]
Sigh. You would think that at some point I'd learn how the go command works. In that case, the closest thing might be something like `go list -f '{{.Path}}/...' -m all | xargs go list`. But in experimenting I realized that's got problems of its own: just because a package is theoretically importable doesn't mean it's downloaded and ready for use. And you probably don't want this command to trigger a network operation.
So realistically, this is the the best architecture you're likely to get, short of having a way to only scan in-scope modules in the imports package.
File internal/lsp/source/known_packages.go:
Patch Set #14, Line 91:
sort.Slice(paths, func(i, j int) bool {An enhancement would be to use Relevance to sort them as the completion code does.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Robert Findley, Heschi Kreinick.
2 comments:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
Those are just bugs and they should be fixed, unless I'm missing something?
I'd imagine if you are using gopls to work on Go itself, it may be more of a feature than a bug. But I'll let Rebecca, or Rob confirm here before making changes.
File internal/lsp/source/known_packages.go:
Patch Set #14, Line 91:
sort.Slice(paths, func(i, j int) bool {An enhancement would be to use Relevance to sort them as the completion code does.
For sure, but I was hoping to keep the behavior consistent with how VSCode currently works. I'm happy to change it to using Relevance, let me know!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
2 comments:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
I'd imagine if you are using gopls to work on Go itself, it may be more of a feature than a bug. […]
I'm not too familiar with the internal rules/standard library, so I'm not sure how the internal rules would be different for it. Marwan, can you clarify what you mean here?
File internal/lsp/source/known_packages.go:
Patch Set #14, Line 91:
sort.Slice(paths, func(i, j int) bool {
For sure, but I was hoping to keep the behavior consistent with how VSCode currently works. […]
I think it probably makes sense to sort by relevance, but also fine if you'd prefer to leave it as an exact match with the current VS Code feature and then improve it later on.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Robert Findley, Heschi Kreinick.
2 comments:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
I'm not too familiar with the internal rules/standard library, so I'm not sure how the internal rule […]
The isValidImport logic checks for "/internal/" and not "internal/" as shown here: https://github.com/golang/tools/blob/123adc86bcb6c2a95bc058e93df2f247c11a9629/internal/lsp/cache/check.go#L716
So this gave me the impression that it wanted to allow for Go's internal packages such as "internal/cfg".
I think my idea was that if you are working on the Go language codebase, you'd like to see packages such as "internal/trace", "internal/cpu", etc. But if you are not working on the Go codebase, you probably never wanna see those as an option because 100% of the time, they are not importable.
In other words, most third party libraries and projects, will always have some namespace. such as "github.com/user/pkg" and if they have an internal library it means it will always be prefixed with "/" due to "github.com/user/pkg/internal".
So the isValidImport will need to make a decision:
1. Either allow all "internal/*" to be included (which is almost always wrong unless you're working on Go itself)
2. Or never allow any "internal/*" to be included (which is almost always right, but feels like a bug for those who work on Go.)
3. Add some magical logic so that it can distinguish whether you are working inside Go or outside of it (and therefore we can use the same isValidImport as you initially suggested)
4. Keep the two functions separated so we don't break anything until we make a more correct decision.Let me know which of the 4 options you prefer! Or if there's any other option which I'll be happy to go with.
Thanks
File internal/lsp/source/known_packages.go:
Patch Set #14, Line 91:
sort.Slice(paths, func(i, j int) bool {
I think it probably makes sense to sort by relevance, but also fine if you'd prefer to leave it as a […]
Sounds good. I think it would be better to keep it as is and iterate later on. Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
The isValidImport logic checks for "/internal/" and not "internal/" as shown here: https://github. […]
Thanks for enumerating all of this! I think option 1 sounds like the easiest first step, with 3 being an eventual goal but something for a separate CL. Does that sound OK?
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Marwan Sulaiman uploaded patch set #15 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M gopls/go.mod
M internal/lsp/cache/check.go
M internal/lsp/cache/load.go
M internal/lsp/cache/snapshot.go
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
M internal/lsp/source/util.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
18 files changed, 325 insertions(+), 68 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
Thanks for enumerating all of this! I think option 1 sounds like the easiest first step, with 3 bein […]
Hi Rebecca,
Apologies for the long delay! I just applied option 1 and resolved all merge conflicts. I should be able to get this over to the finish line now that I have some time back.Let me know if there's anything else for me to do for this CL.
Thanks :)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
Hi Rebecca, […]
Awesome--no worries! Do you mind looking at the unresolved comments and resolving them if you've addressed them? Then I can take another look.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
3 comments:
File internal/lsp/command/interface.go:
importable packages
// that the URIArg can choose to addpackages importable from the given URI?
Ack
Patch Set #14, Line 123:
Note that the method returns the edits but also calls applyEditOutdated?
Ack
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
This is probably better tested with a regression test, I'm afraid. See gopls/internal/regtest. […]
Sounds great. Though I'd def be curious to learn what might be the shortfalls of the current tests :)
I looked at regtest and I am blocked by one thing: there doesn't seem to be a way to _wait_ for the server to call `s.client.ApplyEdit` and wait for that edit to finish.
I see a bunch of ways to wait to wait for different events in regtest/expectation.go but I haven't been able to find one that will wait for the above condition.
Let me know if the current tests are sufficient or if there's a way around the above issue.
Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #16 to this change.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
Awesome--no worries! Do you mind looking at the unresolved comments and resolving them if you've add […]
I believe I addressed all the comments, potentially pending a final response from Heschi regarding testing. But happy to wait or send a follow up CL if need be.
Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman.
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
Sounds great. […]
ApplyEdit is a synchronous call, and so is the ExecuteCommand that will trigger it, so there's no need to wait for anything specially.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman.
Patch set 16:Run-TryBot +1Trust +1
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley.
Patch set 17:Run-TryBot +1Trust +1
2 comments:
File internal/lsp/lsp_test.go:
Patch Set #8, Line 1126:
expected %q to be in list of knownPackages of length %dSounds good. ICYMI, printing the entire resp.Packages (as opposed to len(resp. […]
Done
Patch Set #8, Line 1151:
t.Error(diff)Cool cool, just fyi this is the pattern in the rest of the file but I'm happy to add more context al […]
Resolving, seems fixed
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/dfd85e7f-0b8e-4a14-ae15-f98aca9efb22
Patch set 16:gopls-CI -1
kokoro (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/9d914ce3-d0be-4569-9ed5-30763581d1f6
Patch set 17:gopls-CI -1
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
ApplyEdit is a synchronous call, and so is the ExecuteCommand that will trigger it, so there's no ne […]
Hi Heschi,
Sorry if I wasn't clear. It's the fact that the command is actually called asynchronously. I'm not sure how to link to a specific line in Gerrit, but if you look at `command.go:680` of this CL you'll see a line that says `async: true`.We can potentially make that false and change things up. Lemme know
Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
7 comments:
File gopls/go.mod:
Patch Set #17, Line 1:
module golang.org/x/tools/goplsCan you please revert this change to the go.mod file? It is tidied with Go 1.17, which is why it has the extra indirect requirements.
File internal/lsp/command.go:
Patch Set #17, Line 347:
return fmt.Errorf("running tests failed: %w", err)Why was this changed?
_, err = c.s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{
Edit: protocol.WorkspaceEdit{
DocumentChanges: documentChanges(deps.fh, edits),
},
})
if err != nil {
return fmt.Errorf("could not apply import edits: %v", err)
}nit: you can inline this to
if _, err := c.s.client.ApplyEdit(...); err != nil {
...
}
File internal/lsp/lsp_test.go:
// so that when tests call server.executeCommand which triggers a command message
// this client does not panic.Not needed I think
File internal/lsp/source/known_packages.go:
Patch Set #8, Line 13:
[]string
I don't think there are any real obstacles to returning the correct import name at this point: GetAl […]
Shall we do this in a follow-up?
File internal/lsp/source/known_packages.go:
Patch Set #9, Line 87:
func isValidImport(pkgPath, importPkgPath string) bool {
I believe I addressed all the comments, potentially pending a final response from Heschi regarding t […]
Sounds good. Let me know if you need additional guidance on the test
File internal/lsp/source/known_packages.go:
Patch Set #11, Line 17:
KnownPackages
Sigh. You would think that at some point I'd learn how the go command works. […]
Resolving
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
4 comments:
File gopls/go.mod:
Can you please revert this change to the go.mod file? It is tidied with Go 1. […]
Ack
File internal/lsp/command.go:
Patch Set #17, Line 347:
return fmt.Errorf("running tests failed: %w", err)Why was this changed?
TL;DR: this was just got moved up to lines 111-120 on this file, so that we don't need to duplicate the "c.s.client.ShowMessage" call on every async:true command.
Longer version:
Because when "async" is set to "true" like line 341 above, the error returned from "run" was completely ignored. As you can see in the deleted comments on the left side it says:
// Since we're running asynchronously, any error returned here would be
// ignored.Therefore, this means if you wanna return an "async error" back to the client to show as a message, you'd need to do it _for every async command_ by manually calling "c.s.client.ShowMessage(<err>)"
However, there's no reason to ignore the error if the call was async...We can just default to showing an async error like I have in lines.
This means we don't need to duplicate the logic everywhere and we don't need to violate the function signature and having to put a commend that says "FYI, the returned error is totally ignored".
Definitely happy to change it back though, just let me know!
_, err = c.s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{
Edit: protocol.WorkspaceEdit{
DocumentChanges: documentChanges(deps.fh, edits),
},
})
if err != nil {
return fmt.Errorf("could not apply import edits: %v", err)
}
nit: you can inline this to […]
Sounds good, it won't be on the same line if that's Okay. Since the argument is pretty large. I can also define the argument above the method call if you'd like 👍
File internal/lsp/lsp_test.go:
// so that when tests call server.executeCommand which triggers a command message
// this client does not panic.Not needed I think
Ack
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #18 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
M internal/lsp/cache/check.go
M internal/lsp/cache/load.go
M internal/lsp/cache/snapshot.go
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
M internal/lsp/source/util.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
17 files changed, 322 insertions(+), 64 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Patch set 18:Run-TryBot +1Trust +1
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
3 comments:
File internal/lsp/command.go:
Patch Set #17, Line 347:
return fmt.Errorf("running tests failed: %w", err)
TL;DR: this was just got moved up to lines 111-120 on this file, so that we don't need to duplicate […]
Sounds good thanks for the explanation!
_, err = c.s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{
Edit: protocol.WorkspaceEdit{
DocumentChanges: documentChanges(deps.fh, edits),
},
})
if err != nil {
return fmt.Errorf("could not apply import edits: %v", err)
}
Sounds good, it won't be on the same line if that's Okay. Since the argument is pretty large. […]
Yep, you did what I meant -- should've clarified
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
Hi Heschi, […]
You can add env.Await(env.DoneWithChange()) after the execute command call to wait for the file changes to complete. Would that work?
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/0e86fdcc-6218-4e45-ab67-a6819a1e8793
Patch set 18:gopls-CI -1
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
2 comments:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
You can add env.Await(env. […]
Adding `env.Await(env.DoneWithChange())` does work for me. Though I'm surprised that worked because how would it know there was a random goroutine that was about to change a client file? Unless the goroutine happens to be quick enough to trigger a change before we get to env.DoneWithChange(). If so, this might mean the test could be flaky on slower/different machines?
In any case, now that I have a regression test working I'll push it up now but I'm happy to make further changes or remove it. Whatever y'all think is best.
Thanks!
File internal/lsp/source/known_packages.go:
Patch Set #8, Line 13:
[]string
Shall we do this in a follow-up?
That sounds good to me 👍
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #19 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
A gopls/internal/regtest/misc/add_import_test.go
M internal/lsp/cache/check.go
M internal/lsp/cache/load.go
M internal/lsp/cache/snapshot.go
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
M internal/lsp/source/util.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
18 files changed, 378 insertions(+), 64 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Heschi Kreinick (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley.
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
Adding `env.Await(env.DoneWithChange())` does work for me. […]
FWIW, I don't think makes much sense for a command that does edits to be asynchronous -- the user should probably not be editing the file until the command finishes. And it's not like this is going to take more than a few milliseconds.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
FWIW, I don't think makes much sense for a command that does edits to be asynchronous -- the user sh […]
I'd be happy to turn it into a synchronous call. The only drawback I can think of is if there's a single-threaded client (or more accurately a synchronous client) that can only process one rpc at a time. Such client would end up getting stuck forever.
Let me know and I can change this now or at a follow up CL.
Thanks!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Patch set 19:Trust +1
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
I'd be happy to turn it into a synchronous call. […]
I think it would be best to make it synchronous in this CL if you don't mind
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #20 to this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
A gopls/internal/regtest/misc/add_import_test.go
M internal/lsp/cache/check.go
M internal/lsp/cache/load.go
M internal/lsp/cache/snapshot.go
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
M internal/lsp/source/util.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
18 files changed, 376 insertions(+), 64 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
I think it would be best to make it synchronous in this CL if you don't mind
Done!
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Marwan Sulaiman (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Marwan Sulaiman uploaded patch set #21 to this change.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Patch set 21:Run-TryBot +1Trust +1
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/590c16e5-33ac-4ec5-bbb6-a7b2b2d74546
Patch set 21:gopls-CI -1
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Rebecca Stambler uploaded patch set #22 to the change originally created by Marwan Sulaiman.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
---
M gopls/doc/commands.md
A gopls/internal/regtest/misc/add_import_test.go
M internal/lsp/cache/check.go
M internal/lsp/cache/load.go
M internal/lsp/cache/snapshot.go
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
M internal/lsp/source/util.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
18 files changed, 380 insertions(+), 64 deletions(-)
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/source/add_import.go:
Patch Set #14, Line 14:
// AddImport adds a single import statement to the given file
Done!
Done
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
1 comment:
File internal/lsp/source/known_packages.go:
Patch Set #8, Line 13:
[]string
That sounds good to me 👍
Ack
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/d0407fbe-9609-4464-b2c6-b7346f4ad0bb
Patch set 22:gopls-CI -1
Rebecca Stambler (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Rebecca Stambler uploaded patch set #23 to the change originally created by Marwan Sulaiman.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/911c7aca-9740-45e7-94e3-808a76b81f8a
Patch set 23:gopls-CI -1
Rebecca Stambler (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Rebecca Stambler uploaded patch set #24 to the change originally created by Marwan Sulaiman.
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Rebecca Stambler, Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/cf2b3e9c-e317-48b5-b3d1-ee4cec2de737
Patch set 24:gopls-CI +1
Rebecca Stambler (Gerrit)
Attention is currently required from: Marwan Sulaiman, Robert Findley, Heschi Kreinick.
Patch set 24:Code-Review +2
Rebecca Stambler (Gerrit)
Rebecca Stambler submitted this change.
internal/lsp: add list_known_packages and add_import commands
This CL adds two new commands that let a client request a list of importable packages relative to a Go file and then select which import a programmer would like to add to said file.
Updates golang/go#43351
Change-Id: If12518874a92ed4167bdd711a92e03ee21c7b949
Reviewed-on: https://go-review.googlesource.com/c/tools/+/281412
Trust: Rebecca Stambler <rsta...@golang.org>
Trust: Robert Findley <rfin...@google.com>
Run-TryBot: Rebecca Stambler <rsta...@golang.org>
gopls-CI: kokoro <noreply...@google.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Rebecca Stambler <rsta...@golang.org>
---
M gopls/doc/commands.md
A gopls/internal/regtest/misc/add_import_test.go
M internal/lsp/cache/check.go
M internal/lsp/cache/load.go
M internal/lsp/cache/snapshot.go
M internal/lsp/cmd/test/cmdtest.go
M internal/lsp/command.go
M internal/lsp/command/command_gen.go
M internal/lsp/command/interface.go
M internal/lsp/lsp_test.go
A internal/lsp/source/add_import.go
M internal/lsp/source/api_json.go
A internal/lsp/source/known_packages.go
M internal/lsp/source/source_test.go
M internal/lsp/source/util.go
A internal/lsp/testdata/addimport/addimport.go.golden
A internal/lsp/testdata/addimport/addimport.go.in
M internal/lsp/tests/tests.go
18 files changed, 380 insertions(+), 64 deletions(-)
diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md
index 22ff379..9073d97 100644
--- a/gopls/doc/commands.md
+++ b/gopls/doc/commands.md
@@ -21,16 +21,21 @@
}
```
-### ****
+### **asks the server to add an import path to a given Go file.**
Identifier: `gopls.add_import`
-
+The method will call applyEdit on the client so that clients don't have
+to apply the edit themselves.
Args:
```
{
+ // ImportPath is the target import path that should
+ // be added to the URI file
"ImportPath": string,
+ // URI is the file that the ImportPath should be
+ // added to
"URI": string,
}
```
@@ -136,10 +141,10 @@
}
```
-### ****
+### **retrieves a list of packages**
Identifier: `gopls.list_known_packages`
-
+that are importable from the given URI.
Args:
diff --git a/gopls/internal/regtest/misc/add_import_test.go b/gopls/internal/regtest/misc/add_import_test.go
new file mode 100644
index 0000000..8eb96cf
--- /dev/null
+++ b/gopls/internal/regtest/misc/add_import_test.go
@@ -0,0 +1,59 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package misc
+
+import (
+ "testing"
+
+ "golang.org/x/tools/internal/lsp/command"
+ "golang.org/x/tools/internal/lsp/protocol"
+ . "golang.org/x/tools/internal/lsp/regtest"
+ "golang.org/x/tools/internal/lsp/tests"
+)
+
+func TestAddImport(t *testing.T) {
+ const before = `package main
+
+import "fmt"
+
+func main() {
+ fmt.Println("hello world")
+}
+`
+
+ const want = `package main
+
+import (
+ "bytes"
+ "fmt"
+)
+
+func main() {
+ fmt.Println("hello world")
+}
+`
+
+ Run(t, "", func(t *testing.T, env *Env) {
+ env.CreateBuffer("main.go", before)
+ cmd, err := command.NewAddImportCommand("Add Import", command.AddImportArgs{
+ URI: protocol.URIFromSpanURI(env.Sandbox.Workdir.URI("main.go").SpanURI()),
+ ImportPath: "bytes",
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, err = env.Editor.ExecuteCommand(env.Ctx, &protocol.ExecuteCommandParams{
+ Command: "gopls.add_import",
+ Arguments: cmd.Arguments,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := env.Editor.BufferText("main.go")
+ if got != want {
+ t.Fatalf("gopls.add_import failed\n%s", tests.Diff(t, want, got))
+ }
+ })
+}
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 13f9d1b..ddd0648 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -503,7 +503,7 @@
if dep == nil {
return nil, snapshot.missingPkgError(pkgPath)
}
- if !isValidImport(m.pkgPath, dep.m.pkgPath) {
+ if !source.IsValidImport(string(m.pkgPath), string(dep.m.pkgPath)) {
return nil, errors.Errorf("invalid use of internal package %s", pkgPath)
}
depPkg, err := dep.check(ctx, snapshot)
@@ -834,17 +834,6 @@
return nil
}
-func isValidImport(pkgPath, importPkgPath packagePath) bool {
- i := strings.LastIndex(string(importPkgPath), "/internal/")
- if i == -1 {
- return true
- }
- if isCommandLineArguments(string(pkgPath)) {
- return true
- }
- return strings.HasPrefix(string(pkgPath), string(importPkgPath[:i]))
-}
-
// An importFunc is an implementation of the single-method
// types.Importer interface based on a function value.
type importerFunc func(path string) (*types.Package, error)
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 520c5e6..29a07af 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -55,7 +55,7 @@
for _, scope := range scopes {
switch scope := scope.(type) {
case packagePath:
- if isCommandLineArguments(string(scope)) {
+ if source.IsCommandLineArguments(string(scope)) {
panic("attempted to load command-line-arguments")
}
// The only time we pass package paths is when we're doing a
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 94dca42..7cba3af 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -995,7 +995,7 @@
}
// If the package previously only had a command-line-arguments ID,
// we should just replace it.
- if isCommandLineArguments(string(existingID)) {
+ if source.IsCommandLineArguments(string(existingID)) {
s.ids[uri][i] = id
// Delete command-line-arguments if it was a workspace package.
delete(s.workspacePackages, existingID)
@@ -1012,14 +1012,6 @@
s.ids[uri] = append(newIDs, id)
}
-// isCommandLineArguments reports whether a given value denotes
-// "command-line-arguments" package, which is a package with an unknown ID
-// created by the go command. It can have a test variant, which is why callers
-// should not check that a value equals "command-line-arguments" directly.
-func isCommandLineArguments(s string) bool {
- return strings.Contains(s, "command-line-arguments")
-}
-
func (s *snapshot) isWorkspacePackage(id packageID) bool {
s.mu.Lock()
defer s.mu.Unlock()
@@ -1169,7 +1161,7 @@
func containsCommandLineArguments(pkgs []source.Package) bool {
for _, pkg := range pkgs {
- if isCommandLineArguments(pkg.ID()) {
+ if source.IsCommandLineArguments(pkg.ID()) {
return true
}
}
@@ -1237,7 +1229,7 @@
missingMetadata = true
// Don't try to reload "command-line-arguments" directly.
- if isCommandLineArguments(string(pkgPath)) {
+ if source.IsCommandLineArguments(string(pkgPath)) {
continue
}
pkgPathSet[pkgPath] = struct{}{}
@@ -1646,7 +1638,7 @@
// go command when the user is outside of GOPATH and outside of a
// module. Do not cache them as workspace packages for longer than
// necessary.
- if isCommandLineArguments(string(id)) {
+ if source.IsCommandLineArguments(string(id)) {
if invalidateMetadata, ok := idsToInvalidate[id]; invalidateMetadata && ok {
continue
}
diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go
index 869d4f5..b63a92a 100644
--- a/internal/lsp/cmd/test/cmdtest.go
+++ b/internal/lsp/cmd/test/cmdtest.go
@@ -100,6 +100,10 @@
//TODO: function extraction not supported on command line
}
+func (r *runner) AddImport(t *testing.T, uri span.URI, expectedImport string) {
+ //TODO: import addition not supported on command line
+}
+
func (r *runner) runGoplsCmd(t testing.TB, args ...string) (string, string) {
rStdout, wStdout, err := os.Pipe()
if err != nil {
diff --git a/internal/lsp/command.go b/internal/lsp/command.go
index bc521c9..e877ac1 100644
--- a/internal/lsp/command.go
+++ b/internal/lsp/command.go
@@ -54,7 +54,7 @@
// commandConfig configures common command set-up and execution.
type commandConfig struct {
- async bool // whether to run the command asynchronously. Async commands cannot return results.
+ async bool // whether to run the command asynchronously. Async commands can only return errors.
requireSave bool // whether all files must be saved for the command to work
progress string // title to use for progress reporting. If empty, no progress will be reported.
forURI protocol.DocumentURI // URI to resolve to a snapshot. If unset, snapshot will be nil.
@@ -108,7 +108,16 @@
return err
}
if cfg.async {
- go runcmd()
+ go func() {
+ if err := runcmd(); err != nil {
+ if showMessageErr := c.s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
+ Type: protocol.Error,
+ Message: err.Error(),
+ }); showMessageErr != nil {
+ event.Error(ctx, fmt.Sprintf("failed to show message: %q", err.Error()), showMessageErr)
+ }
+ }
+ }()
return nil
}
return runcmd()
@@ -335,15 +344,8 @@
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
if err := c.runTests(ctx, deps.snapshot, deps.work, args.URI, args.Tests, args.Benchmarks); err != nil {
- if err := c.s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
- Type: protocol.Error,
- Message: fmt.Sprintf("Running tests failed: %v", err),
- }); err != nil {
- event.Error(ctx, "running tests: failed to show message", err)
- }
+ return errors.Errorf("running tests failed: %w", err)
}
- // Since we're running asynchronously, any error returned here would be
- // ignored.
return nil
})
}
@@ -664,26 +666,33 @@
func (c *commandHandler) ListKnownPackages(ctx context.Context, args command.URIArg) (command.ListKnownPackagesResult, error) {
var result command.ListKnownPackagesResult
err := c.run(ctx, commandConfig{
- progress: "Listing packages", // optional, causes a progress report during command execution
- forURI: args.URI, // optional, populates deps.snapshot and deps.fh
+ progress: "Listing packages",
+ forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
- // Marwan: add implementation here. deps.snapshot and deps.fh are available for use.
- result.Packages = []string{}
- return nil
+ var err error
+ result.Packages, err = source.KnownPackages(ctx, deps.snapshot, deps.fh)
+ return err
})
return result, err
}
-
-func (c *commandHandler) AddImport(ctx context.Context, args command.AddImportArgs) (command.AddImportResult, error) {
- var result command.AddImportResult
- err := c.run(ctx, commandConfig{
+func (c *commandHandler) AddImport(ctx context.Context, args command.AddImportArgs) error {
+ return c.run(ctx, commandConfig{
progress: "Adding import",
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
- result.Edits = nil
+ edits, err := source.AddImport(ctx, deps.snapshot, deps.fh, args.ImportPath)
+ if err != nil {
+ return fmt.Errorf("could not add import: %v", err)
+ }
+ if _, err := c.s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{
+ Edit: protocol.WorkspaceEdit{
+ DocumentChanges: documentChanges(deps.fh, edits),
+ },
+ }); err != nil {
+ return fmt.Errorf("could not apply import edits: %v", err)
+ }
return nil
})
- return result, err
}
func (c *commandHandler) WorkspaceMetadata(ctx context.Context) (command.WorkspaceMetadataResult, error) {
diff --git a/internal/lsp/command/command_gen.go b/internal/lsp/command/command_gen.go
index d734345..09ebd2f 100644
--- a/internal/lsp/command/command_gen.go
+++ b/internal/lsp/command/command_gen.go
@@ -77,7 +77,7 @@
if err := UnmarshalArgs(params.Arguments, &a0); err != nil {
return nil, err
}
- return s.AddImport(ctx, a0)
+ return nil, s.AddImport(ctx, a0)
case "gopls.apply_fix":
var a0 ApplyFixArgs
if err := UnmarshalArgs(params.Arguments, &a0); err != nil {
diff --git a/internal/lsp/command/interface.go b/internal/lsp/command/interface.go
index 03040bf..2347950 100644
--- a/internal/lsp/command/interface.go
+++ b/internal/lsp/command/interface.go
@@ -115,9 +115,14 @@
// (Re)generate the gopls.mod file for a workspace.
GenerateGoplsMod(context.Context, URIArg) error
+ // ListKnownPackages: retrieves a list of packages
+ // that are importable from the given URI.
ListKnownPackages(context.Context, URIArg) (ListKnownPackagesResult, error)
- AddImport(context.Context, AddImportArgs) (AddImportResult, error)
+ // AddImport: asks the server to add an import path to a given Go file.
+ // The method will call applyEdit on the client so that clients don't have
+ // to apply the edit themselves.
+ AddImport(context.Context, AddImportArgs) error
WorkspaceMetadata(context.Context) (WorkspaceMetadataResult, error)
@@ -196,18 +201,21 @@
AddRequire bool
}
-// TODO (Marwan): document :)
-
type AddImportArgs struct {
+ // ImportPath is the target import path that should
+ // be added to the URI file
ImportPath string
- URI protocol.DocumentURI
-}
-
-type AddImportResult struct {
- Edits []protocol.TextDocumentEdit
+ // URI is the file that the ImportPath should be
+ // added to
+ URI protocol.DocumentURI
}
type ListKnownPackagesResult struct {
+ // Packages is a list of packages relative
+ // to the URIArg passed by the command request.
+ // In other words, it omits paths that are already
+ // imported or cannot be imported due to compiler
+ // restrictions.
Packages []string
}
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 7208216..a349a50 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -16,6 +16,7 @@
"testing"
"golang.org/x/tools/internal/lsp/cache"
+ "golang.org/x/tools/internal/lsp/command"
"golang.org/x/tools/internal/lsp/diff"
"golang.org/x/tools/internal/lsp/diff/myers"
"golang.org/x/tools/internal/lsp/protocol"
@@ -111,6 +112,10 @@
return nil
}
+func (c testClient) ShowMessage(context.Context, *protocol.ShowMessageParams) error {
+ return nil
+}
+
func (c testClient) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceEditParams) (*protocol.ApplyWorkspaceEditResponse, error) {
res, err := applyTextDocumentEdits(c.runner, params.Edit.DocumentChanges)
if err != nil {
@@ -1098,6 +1103,57 @@
}
}
+func (r *runner) AddImport(t *testing.T, uri span.URI, expectedImport string) {
+ cmd, err := command.NewListKnownPackagesCommand("List Known Packages", command.URIArg{
+ URI: protocol.URIFromSpanURI(uri),
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ resp, err := r.server.executeCommand(r.ctx, &protocol.ExecuteCommandParams{
+ Command: cmd.Command,
+ Arguments: cmd.Arguments,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ res := resp.(command.ListKnownPackagesResult)
+ var hasPkg bool
+ for _, p := range res.Packages {
+ if p == expectedImport {
+ hasPkg = true
+ break
+ }
+ }
+ if !hasPkg {
+ t.Fatalf("%s: got %v packages\nwant contains %q", command.ListKnownPackages, res.Packages, expectedImport)
+ }
+ cmd, err = command.NewAddImportCommand("Add Imports", command.AddImportArgs{
+ URI: protocol.URIFromSpanURI(uri),
+ ImportPath: expectedImport,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, err = r.server.executeCommand(r.ctx, &protocol.ExecuteCommandParams{
+ Command: cmd.Command,
+ Arguments: cmd.Arguments,
+ })
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := (<-r.editRecv)[uri]
+ want := r.data.Golden("addimport", uri.Filename(), func() ([]byte, error) {
+ return []byte(got), nil
+ })
+ if want == nil {
+ t.Fatalf("golden file %q not found", uri.Filename())
+ }
+ if diff := tests.Diff(t, got, string(want)); diff != "" {
+ t.Errorf("%s mismatch\n%s", command.AddImport, diff)
+ }
+}
+
func TestBytesOffset(t *testing.T) {
tests := []struct {
text string
diff --git a/internal/lsp/source/add_import.go b/internal/lsp/source/add_import.go
new file mode 100644
index 0000000..816acc2
--- /dev/null
+++ b/internal/lsp/source/add_import.go
@@ -0,0 +1,26 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+ "context"
+
+ "golang.org/x/tools/internal/imports"
+ "golang.org/x/tools/internal/lsp/protocol"
+)
+
+// AddImport adds a single import statement to the given file
+func AddImport(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle, importPath string) ([]protocol.TextEdit, error) {
+ _, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
+ if err != nil {
+ return nil, err
+ }
+ return ComputeOneImportFixEdits(snapshot, pgf, &imports.ImportFix{
+ StmtInfo: imports.ImportInfo{
+ ImportPath: importPath,
+ },
+ FixType: imports.AddImport,
+ })
+}
diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go
index ae61031..8017192 100755
--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@@ -751,9 +751,9 @@
},
{
Command: "gopls.add_import",
- Title: "",
- Doc: "",
- ArgDoc: "{\n\t\"ImportPath\": string,\n\t\"URI\": string,\n}",
+ Title: "asks the server to add an import path to a given Go file.",
+ Doc: "The method will call applyEdit on the client so that clients don't have\nto apply the edit themselves.",
+ ArgDoc: "{\n\t// ImportPath is the target import path that should\n\t// be added to the URI file\n\t\"ImportPath\": string,\n\t// URI is the file that the ImportPath should be\n\t// added to\n\t\"URI\": string,\n}",
},
{
Command: "gopls.apply_fix",
@@ -793,8 +793,8 @@
},
{
Command: "gopls.list_known_packages",
- Title: "",
- Doc: "",
+ Title: "retrieves a list of packages",
+ Doc: "that are importable from the given URI.",
ArgDoc: "{\n\t// The file URI.\n\t\"URI\": string,\n}",
},
{
diff --git a/internal/lsp/source/known_packages.go b/internal/lsp/source/known_packages.go
new file mode 100644
index 0000000..49ede16
--- /dev/null
+++ b/internal/lsp/source/known_packages.go
@@ -0,0 +1,118 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package source
+
+import (
+ "context"
+ "sort"
+ "strings"
+ "sync"
+ "time"
+
+ "golang.org/x/tools/internal/event"
+ "golang.org/x/tools/internal/imports"
+ errors "golang.org/x/xerrors"
+)
+
+// KnownPackages returns a list of all known packages
+// in the package graph that could potentially be imported
+// by the given file.
+func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle) ([]string, error) {
+ pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage)
+ if err != nil {
+ return nil, errors.Errorf("GetParsedFile: %w", err)
+ }
+ alreadyImported := map[string]struct{}{}
+ for _, imp := range pgf.File.Imports {
+ alreadyImported[imp.Path.Value] = struct{}{}
+ }
+ pkgs, err := snapshot.CachedImportPaths(ctx)
+ if err != nil {
+ return nil, err
+ }
+ var (
+ seen = make(map[string]struct{})
+ paths []string
+ )
+ for path, knownPkg := range pkgs {
+ gofiles := knownPkg.CompiledGoFiles()
+ if len(gofiles) == 0 || gofiles[0].File.Name == nil {
+ continue
+ }
+ pkgName := gofiles[0].File.Name.Name
+ // package main cannot be imported
+ if pkgName == "main" {
+ continue
+ }
+ // test packages cannot be imported
+ if knownPkg.ForTest() != "" {
+ continue
+ }
+ // no need to import what the file already imports
+ if _, ok := alreadyImported[path]; ok {
+ continue
+ }
+ // snapshot.KnownPackages could have multiple versions of a pkg
+ if _, ok := seen[path]; ok {
+ continue
+ }
+ seen[path] = struct{}{}
+ // make sure internal packages are importable by the file
+ if !IsValidImport(pkg.PkgPath(), path) {
+ continue
+ }
+ // naive check on cyclical imports
+ if isDirectlyCyclical(pkg, knownPkg) {
+ continue
+ }
+ paths = append(paths, path)
+ seen[path] = struct{}{}
+ }
+ err = snapshot.RunProcessEnvFunc(ctx, func(o *imports.Options) error {
+ var mu sync.Mutex
+ ctx, cancel := context.WithTimeout(ctx, time.Millisecond*80)
+ defer cancel()
+ return imports.GetAllCandidates(ctx, func(ifix imports.ImportFix) {
+ mu.Lock()
+ defer mu.Unlock()
+ if _, ok := seen[ifix.StmtInfo.ImportPath]; ok {
+ return
+ }
+ paths = append(paths, ifix.StmtInfo.ImportPath)
+ }, "", pgf.URI.Filename(), pkg.GetTypes().Name(), o.Env)
+ })
+ if err != nil {
+ // if an error occurred, we stil have a decent list we can
+ // show to the user through snapshot.CachedImportPaths
+ event.Error(ctx, "imports.GetAllCandidates", err)
+ }
+ sort.Slice(paths, func(i, j int) bool {
+ importI, importJ := paths[i], paths[j]
+ iHasDot := strings.Contains(importI, ".")
+ jHasDot := strings.Contains(importJ, ".")
+ if iHasDot && !jHasDot {
+ return false
+ }
+ if jHasDot && !iHasDot {
+ return true
+ }
+ return importI < importJ
+ })
+ return paths, nil
+}
+
+// isDirectlyCyclical checks if imported directly imports pkg.
+// It does not (yet) offer a full cyclical check because showing a user
+// a list of importable packages already generates a very large list
+// and having a few false positives in there could be worth the
+// performance snappiness.
+func isDirectlyCyclical(pkg, imported Package) bool {
+ for _, imp := range imported.Imports() {
+ if imp.PkgPath() == pkg.PkgPath() {
+ return true
+ }
+ }
+ return false
+}
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 23df0c5..c09b2fe 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -936,6 +936,7 @@
}
func (r *runner) FunctionExtraction(t *testing.T, start span.Span, end span.Span) {}
func (r *runner) CodeLens(t *testing.T, uri span.URI, want []protocol.CodeLens) {}
+func (r *runner) AddImport(t *testing.T, uri span.URI, expectedImport string) {}
func spanToRange(data *tests.Data, spn span.Span) (*protocol.ColumnMapper, protocol.Range, error) {
m, err := data.Mapper(spn.URI())
diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go
index 8865a55..a917a54 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -507,3 +507,24 @@
return false
}
}
+
+// IsValidImport returns whether importPkgPath is importable
+// by pkgPath
+func IsValidImport(pkgPath, importPkgPath string) bool {
+ i := strings.LastIndex(string(importPkgPath), "/internal/")
+ if i == -1 {
+ return true
+ }
+ if IsCommandLineArguments(string(pkgPath)) {
+ return true
+ }
+ return strings.HasPrefix(string(pkgPath), string(importPkgPath[:i]))
+}
+
+// IsCommandLineArguments reports whether a given value denotes
+// "command-line-arguments" package, which is a package with an unknown ID
+// created by the go command. It can have a test variant, which is why callers
+// should not check that a value equals "command-line-arguments" directly.
+func IsCommandLineArguments(s string) bool {
+ return strings.Contains(s, "command-line-arguments")
+}
diff --git a/internal/lsp/testdata/addimport/addimport.go.golden b/internal/lsp/testdata/addimport/addimport.go.golden
new file mode 100644
index 0000000..9605aa6
--- /dev/null
+++ b/internal/lsp/testdata/addimport/addimport.go.golden
@@ -0,0 +1,7 @@
+-- addimport --
+package addimport //@addimport("", "bytes")
+
+import "bytes"
+
+func main() {}
+
diff --git a/internal/lsp/testdata/addimport/addimport.go.in b/internal/lsp/testdata/addimport/addimport.go.in
new file mode 100644
index 0000000..07b454f
--- /dev/null
+++ b/internal/lsp/testdata/addimport/addimport.go.in
@@ -0,0 +1,3 @@
+package addimport //@addimport("", "bytes")
+
+func main() {}
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index a6e0a26..53861e0 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -74,6 +74,7 @@
type WorkspaceSymbols map[WorkspaceSymbolsTestType]map[span.URI][]string
type Signatures map[span.Span]*protocol.SignatureHelp
type Links map[span.URI][]Link
+type AddImport map[span.URI]string
type Data struct {
Config packages.Config
@@ -107,6 +108,7 @@
WorkspaceSymbols WorkspaceSymbols
Signatures Signatures
Links Links
+ AddImport AddImport
t testing.TB
fragments map[string]string
@@ -147,6 +149,7 @@
WorkspaceSymbols(*testing.T, span.URI, string, WorkspaceSymbolsTestType)
SignatureHelp(*testing.T, span.Span, *protocol.SignatureHelp)
Link(*testing.T, span.URI, []Link)
+ AddImport(*testing.T, span.URI, string)
}
type Definition struct {
@@ -293,6 +296,7 @@
WorkspaceSymbols: make(WorkspaceSymbols),
Signatures: make(Signatures),
Links: make(Links),
+ AddImport: make(AddImport),
t: t,
dir: dir,
@@ -447,6 +451,7 @@
"extractfunc": datum.collectFunctionExtractions,
"incomingcalls": datum.collectIncomingCalls,
"outgoingcalls": datum.collectOutgoingCalls,
+ "addimport": datum.collectAddImports,
}); err != nil {
t.Fatal(err)
}
@@ -786,6 +791,15 @@
}
})
+ t.Run("AddImport", func(t *testing.T) {
+ t.Helper()
+ for uri, exp := range data.AddImport {
+ t.Run(uriName(uri), func(t *testing.T) {
+ tests.AddImport(t, uri, exp)
+ })
+ }
+ })
+
if *UpdateGolden {
for _, golden := range data.golden {
if !golden.Modified {
@@ -1077,6 +1091,10 @@
data.Imports = append(data.Imports, spn)
}
+func (data *Data) collectAddImports(spn span.Span, imp string) {
+ data.AddImport[spn.URI()] = imp
+}
+
func (data *Data) collectSemanticTokens(spn span.Span) {
data.SemanticTokens = append(data.SemanticTokens, spn)
}
To view, visit change 281412. To unsubscribe, or for help writing mail filters, visit settings.