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

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."),
+		)
+	})
+}

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

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
 }

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
 			}

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

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