From 44a64ad78b9b521790ab78240c17a3bc75b5eaa7 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 2 Apr 2020 17:27:48 -0400 Subject: [PATCH] internal/lsp, go/packages: don't log context cancellation errors Instead of checking the context, check the error. This may expose some errors that are not wrapped correctly. Replaced all uses of errors with golang.org/x/xerrors. Change-Id: Ia40160f8ea352e02618765f2a9415a4ece0dcd94 Reviewed-on: https://go-review.googlesource.com/c/tools/+/227036 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Robert Findley --- go/packages/golist.go | 3 ++- go/packages/loadmode_string.go | 2 +- internal/lsp/debug/serve.go | 13 +++++++++++-- internal/lsp/generate.go | 17 +++++++++-------- internal/lsp/protocol/context.go | 3 --- internal/lsp/source/references.go | 4 ++-- 6 files changed, 25 insertions(+), 17 deletions(-) diff --git a/go/packages/golist.go b/go/packages/golist.go index f37824266f..099207d78c 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -25,6 +25,7 @@ import ( "golang.org/x/tools/go/internal/packagesdriver" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" + "golang.org/x/xerrors" ) // debug controls verbose logging. @@ -736,7 +737,7 @@ func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, if !ok { // Catastrophic error: // - context cancellation - return nil, fmt.Errorf("couldn't run 'go': %v", err) + return nil, xerrors.Errorf("couldn't run 'go': %w", err) } // Old go version? diff --git a/go/packages/loadmode_string.go b/go/packages/loadmode_string.go index aff94a3fe9..7ea37e7eea 100644 --- a/go/packages/loadmode_string.go +++ b/go/packages/loadmode_string.go @@ -38,7 +38,7 @@ var modeStrings = []string{ func (mod LoadMode) String() string { m := mod if m == 0 { - return fmt.Sprintf("LoadMode(0)") + return "LoadMode(0)" } var out []string for i, x := range allModes { diff --git a/internal/lsp/debug/serve.go b/internal/lsp/debug/serve.go index 2004716830..11364b6e0e 100644 --- a/internal/lsp/debug/serve.go +++ b/internal/lsp/debug/serve.go @@ -34,6 +34,7 @@ import ( "golang.org/x/tools/internal/telemetry/export/metric" "golang.org/x/tools/internal/telemetry/export/ocagent" "golang.org/x/tools/internal/telemetry/export/prometheus" + "golang.org/x/xerrors" ) type instanceKeyType int @@ -543,8 +544,16 @@ func (i *Instance) writeMemoryDebug(threshold uint64) error { func makeGlobalExporter(stderr io.Writer) event.Exporter { return func(ctx context.Context, ev event.Event, tags event.TagMap) context.Context { i := GetInstance(ctx) - if ev.IsLog() && (event.Err.Get(ev.Map()) != nil || i == nil) { - fmt.Fprintf(stderr, "%v\n", ev) + + if ev.IsLog() { + // Don't log context cancellation errors. + if err := event.Err.Get(ev.Map()); xerrors.Is(err, context.Canceled) { + return ctx + } + // Make sure any log messages without an instance go to stderr. + if i == nil { + fmt.Fprintf(stderr, "%v\n", ev) + } } ctx = protocol.LogEvent(ctx, ev, tags) if i == nil { diff --git a/internal/lsp/generate.go b/internal/lsp/generate.go index 08840aa986..2bcd037c3a 100644 --- a/internal/lsp/generate.go +++ b/internal/lsp/generate.go @@ -7,7 +7,6 @@ package lsp import ( "context" "io" - "log" "math/rand" "strconv" @@ -15,7 +14,7 @@ import ( "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/telemetry/event" - errors "golang.org/x/xerrors" + "golang.org/x/xerrors" ) func (s *Server) runGenerate(ctx context.Context, dir string, recursive bool) { @@ -43,12 +42,14 @@ func (s *Server) runGenerate(ctx context.Context, dir string, recursive bool) { } stderr := io.MultiWriter(er, wc) err := inv.RunPiped(ctx, er, stderr) - if err != nil && !errors.Is(ctx.Err(), context.Canceled) { - log.Printf("generate: command error: %v", err) - s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ - Type: protocol.Error, - Message: "go generate exited with an error, check gopls logs", - }) + if err != nil { + event.Error(ctx, "generate: command error: %v", err, tag.Directory.Of(dir)) + if !xerrors.Is(err, context.Canceled) { + s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: "go generate exited with an error, check gopls logs", + }) + } } } diff --git a/internal/lsp/protocol/context.go b/internal/lsp/protocol/context.go index dac850d763..560de4dbd0 100644 --- a/internal/lsp/protocol/context.go +++ b/internal/lsp/protocol/context.go @@ -19,9 +19,6 @@ func WithClient(ctx context.Context, client Client) context.Context { } func LogEvent(ctx context.Context, ev event.Event, tags event.TagMap) context.Context { - if ctx.Err() != nil { - return ctx - } if !ev.IsLog() { return ctx } diff --git a/internal/lsp/source/references.go b/internal/lsp/source/references.go index d1cc60261a..a6d35e8f8e 100644 --- a/internal/lsp/source/references.go +++ b/internal/lsp/source/references.go @@ -6,13 +6,13 @@ package source import ( "context" - "errors" "go/ast" "go/token" "go/types" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/telemetry/event" + "golang.org/x/xerrors" ) // ReferenceInfo holds information about reference to an identifier in Go source. @@ -33,7 +33,7 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit qualifiedObjs, err := qualifiedObjsAtProtocolPos(ctx, s, f, pp) // Don't return references for builtin types. - if errors.Is(err, errBuiltin) { + if xerrors.Is(err, errBuiltin) { return nil, nil } if err != nil {