From 11affa06ffda97c4827bd32c3ccc942ce3d64258 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 6 Sep 2019 18:25:36 -0400 Subject: [PATCH] internal/lsp: show errors when the user is in the wrong directory If we encounter `go list` errors when loading a user's package, we should try to see if they've encountered any of our common error cases. They are: 1) a user has GO111MODULE=off, but is outside of their GOPATH, and 2) a user is in module mode but doesn't have a go.mod file. Fortunately, go/packages does a great job handling edge cases so gopls will work well for most of them. The main issue will be unresolved imports. These show up in DepErrors in `go list`, so go/packages doesn't propagate them through to the list of errors. This will require changes to go/packages. Updates golang/go#31668 Change-Id: Ibd5253b33b38caffeaad54a403c74c0b861fcc14 Reviewed-on: https://go-review.googlesource.com/c/tools/+/194018 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cmd/definition.go | 7 +-- internal/lsp/diagnostics.go | 8 ++- internal/lsp/general.go | 7 ++- internal/lsp/lsp_test.go | 4 +- internal/lsp/source/diagnostics.go | 22 +++++--- internal/lsp/source/errors.go | 86 ++++++++++++++++++++++++++++++ internal/lsp/source/format.go | 2 + internal/lsp/source/source_test.go | 2 +- 8 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 internal/lsp/source/errors.go diff --git a/internal/lsp/cmd/definition.go b/internal/lsp/cmd/definition.go index 103abc96a6..c804aeca61 100644 --- a/internal/lsp/cmd/definition.go +++ b/internal/lsp/cmd/definition.go @@ -79,9 +79,7 @@ func (d *definition) Run(ctx context.Context, args ...string) error { Position: loc.Range.Start, } p := protocol.DefinitionParams{ - tdpp, - protocol.WorkDoneProgressParams{}, - protocol.PartialResultParams{}, + TextDocumentPositionParams: tdpp, } locs, err := conn.Definition(ctx, &p) if err != nil { @@ -92,8 +90,7 @@ func (d *definition) Run(ctx context.Context, args ...string) error { return errors.Errorf("%v: not an identifier", from) } q := protocol.HoverParams{ - tdpp, - protocol.WorkDoneProgressParams{}, + TextDocumentPositionParams: tdpp, } hover, err := conn.Hover(ctx, &q) if err != nil { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index a7933077ea..b625439953 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -33,10 +33,16 @@ func (s *Server) diagnostics(view source.View, uri span.URI) error { if !ok { return errors.Errorf("%s is not a Go file", f.URI()) } - reports, err := source.Diagnostics(ctx, view, gof, view.Options().DisabledAnalyses) + reports, warningMsg, err := source.Diagnostics(ctx, view, gof, view.Options().DisabledAnalyses) if err != nil { return err } + if warningMsg != "" { + s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Info, + Message: warningMsg, + }) + } s.undeliveredMu.Lock() defer s.undeliveredMu.Unlock() diff --git a/internal/lsp/general.go b/internal/lsp/general.go index cb485c5aae..76d3b24698 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -46,10 +46,9 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitia) ( Name: path.Base(params.RootURI), }} } else { - // no folders and no root, single file mode - //TODO(iancottrell): not sure how to do single file mode yet - //issue: golang.org/issue/31168 - return nil, errors.Errorf("single file mode not supported yet") + // No folders and no root--we are in single file mode. + // TODO: https://golang.org/issue/34160. + return nil, errors.Errorf("gopls does not yet support editing a single file. Please open a directory.") } } diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index bba1bc24b7..63c850f190 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -94,7 +94,7 @@ func (r *runner) Diagnostics(t *testing.T, data tests.Diagnostics) { if !ok { t.Fatalf("%s is not a Go file: %v", uri, err) } - results, err := source.Diagnostics(r.ctx, v, gof, nil) + results, _, err := source.Diagnostics(r.ctx, v, gof, nil) if err != nil { t.Fatal(err) } @@ -558,7 +558,7 @@ func (r *runner) SuggestedFix(t *testing.T, data tests.SuggestedFixes) { if err != nil { t.Fatal(err) } - results, err := source.Diagnostics(r.ctx, v, f, nil) + results, _, err := source.Diagnostics(r.ctx, v, f, nil) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 5e57c565af..7a275f2953 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -64,26 +64,36 @@ const ( SeverityError ) -func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[string]struct{}) (map[span.URI][]Diagnostic, error) { +func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[string]struct{}) (map[span.URI][]Diagnostic, string, error) { ctx, done := trace.StartSpan(ctx, "source.Diagnostics", telemetry.File.Of(f.URI())) defer done() cphs, err := f.CheckPackageHandles(ctx) if err != nil { - return nil, err + return nil, "", err } cph := NarrowestCheckPackageHandle(cphs) pkg, err := cph.Check(ctx) if err != nil { log.Error(ctx, "no package for file", err) - return singleDiagnostic(f.URI(), "%s is not part of a package", f.URI()), nil + return singleDiagnostic(f.URI(), "%s is not part of a package", f.URI()), "", nil } + // Prepare the reports we will send for the files in this package. reports := make(map[span.URI][]Diagnostic) for _, fh := range pkg.Files() { clearReports(view, reports, fh.File().Identity().URI) } + // If we have `go list` errors, we may want to offer a warning message to the user. + var warningMsg string + if hasListErrors(pkg.GetErrors()) { + warningMsg, err = checkCommonErrors(ctx, view, f.URI()) + if err != nil { + log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI)) + } + } + // Prepare any additional reports for the errors in this package. for _, err := range pkg.GetErrors() { if err.Kind != packages.ListError { @@ -104,19 +114,19 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ for _, f := range revDeps { cphs, err := f.CheckPackageHandles(ctx) if err != nil { - return nil, err + return nil, "", err } cph := WidestCheckPackageHandle(cphs) pkg, err := cph.Check(ctx) if err != nil { - return nil, err + return nil, warningMsg, err } for _, fh := range pkg.Files() { clearReports(view, reports, fh.File().Identity().URI) } diagnostics(ctx, view, pkg, reports) } - return reports, nil + return reports, warningMsg, nil } type diagnosticSet struct { diff --git a/internal/lsp/source/errors.go b/internal/lsp/source/errors.go new file mode 100644 index 0000000000..9b740fe645 --- /dev/null +++ b/internal/lsp/source/errors.go @@ -0,0 +1,86 @@ +package source + +import ( + "bytes" + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + + "golang.org/x/tools/internal/span" +) + +func checkCommonErrors(ctx context.Context, view View, uri span.URI) (string, error) { + // Some cases we should be able to detect: + // + // 1. The user is in GOPATH mode and is working outside their GOPATH + // 2. The user is in module mode and has opened a subdirectory of their module + // + gopath := os.Getenv("GOPATH") + cfg := view.Config(ctx) + + // Invoke `go env GOMOD` inside of the directory of the file. + fdir := filepath.Dir(uri.Filename()) + b, err := invokeGo(ctx, fdir, cfg.Env, "env", "GOMOD") + if err != nil { + return "", err + } + modFile := strings.TrimSpace(b.String()) + if modFile == filepath.FromSlash("/dev/null") { + modFile = "" + } + + // Not inside of a module. + inAModule := modFile != "" + inGopath := strings.HasPrefix(uri.Filename(), filepath.Join(gopath, "src")) + moduleMode := os.Getenv("GO111MODULE") + + var msg string + // The user is in a module. + if inAModule { + // The workspace root is open to a directory different from the module root. + if modRoot := filepath.Dir(modFile); cfg.Dir != filepath.Dir(modFile) { + msg = fmt.Sprintf("Your workspace root is %s, but your module root is %s. Please add %s as a workspace folder.", cfg.Dir, modRoot, modRoot) + } + } else if inGopath { + if moduleMode == "on" { + msg = "You are in module mode, but you are not inside of a module. Please create a module." + } + } else { + msg = "You are neither in a module nor in your GOPATH. Please see X for information on how to set up your Go project." + } + return msg, nil +} + +// invokeGo returns the stdout of a go command invocation. +// Borrowed from golang.org/x/tools/go/packages/golist.go. +func invokeGo(ctx context.Context, dir string, env []string, args ...string) (*bytes.Buffer, error) { + stdout := new(bytes.Buffer) + stderr := new(bytes.Buffer) + cmd := exec.CommandContext(ctx, "go", args...) + // On darwin the cwd gets resolved to the real path, which breaks anything that + // expects the working directory to keep the original path, including the + // go command when dealing with modules. + // The Go stdlib has a special feature where if the cwd and the PWD are the + // same node then it trusts the PWD, so by setting it in the env for the child + // process we fix up all the paths returned by the go command. + cmd.Env = append(append([]string{}, env...), "PWD="+dir) + cmd.Dir = dir + cmd.Stdout = stdout + cmd.Stderr = stderr + + if err := cmd.Run(); err != nil { + // Check for 'go' executable not being found. + if ee, ok := err.(*exec.Error); ok && ee.Err == exec.ErrNotFound { + return nil, fmt.Errorf("'gopls requires 'go', but %s", exec.ErrNotFound) + } + if _, ok := err.(*exec.ExitError); !ok { + // Catastrophic error: + // - context cancellation + return nil, fmt.Errorf("couldn't exec 'go %v': %s %T", args, err, err) + } + } + return stdout, nil +} diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index cf8098c23f..ba2ce3d159 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -9,6 +9,7 @@ import ( "bytes" "context" "go/format" + "log" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/imports" @@ -268,6 +269,7 @@ func hasParseErrors(pkg Package, uri span.URI) bool { func hasListErrors(errors []packages.Error) bool { for _, err := range errors { if err.Kind == packages.ListError { + log.Printf("LIST ERROR: %v", err) return true } } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 7411829218..9e1398fad0 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -68,7 +68,7 @@ func (r *runner) Diagnostics(t *testing.T, data tests.Diagnostics) { if err != nil { t.Fatal(err) } - results, err := source.Diagnostics(r.ctx, r.view, f.(source.GoFile), nil) + results, _, err := source.Diagnostics(r.ctx, r.view, f.(source.GoFile), nil) if err != nil { t.Fatal(err) }