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 <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

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) {

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

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 {

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")
 )