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 <[email protected]>
Trust: Robert Findley <[email protected]>
Run-TryBot: Rebecca Stambler <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

Author: marwan-at-work

gopls/doc/commands.md

@@ -21,16 +21,21 @@ Args:
 }
 ```
 
-### ****
+### **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 @@ Args:
 }
 ```
 
-### ****
+### **retrieves a list of packages**
 Identifier: `gopls.list_known_packages`
 
-
+that are importable from the given URI.
 
 Args:
 

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

internal/lsp/cache/check.go

@@ -503,7 +503,7 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode sour
 			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 @@ func resolveImportPath(importPath string, pkg *pkg, deps map[packagePath]*packag
 	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)

internal/lsp/cache/load.go

@@ -55,7 +55,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	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

internal/lsp/cache/snapshot.go

@@ -995,7 +995,7 @@ func (s *snapshot) addID(uri span.URI, id packageID) {
 		}
 		// 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 @@ func (s *snapshot) addID(uri span.URI, id packageID) {
 	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 shouldShowAdHocPackagesWarning(snapshot source.Snapshot, pkgs []source.Pack
 
 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 @@ func (s *snapshot) reloadWorkspace(ctx context.Context) error {
 		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 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		// 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
 			}

internal/lsp/cmd/test/cmdtest.go

@@ -100,6 +100,10 @@ func (r *runner) FunctionExtraction(t *testing.T, start span.Span, end span.Span
 	//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 {

internal/lsp/command.go

@@ -54,7 +54,7 @@ type commandHandler struct {
 
 // 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 @@ func (c *commandHandler) run(ctx context.Context, cfg commandConfig, run command
 		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 @@ func (c *commandHandler) RunTests(ctx context.Context, args command.RunTestsArgs
 		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) GenerateGoplsMod(ctx context.Context, args command.URIA
 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) {

internal/lsp/command/command_gen.go

@@ -115,9 +115,14 @@ type Interface interface {
 	// (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 @@ type GoGetPackageArgs struct {
 	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
 }