From 38ae2c8f64122bd595b7f93f968a6686cd27bb5a Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 28 Jun 2019 16:21:07 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell Reviewed-by: Heschi Kreinick --- internal/imports/fix.go | 24 ++++++++----- internal/imports/imports.go | 6 ++++ internal/imports/mod.go | 5 ++- internal/lsp/cache/load.go | 2 +- internal/lsp/cache/view.go | 4 +-- internal/lsp/code_action.go | 2 +- internal/lsp/source/format.go | 54 +++++++++++++++++++++++++++--- internal/lsp/source/source_test.go | 2 +- internal/lsp/source/view.go | 2 ++ internal/lsp/tests/tests.go | 2 +- 10 files changed, 81 insertions(+), 22 deletions(-) diff --git a/internal/imports/fix.go b/internal/imports/fix.go index d9ba3b786dd..76a79e16008 100644 --- a/internal/imports/fix.go +++ b/internal/imports/fix.go @@ -13,7 +13,6 @@ import ( "go/parser" "go/token" "io/ioutil" - "log" "os" "os/exec" "path" @@ -246,6 +245,12 @@ type pass struct { // 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 @@ func (p *pass) load() bool { 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 @@ func fixImportsDefault(fset *token.FileSet, f *ast.File, filename string, env *P } 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 @@ type ProcessEnv struct { // 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 @@ func (e *ProcessEnv) invokeGo(args ...string) (*bytes.Buffer, error) { 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 @@ func VendorlessPath(ipath string) string { // 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 @@ func loadExports(ctx context.Context, env *ProcessEnv, expectPackage string, pkg 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 @@ func findImport(ctx context.Context, pass *pass, dirScan []*pkg, pkgName string, 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 @@ func findImport(ctx context.Context, pass *pass, dirScan []*pkg, pkgName string, 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 62533376042..a47a815f580 100644 --- a/internal/imports/imports.go +++ b/internal/imports/imports.go @@ -19,6 +19,7 @@ import ( "go/token" "io" "io/ioutil" + "log" "regexp" "strconv" "strings" @@ -50,6 +51,11 @@ func Process(filename string, src []byte, opt *Options) ([]byte, error) { 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 a072214eee9..a0ebd0737cc 100644 --- a/internal/imports/mod.go +++ b/internal/imports/mod.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/json" "io/ioutil" - "log" "os" "path" "path/filepath" @@ -63,7 +62,7 @@ func (r *moduleResolver) init() error { } 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 @@ func (r *moduleResolver) scan(_ references) ([]*pkg, error) { 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 6877650178a..2b6661e5de0 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -84,7 +84,7 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*met 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 a56dcbb826a..189a8d18236 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -111,7 +111,7 @@ func (v *view) Folder() span.URI { // 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 @@ func (v *view) BuiltinPackage() *ast.Package { // 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 e5195e7b484..e1b06434387 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -100,7 +100,7 @@ func organizeImports(ctx context.Context, view source.View, s span.Span) ([]prot 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 aa485a53107..07200a3c08d 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -10,10 +10,11 @@ import ( "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 @@ func Format(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) { 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 @@ func Format(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) { } // 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 @@ func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) 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 @@ func hasListErrors(errors []packages.Error) bool { 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 641b0ba08b1..ff843489f07 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -343,7 +343,7 @@ func (r *runner) Import(t *testing.T, data tests.Imports) { 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 872e463cb89..62a281a165b 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -209,6 +209,8 @@ type View interface { // 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 447cfd7082f..6d659a5cc65 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -25,7 +25,7 @@ import ( // 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