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>
diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go
index 74687b8..c68aa8f 100644
--- a/gopls/internal/regtest/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics_test.go
@@ -1594,3 +1594,36 @@
 		})
 	})
 }
+
+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 1abb844..6a579fa 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -19,6 +19,7 @@
 	"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 @@
 	}
 	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 @@
 	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 @@
 		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 0697669..d6b598b 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -923,7 +923,11 @@
 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 11d685b..a603cb3 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -517,6 +517,9 @@
 		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 @@
 		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 16af4b6..d3d8474 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -176,17 +176,14 @@
 	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 @@
 
 // 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 @@
 	// 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 ddf8f6b..04021d1 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -536,15 +536,26 @@
 	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 @@
 )
 
 func (e *Error) Error() string {
+	if e.URI == "" {
+		return e.Message
+	}
 	return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Message)
 }