From 7beb506c84129efb2dcf019daa47a431125d625c Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sun, 22 Nov 2020 22:23:48 -0500 Subject: [PATCH] internal/lsp: improve errors in multi-module workspaces (GO111MODULE=on) Currently, when a user opens a workspace with no top-level module but multiple modules in subdirectories, gopls treats that as an invalid build configuration and reports an error message that may be difficult for the user to understand (a go list error message about creating a main module in the top-level directory). Instead, show a more useful error message about the gopls workspace layout in both the progress bar and as a diagnostic on every open file. This fix only works for GO111MODULE=on (for now) because it's a lot easier to interpret user intent, and the go command will return no packages. The next step will be to improve error messaging for GO111MODULE=auto and for users with nested modules. Updates golang/go#42109 Change-Id: I702ca6745f7e080ff6704ade7843972ab469ccf3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/272346 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley --- gopls/internal/regtest/diagnostics_test.go | 33 +++++++ internal/lsp/cache/load.go | 100 ++++++++++++++++++--- internal/lsp/cache/snapshot.go | 4 + internal/lsp/cache/view.go | 8 +- internal/lsp/diagnostics.go | 18 ++-- internal/lsp/source/view.go | 22 ++++- 6 files changed, 164 insertions(+), 21 deletions(-) diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index 74687b81fc..c68aa8fc02 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -1594,3 +1594,36 @@ import ( }) }) } + +func TestMultipleModules_GO111MODULE_on(t *testing.T) { + const modules = ` +-- a/go.mod -- +module a.com + +go 1.12 +-- a/a.go -- +package a +-- b/go.mod -- +module b.com + +go 1.12 +-- b/b.go -- +package b +` + withOptions( + WithModes(WithoutExperiments), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": "on", + }, + }, + ).run(t, modules, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + env.OpenFile("b/go.mod") + env.Await( + env.DiagnosticAtRegexp("a/a.go", "package a"), + env.DiagnosticAtRegexp("b/go.mod", "module b.com"), + OutstandingWork("Error loading workspace", "gopls requires a module at the root of your workspace."), + ) + }) +} diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 1abb844b8f..6a579fa04b 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -19,6 +19,7 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/lsp/debug/tag" + "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/memoize" "golang.org/x/tools/internal/packagesinternal" @@ -123,9 +124,10 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { } if len(pkgs) == 0 { if err != nil { - // Try to extract the error into a diagnostic. - if srcErrs := s.parseLoadError(ctx, err); srcErrs != nil { - return srcErrs + // Try to extract the load error into a structured error with + // diagnostics. + if criticalErr := s.parseLoadError(ctx, err); criticalErr != nil { + return criticalErr } } else { err = fmt.Errorf("no packages returned") @@ -167,8 +169,16 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { return nil } -func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) source.ErrorList { - var srcErrs source.ErrorList +func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) *source.CriticalError { + // The error may be a result of the user's workspace layout. Check for + // a valid workspace configuration first. + if criticalErr := s.workspaceLayoutErrors(ctx, loadErr); criticalErr != nil { + return criticalErr + } + criticalErr := &source.CriticalError{ + MainError: loadErr, + } + // Attempt to place diagnostics in the relevant go.mod files, if any. for _, uri := range s.ModFiles() { fh, err := s.GetFile(ctx, uri) if err != nil { @@ -178,12 +188,82 @@ func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) source.Err if srcErr == nil { continue } - if srcErrs == nil { - srcErrs = source.ErrorList{} - } - srcErrs = append(srcErrs, srcErr) + criticalErr.ErrorList = append(criticalErr.ErrorList, srcErr) } - return srcErrs + return criticalErr +} + +// workspaceLayoutErrors returns a diagnostic for every open file, as well as +// an error message if there are no open files. +func (s *snapshot) workspaceLayoutErrors(ctx context.Context, err error) *source.CriticalError { + // Assume the workspace is misconfigured only if we've detected an invalid + // build configuration. Currently, a valid build configuration is either a + // module at the root of the view or a GOPATH workspace. + if s.ValidBuildConfiguration() { + return nil + } + if len(s.workspace.getKnownModFiles()) == 0 { + return nil + } + // TODO(rstambler): Handle GO111MODULE=auto. + if s.view.go111module != "on" { + return nil + } + if s.workspace.moduleSource != legacyWorkspace { + return nil + } + // The user's workspace contains go.mod files and they have + // GO111MODULE=on, so we should guide them to create a + // workspace folder for each module. + + // Add a diagnostic to every open file, or return a general error if + // there aren't any. + var open []source.VersionedFileHandle + s.mu.Lock() + for _, fh := range s.files { + if s.isOpenLocked(fh.URI()) { + open = append(open, fh) + } + } + s.mu.Unlock() + + msg := `gopls requires a module at the root of your workspace. +You can work with multiple modules by opening each one as a workspace folder. +Improvements to this workflow will be coming soon (https://github.com/golang/go/issues/32394), +and you can learn more here: https://github.com/golang/go/issues/36899.` + + criticalError := &source.CriticalError{ + MainError: errors.New(msg), + } + if len(open) == 0 { + return criticalError + } + for _, fh := range open { + // Place the diagnostics on the package or module declarations. + var rng protocol.Range + switch fh.Kind() { + case source.Go: + if pgf, err := s.ParseGo(ctx, fh, source.ParseHeader); err == nil { + pkgDecl := span.NewRange(s.FileSet(), pgf.File.Package, pgf.File.Name.End()) + if spn, err := pkgDecl.Span(); err == nil { + rng, _ = pgf.Mapper.Range(spn) + } + } + case source.Mod: + if pmf, err := s.ParseMod(ctx, fh); err == nil { + if pmf.File.Module != nil && pmf.File.Module.Syntax != nil { + rng, _ = rangeFromPositions(pmf.Mapper, pmf.File.Module.Syntax.Start, pmf.File.Module.Syntax.End) + } + } + } + criticalError.ErrorList = append(criticalError.ErrorList, &source.Error{ + URI: fh.URI(), + Range: rng, + Kind: source.ListError, + Message: msg, + }) + } + return criticalError } type workspaceDirKey string diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 0697669810..d6b598bc62 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -923,7 +923,11 @@ func (s *snapshot) getFileLocked(ctx context.Context, f *fileBase) (source.Versi func (s *snapshot) IsOpen(uri span.URI) bool { s.mu.Lock() defer s.mu.Unlock() + return s.isOpenLocked(uri) +} + +func (s *snapshot) isOpenLocked(uri span.URI) bool { _, open := s.files[uri].(*overlay) return open } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 11d685be33..a603cb3ea1 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -517,6 +517,9 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { var scopes []interface{} var modErrors source.ErrorList addError := func(uri span.URI, err error) { + if modErrors == nil { + modErrors = source.ErrorList{} + } modErrors = append(modErrors, &source.Error{ URI: uri, Category: "compiler", @@ -552,7 +555,10 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { if err != nil { event.Error(ctx, "initial workspace load failed", err) if modErrors != nil { - s.initializedErr = errors.Errorf("errors loading modules: %v: %w", err, modErrors) + s.initializedErr = &source.CriticalError{ + MainError: errors.Errorf("errors loading modules: %v: %w", err, modErrors), + ErrorList: modErrors, + } } else { s.initializedErr = err } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 16af4b63f5..d3d8474470 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -176,17 +176,14 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA if s.shouldIgnoreError(ctx, snapshot, err) { return false } + // Show the error as a progress error report so that it appears in the // status bar. If a client doesn't support progress reports, the error // will still be shown as a ShowMessage. If there is no error, any running // error progress reports will be closed. - s.showCriticalErrorStatus(ctx, err) + s.showCriticalErrorStatus(ctx, snapshot, err) if err != nil { - // Some error messages can also be displayed as diagnostics. - if errList := (source.ErrorList)(nil); errors.As(err, &errList) { - s.storeErrorDiagnostics(ctx, snapshot, typeCheckSource, errList) - } event.Error(ctx, "errors diagnosing workspace", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) return false } @@ -361,7 +358,7 @@ func hasUndeclaredErrors(pkg source.Package) bool { // showCriticalErrorStatus shows the error as a progress report. // If the error is nil, it clears any existing error progress report. -func (s *Server) showCriticalErrorStatus(ctx context.Context, err error) { +func (s *Server) showCriticalErrorStatus(ctx context.Context, snapshot source.Snapshot, err error) { s.criticalErrorStatusMu.Lock() defer s.criticalErrorStatusMu.Unlock() @@ -369,6 +366,15 @@ func (s *Server) showCriticalErrorStatus(ctx context.Context, err error) { // status bar. var errMsg string if err != nil { + // Some error messages can also be displayed as diagnostics. But don't + // show source.ErrorLists as critical errors--only CriticalErrors + // should be shown. + if criticalErr := (*source.CriticalError)(nil); errors.As(err, &criticalErr) { + s.storeErrorDiagnostics(ctx, snapshot, typeCheckSource, criticalErr.ErrorList) + } else if srcErrList := (source.ErrorList)(nil); errors.As(err, &srcErrList) { + s.storeErrorDiagnostics(ctx, snapshot, typeCheckSource, srcErrList) + return + } errMsg = strings.Replace(err.Error(), "\n", " ", -1) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index ddf8f6b477..04021d1c1f 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -536,15 +536,26 @@ type Package interface { Version() *module.Version } +type CriticalError struct { + MainError error + ErrorList +} + +func (err *CriticalError) Error() string { + if err.MainError == nil { + return "" + } + return err.MainError.Error() +} + type ErrorList []*Error func (err ErrorList) Error() string { - var b strings.Builder - b.WriteString("source error list:") + var list []string for _, e := range err { - b.WriteString(fmt.Sprintf("\n\t%s", e)) + list = append(list, e.Error()) } - return b.String() + return strings.Join(list, "\n\t") } // An Error corresponds to an LSP Diagnostic. @@ -577,6 +588,9 @@ const ( ) func (e *Error) Error() string { + if e.URI == "" { + return e.Message + } return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Message) }