From 2ab23861a01c7c874e7df3a5cd0629cdfe5a403e Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Thu, 28 Jan 2021 17:59:26 -0500 Subject: [PATCH] internal/lsp: stop using structured errors Using structured errors in gopls has proven to be difficult to manage: it's hard to know whether a given error return is expected to be structured without any type information. We have mostly eliminated them; finish the job. I don't intend any semantic changes here. I considered eliminating CriticalError altogether, but it does seem useful to have a convenient bundle for return values. So I left it alone for now. Change-Id: I4b5f85a8de9712babffbc1383088151b596bd815 Reviewed-on: https://go-review.googlesource.com/c/tools/+/287792 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/snapshot.go | 9 ++++++--- internal/lsp/cache/view.go | 6 ++++-- internal/lsp/diagnostics.go | 10 +++------- internal/lsp/source/view.go | 16 ++-------------- 4 files changed, 15 insertions(+), 26 deletions(-) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 7bea58e2a3..6a3995a9b7 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -60,7 +60,7 @@ type snapshot struct { // initializedErr holds the last error resulting from initialization. If // initialization fails, we only retry when the the workspace modules change, // to avoid too many go/packages calls. - initializedErr error + initializedErr *source.CriticalError // mu guards all of the maps in the snapshot. mu sync.Mutex @@ -1012,7 +1012,7 @@ func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError { wsPkgs, _ := s.WorkspacePackages(ctx) if msg := shouldShowAdHocPackagesWarning(s, wsPkgs); msg != "" { return &source.CriticalError{ - MainError: fmt.Errorf(msg), + MainError: errors.New(msg), } } // Even if workspace packages were returned, there still may be an error @@ -1084,7 +1084,10 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) error { // TODO(rstambler): Should we be more careful about returning the // initialization error? Is it possible for the initialization error to be // corrected without a successful reinitialization? - return s.initializedErr + if s.initializedErr == nil { + return nil + } + return s.initializedErr.MainError } func (s *snapshot) AwaitInitialized(ctx context.Context) { diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 5b767d1157..55c52eb44a 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -9,7 +9,6 @@ import ( "context" "encoding/json" "fmt" - exec "golang.org/x/sys/execabs" "io" "io/ioutil" "os" @@ -23,6 +22,7 @@ import ( "golang.org/x/mod/modfile" "golang.org/x/mod/semver" + exec "golang.org/x/sys/execabs" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" @@ -581,7 +581,9 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { ErrorList: modErrors, } } else { - s.initializedErr = err + s.initializedErr = &source.CriticalError{ + MainError: err, + } } } else { // Clear out the initialization error, in case it had been set diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index adf19ad86d..ceee5f0d9e 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -350,13 +350,9 @@ func (s *Server) showCriticalErrorStatus(ctx context.Context, snapshot source.Sn // status bar. var errMsg string if err != nil { - event.Error(ctx, "errors loading workspace", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) - - // Some error messages can also be displayed as diagnostics. - if criticalErr := (*source.CriticalError)(nil); errors.As(err, &criticalErr) { - s.storeErrorDiagnostics(ctx, snapshot, modSource, criticalErr.ErrorList) - } - errMsg = strings.Replace(err.Error(), "\n", " ", -1) + event.Error(ctx, "errors loading workspace", err.MainError, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) + s.storeErrorDiagnostics(ctx, snapshot, modSource, err.ErrorList) + errMsg = strings.Replace(err.MainError.Error(), "\n", " ", -1) } if s.criticalErrorStatus == nil { diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 7a576a6dac..2e0b8517dd 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -556,17 +556,12 @@ type Package interface { } type CriticalError struct { + // MainError is the primary error. Must be non-nil. MainError error + // ErrorList contains any supplemental (structured) errors. ErrorList []*Error } -func (err *CriticalError) Error() string { - if err.MainError == nil { - return "" - } - return err.MainError.Error() -} - // An Error corresponds to an LSP Diagnostic. // https://microsoft.github.io/language-server-protocol/specification#diagnostic type Error struct { @@ -601,13 +596,6 @@ const ( UpgradeNotification ) -func (e *Error) Error() string { - if e.URI == "" { - return e.Message - } - return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Message) -} - var ( PackagesLoadError = errors.New("packages.Load error") )