internal/lsp, internal/imports: use the internal goimports library
This change modifies gopls to use the internal goimports library, which
allows us to manually configure the ProcessEnv. We also add a logger to
the ProcessEnv to allow this change not to conflict with gopls's logging
mechanism.
Fixes golang/go#32585
Change-Id: Ic9aae69c7cfbc9b1f2e66aa8d812175dbc0065ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184198
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/imports/fix.go b/internal/imports/fix.go
index d9ba3b7..76a79e1 100644
--- a/internal/imports/fix.go
+++ b/internal/imports/fix.go
@@ -13,7 +13,6 @@
"go/parser"
"go/token"
"io/ioutil"
- "log"
"os"
"os/exec"
"path"
@@ -246,6 +245,12 @@
// loadPackageNames saves the package names for everything referenced by imports.
func (p *pass) loadPackageNames(imports []*importInfo) error {
+ if p.env.Debug {
+ p.env.Logf("loading package names for %v packages", len(imports))
+ defer func() {
+ p.env.Logf("done loading package names for %v packages", len(imports))
+ }()
+ }
var unknown []string
for _, imp := range imports {
if _, ok := p.knownPackages[imp.importPath]; ok {
@@ -313,7 +318,7 @@
err := p.loadPackageNames(append(imports, p.candidates...))
if err != nil {
if p.env.Debug {
- log.Printf("loading package names: %v", err)
+ p.env.Logf("loading package names: %v", err)
}
return false
}
@@ -443,7 +448,7 @@
}
srcDir := filepath.Dir(abs)
if env.Debug {
- log.Printf("fixImports(filename=%q), abs=%q, srcDir=%q ...", filename, abs, srcDir)
+ env.Logf("fixImports(filename=%q), abs=%q, srcDir=%q ...", filename, abs, srcDir)
}
// First pass: looking only at f, and using the naive algorithm to
@@ -512,6 +517,9 @@
// If true, use go/packages regardless of the environment.
ForceGoPackages bool
+ // Logf is the default logger for the ProcessEnv.
+ Logf func(format string, args ...interface{})
+
resolver resolver
}
@@ -577,7 +585,7 @@
cmd.Dir = e.WorkingDir
if e.Debug {
- defer func(start time.Time) { log.Printf("%s for %v", time.Since(start), cmdDebugStr(cmd)) }(time.Now())
+ defer func(start time.Time) { e.Logf("%s for %v", time.Since(start), cmdDebugStr(cmd)) }(time.Now())
}
if err := cmd.Run(); err != nil {
return nil, fmt.Errorf("running go: %v (stderr:\n%s)", err, stderr)
@@ -943,7 +951,7 @@
// It returns nil on error or if the package name in dir does not match expectPackage.
func loadExports(ctx context.Context, env *ProcessEnv, expectPackage string, pkg *pkg) (map[string]bool, error) {
if env.Debug {
- log.Printf("loading exports in dir %s (seeking package %s)", pkg.dir, expectPackage)
+ env.Logf("loading exports in dir %s (seeking package %s)", pkg.dir, expectPackage)
}
if pkg.goPackage != nil {
exports := map[string]bool{}
@@ -1021,7 +1029,7 @@
exportList = append(exportList, k)
}
sort.Strings(exportList)
- log.Printf("loaded exports in dir %v (package %v): %v", pkg.dir, expectPackage, strings.Join(exportList, ", "))
+ env.Logf("loaded exports in dir %v (package %v): %v", pkg.dir, expectPackage, strings.Join(exportList, ", "))
}
return exports, nil
}
@@ -1058,7 +1066,7 @@
sort.Sort(byDistanceOrImportPathShortLength(candidates))
if pass.env.Debug {
for i, c := range candidates {
- log.Printf("%s candidate %d/%d: %v in %v", pkgName, i+1, len(candidates), c.pkg.importPathShort, c.pkg.dir)
+ pass.env.Logf("%s candidate %d/%d: %v in %v", pkgName, i+1, len(candidates), c.pkg.importPathShort, c.pkg.dir)
}
}
@@ -1098,7 +1106,7 @@
exports, err := loadExports(ctx, pass.env, pkgName, c.pkg)
if err != nil {
if pass.env.Debug {
- log.Printf("loading exports in dir %s (seeking package %s): %v", c.pkg.dir, pkgName, err)
+ pass.env.Logf("loading exports in dir %s (seeking package %s): %v", c.pkg.dir, pkgName, err)
}
resc <- nil
return
diff --git a/internal/imports/imports.go b/internal/imports/imports.go
index 6253337..a47a815 100644
--- a/internal/imports/imports.go
+++ b/internal/imports/imports.go
@@ -19,6 +19,7 @@
"go/token"
"io"
"io/ioutil"
+ "log"
"regexp"
"strconv"
"strings"
@@ -50,6 +51,11 @@
src = b
}
+ // Set the logger if the user has not provided it.
+ if opt.Env.Logf == nil {
+ opt.Env.Logf = log.Printf
+ }
+
fileSet := token.NewFileSet()
file, adjust, err := parse(fileSet, filename, src, opt)
if err != nil {
diff --git a/internal/imports/mod.go b/internal/imports/mod.go
index a072214..a0ebd07 100644
--- a/internal/imports/mod.go
+++ b/internal/imports/mod.go
@@ -4,7 +4,6 @@
"bytes"
"encoding/json"
"io/ioutil"
- "log"
"os"
"path"
"path/filepath"
@@ -63,7 +62,7 @@
}
if mod.Dir == "" {
if r.env.Debug {
- log.Printf("module %v has not been downloaded and will be ignored", mod.Path)
+ r.env.Logf("module %v has not been downloaded and will be ignored", mod.Path)
}
// Can't do anything with a module that's not downloaded.
continue
@@ -254,7 +253,7 @@
modPath, err := module.DecodePath(filepath.ToSlash(matches[1]))
if err != nil {
if r.env.Debug {
- log.Printf("decoding module cache path %q: %v", subdir, err)
+ r.env.Logf("decoding module cache path %q: %v", subdir, err)
}
return
}
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 6877650..2b6661e 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -84,7 +84,7 @@
return nil, nil, ctx.Err()
}
- pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", f.filename()))
+ pkgs, err := packages.Load(v.Config(), fmt.Sprintf("file=%s", f.filename()))
if len(pkgs) == 0 {
if err == nil {
err = fmt.Errorf("go/packages.Load: no packages found for %s", f.filename())
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index a56dcbb..189a8d1 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -111,7 +111,7 @@
// Config returns the configuration used for the view's interaction with the
// go/packages API. It is shared across all views.
-func (v *view) buildConfig() *packages.Config {
+func (v *view) Config() *packages.Config {
// TODO: Should we cache the config and/or overlay somewhere?
return &packages.Config{
Dir: v.folder.Filename(),
@@ -187,7 +187,7 @@
// It assumes that the view is not active yet,
// i.e. it has not been added to the session's list of views.
func (v *view) buildBuiltinPkg() {
- cfg := *v.buildConfig()
+ cfg := *v.Config()
pkgs, _ := packages.Load(&cfg, "builtin")
if len(pkgs) != 1 {
v.builtinPkg, _ = ast.NewPackage(cfg.Fset, nil, nil, nil)
diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go
index e5195e7..e1b0643 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -100,7 +100,7 @@
if err != nil {
return nil, err
}
- edits, err := source.Imports(ctx, f, rng)
+ edits, err := source.Imports(ctx, view, f, rng)
if err != nil {
return nil, err
}
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index aa485a5..07200a3 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -10,10 +10,11 @@
"context"
"fmt"
"go/format"
+ "strings"
"golang.org/x/tools/go/ast/astutil"
"golang.org/x/tools/go/packages"
- "golang.org/x/tools/imports"
+ "golang.org/x/tools/internal/imports"
"golang.org/x/tools/internal/lsp/diff"
"golang.org/x/tools/internal/span"
)
@@ -33,12 +34,14 @@
return nil, fmt.Errorf("no exact AST node matching the specified range")
}
node := path[0]
+
+ fset := f.FileSet()
+ buf := &bytes.Buffer{}
+
// format.Node changes slightly from one release to another, so the version
// of Go used to build the LSP server will determine how it formats code.
// This should be acceptable for all users, who likely be prompted to rebuild
// the LSP server on each Go release.
- fset := f.FileSet()
- buf := &bytes.Buffer{}
if err := format.Node(buf, fset, node); err != nil {
return nil, err
}
@@ -46,7 +49,7 @@
}
// Imports formats a file using the goimports tool.
-func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) {
+func Imports(ctx context.Context, view View, f GoFile, rng span.Range) ([]TextEdit, error) {
data, _, err := f.Handle(ctx).Read(ctx)
if err != nil {
return nil, err
@@ -58,7 +61,17 @@
if hasListErrors(pkg.GetErrors()) {
return nil, fmt.Errorf("%s has list errors, not running goimports", f.URI())
}
- formatted, err := imports.Process(f.URI().Filename(), data, nil)
+ options := &imports.Options{
+ Env: buildProcessEnv(ctx, view),
+ // Defaults.
+ AllErrors: true,
+ Comments: true,
+ Fragment: true,
+ FormatOnly: false,
+ TabIndent: true,
+ TabWidth: 8,
+ }
+ formatted, err := imports.Process(f.URI().Filename(), data, options)
if err != nil {
return nil, err
}
@@ -83,6 +96,37 @@
return false
}
+func buildProcessEnv(ctx context.Context, view View) *imports.ProcessEnv {
+ cfg := view.Config()
+ env := &imports.ProcessEnv{
+ WorkingDir: cfg.Dir,
+ Logf: func(format string, v ...interface{}) {
+ view.Session().Logger().Infof(ctx, format, v...)
+ },
+ }
+ for _, kv := range cfg.Env {
+ split := strings.Split(kv, "=")
+ if len(split) < 2 {
+ continue
+ }
+ switch split[0] {
+ case "GOPATH":
+ env.GOPATH = split[1]
+ case "GOROOT":
+ env.GOROOT = split[1]
+ case "GO111MODULE":
+ env.GO111MODULE = split[1]
+ case "GOPROXY":
+ env.GOROOT = split[1]
+ case "GOFLAGS":
+ env.GOFLAGS = split[1]
+ case "GOSUMDB":
+ env.GOSUMDB = split[1]
+ }
+ }
+ return env
+}
+
func computeTextEdits(ctx context.Context, file File, formatted string) (edits []TextEdit) {
data, _, err := file.Handle(ctx).Read(ctx)
if err != nil {
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index 641b0ba..ff84348 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -343,7 +343,7 @@
if err != nil {
t.Fatalf("failed for %v: %v", spn, err)
}
- edits, err := source.Imports(ctx, f.(source.GoFile), rng)
+ edits, err := source.Imports(ctx, r.view, f.(source.GoFile), rng)
if err != nil {
if goimported != "" {
t.Error(err)
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 872e463..62a281a 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -209,6 +209,8 @@
// Ignore returns true if this file should be ignored by this view.
Ignore(span.URI) bool
+
+ Config() *packages.Config
}
// File represents a source file of any type.
diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go
index 447cfd7..6d659a5 100644
--- a/internal/lsp/tests/tests.go
+++ b/internal/lsp/tests/tests.go
@@ -25,7 +25,7 @@
// We hardcode the expected number of test cases to ensure that all tests
// are being executed. If a test is added, this number must be changed.
const (
- ExpectedCompletionsCount = 137
+ ExpectedCompletionsCount = 138
ExpectedCompletionSnippetCount = 15
ExpectedDiagnosticsCount = 17
ExpectedFormatCount = 5
