internal/lsp/cache: extract module load errors when go.work is used

When using go.work, the go command packs module-specific errors into
synthetic results corresponding to the module query ("modulepath/...").

Extract these for use in diagnostics, by packing them into a new
moduleErrorMap error type.

Fixes golang/go#50862

Change-Id: I68bf9cf4e9ebf4595acdc4da0347ed97171d637f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405259
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>

Author: findleyr

gopls/internal/regtest/workspace/workspace_test.go

@@ -705,7 +705,7 @@ use (
 	WithOptions(
 		ProxyFiles(workspaceModuleProxy),
 	).Run(t, multiModule, func(t *testing.T, env *Env) {
-		// Initially, the gopls.mod should cause only the a.com module to be
+		// Initially, the go.work should cause only the a.com module to be
 		// loaded. Validate this by jumping to a definition in b.com and ensuring
 		// that we go to the module cache.
 		env.OpenFile("moda/a/a.go")
@@ -726,7 +726,7 @@ use (
 			t.Fatal(err)
 		}
 
-		// Now, modify the gopls.mod file on disk to activate the b.com module in
+		// Now, modify the go.work file on disk to activate the b.com module in
 		// the workspace.
 		env.WriteWorkspaceFile("go.work", `
 go 1.17
@@ -742,26 +742,22 @@ use (
 		env.OpenFile("modb/go.mod")
 		env.Await(env.DoneWithOpen())
 
-		//  TODO(golang/go#50862): the go command drops error messages when using
-		//  go.work, so we need to build our go.mod diagnostics in a different way.
-		if testenv.Go1Point() < 18 {
-			var d protocol.PublishDiagnosticsParams
-			env.Await(
-				OnceMet(
-					env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"),
-					ReadDiagnostics("modb/go.mod", &d),
-				),
-			)
-			env.ApplyQuickFixes("modb/go.mod", d.Diagnostics)
-			env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x"))
-		}
+		var d protocol.PublishDiagnosticsParams
+		env.Await(
+			OnceMet(
+				env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"),
+				ReadDiagnostics("modb/go.mod", &d),
+			),
+		)
+		env.ApplyQuickFixes("modb/go.mod", d.Diagnostics)
+		env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x"))
 
 		// Jumping to definition should now go to b.com in the workspace.
 		if err := checkHelloLocation("modb/b/b.go"); err != nil {
 			t.Fatal(err)
 		}
 
-		// Now, let's modify the gopls.mod *overlay* (not on disk), and verify that
+		// Now, let's modify the go.work *overlay* (not on disk), and verify that
 		// this change is only picked up once it is saved.
 		env.OpenFile("go.work")
 		env.Await(env.DoneWithOpen())
@@ -771,22 +767,26 @@ use (
 	./moda/a
 )`)
 
-		// Editing the gopls.mod removes modb from the workspace modules, and so
-		// should clear outstanding diagnostics...
-		env.Await(OnceMet(
-			env.DoneWithChange(),
-			EmptyOrNoDiagnostics("modb/go.mod"),
-		))
-		// ...but does not yet cause a workspace reload, so we should still jump to modb.
+		// Simply modifying the go.work file does not cause a reload, so we should
+		// still jump within the workspace.
+		//
+		// TODO: should editing the go.work above cause modb diagnostics to be
+		// suppressed?
+		env.Await(env.DoneWithChange())
 		if err := checkHelloLocation("modb/b/b.go"); err != nil {
 			t.Fatal(err)
 		}
+
 		// Saving should reload the workspace.
 		env.SaveBufferWithoutActions("go.work")
 		if err := checkHelloLocation("b.com@v1.2.3/b/b.go"); err != nil {
 			t.Fatal(err)
 		}
 
+		// This fails if guarded with a OnceMet(DoneWithSave(), ...), because it is
+		// debounced (and therefore not synchronous with the change).
+		env.Await(EmptyOrNoDiagnostics("modb/go.mod"))
+
 		// Test Formatting.
 		env.SetBufferContent("go.work", `go 1.18
   use      (

internal/lsp/cache/load.go

@@ -5,6 +5,7 @@
 package cache
 
 import (
+	"bytes"
 	"context"
 	"crypto/sha256"
 	"errors"
@@ -29,9 +30,16 @@ import (
 
 // load calls packages.Load for the given scopes, updating package metadata,
 // import graph, and mapped files with the result.
+//
+// The resulting error may wrap the moduleErrorMap error type, representing
+// errors associated with specific modules.
 func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) (err error) {
 	var query []string
 	var containsDir bool // for logging
+
+	// Keep track of module query -> module path so that we can later correlate query
+	// errors with errors.
+	moduleQueries := make(map[string]string)
 	for _, scope := range scopes {
 		if !s.shouldLoad(scope) {
 			continue
@@ -66,7 +74,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 			case "std", "cmd":
 				query = append(query, string(scope))
 			default:
-				query = append(query, fmt.Sprintf("%s/...", scope))
+				modQuery := fmt.Sprintf("%s/...", scope)
+				query = append(query, modQuery)
+				moduleQueries[modQuery] = string(scope)
 			}
 		case viewLoadScope:
 			// If we are outside of GOPATH, a module, or some other known
@@ -137,7 +147,24 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 		}
 		return fmt.Errorf("%v: %w", err, source.PackagesLoadError)
 	}
+
+	moduleErrs := make(map[string][]packages.Error) // module path -> errors
 	for _, pkg := range pkgs {
+		// The Go command returns synthetic list results for module queries that
+		// encountered module errors.
+		//
+		// For example, given a module path a.mod, we'll query for "a.mod/..." and
+		// the go command will return a package named "a.mod/..." holding this
+		// error. Save it for later interpretation.
+		//
+		// See golang/go#50862 for more details.
+		if mod := moduleQueries[pkg.PkgPath]; mod != "" { // a synthetic result for the unloadable module
+			if len(pkg.Errors) > 0 {
+				moduleErrs[mod] = pkg.Errors
+			}
+			continue
+		}
+
 		if !containsDir || s.view.Options().VerboseOutput {
 			event.Log(ctx, "go/packages.Load",
 				tag.Snapshot.Of(s.ID()),
@@ -180,9 +207,35 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
 	// Rebuild the import graph when the metadata is updated.
 	s.clearAndRebuildImportGraph()
 
+	if len(moduleErrs) > 0 {
+		return &moduleErrorMap{moduleErrs}
+	}
+
 	return nil
 }
 
+type moduleErrorMap struct {
+	errs map[string][]packages.Error // module path -> errors
+}
+
+func (m *moduleErrorMap) Error() string {
+	var paths []string // sort for stability
+	for path, errs := range m.errs {
+		if len(errs) > 0 { // should always be true, but be cautious
+			paths = append(paths, path)
+		}
+	}
+	sort.Strings(paths)
+
+	var buf bytes.Buffer
+	fmt.Fprintf(&buf, "%d modules have errors:\n", len(paths))
+	for _, path := range paths {
+		fmt.Fprintf(&buf, "\t%s", m.errs[path][0].Msg)
+	}
+
+	return buf.String()
+}
+
 // workspaceLayoutErrors returns a diagnostic for every open file, as well as
 // an error message if there are no open files.
 func (s *snapshot) workspaceLayoutError(ctx context.Context) *source.CriticalError {

internal/lsp/cache/mod.go

@@ -6,6 +6,7 @@ package cache
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"regexp"
@@ -297,36 +298,68 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string
 
 // extractGoCommandError tries to parse errors that come from the go command
 // and shape them into go.mod diagnostics.
-func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError string) ([]*source.Diagnostic, error) {
-	diagLocations := map[*source.ParsedModule]span.Span{}
-	backupDiagLocations := map[*source.ParsedModule]span.Span{}
-
-	// The go command emits parse errors for completely invalid go.mod files.
-	// Those are reported by our own diagnostics and can be ignored here.
-	// As of writing, we are not aware of any other errors that include
-	// file/position information, so don't even try to find it.
-	if strings.Contains(goCmdError, "errors parsing go.mod") {
-		return nil, nil
+// TODO: rename this to 'load errors'
+func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError error) []*source.Diagnostic {
+	if goCmdError == nil {
+		return nil
+	}
+
+	type locatedErr struct {
+		spn span.Span
+		msg string
 	}
+	diagLocations := map[*source.ParsedModule]locatedErr{}
+	backupDiagLocations := map[*source.ParsedModule]locatedErr{}
+
+	// If moduleErrs is non-nil, go command errors are scoped to specific
+	// modules.
+	var moduleErrs *moduleErrorMap
+	_ = errors.As(goCmdError, &moduleErrs)
 
 	// Match the error against all the mod files in the workspace.
 	for _, uri := range s.ModFiles() {
 		fh, err := s.GetFile(ctx, uri)
 		if err != nil {
-			return nil, err
+			event.Error(ctx, "getting modfile for Go command error", err)
+			continue
 		}
 		pm, err := s.ParseMod(ctx, fh)
 		if err != nil {
-			return nil, err
-		}
-		spn, found, err := s.matchErrorToModule(ctx, pm, goCmdError)
-		if err != nil {
-			return nil, err
+			// Parsing errors are reported elsewhere
+			return nil
 		}
-		if found {
-			diagLocations[pm] = spn
+		var msgs []string // error messages to consider
+		if moduleErrs != nil {
+			if pm.File.Module != nil {
+				for _, mes := range moduleErrs.errs[pm.File.Module.Mod.Path] {
+					msgs = append(msgs, mes.Error())
+				}
+			}
 		} else {
-			backupDiagLocations[pm] = spn
+			msgs = append(msgs, goCmdError.Error())
+		}
+		for _, msg := range msgs {
+			if strings.Contains(goCmdError.Error(), "errors parsing go.mod") {
+				// The go command emits parse errors for completely invalid go.mod files.
+				// Those are reported by our own diagnostics and can be ignored here.
+				// As of writing, we are not aware of any other errors that include
+				// file/position information, so don't even try to find it.
+				continue
+			}
+			spn, found, err := s.matchErrorToModule(ctx, pm, msg)
+			if err != nil {
+				event.Error(ctx, "matching error to module", err)
+				continue
+			}
+			le := locatedErr{
+				spn: spn,
+				msg: msg,
+			}
+			if found {
+				diagLocations[pm] = le
+			} else {
+				backupDiagLocations[pm] = le
+			}
 		}
 	}
 
@@ -336,14 +369,15 @@ func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError string
 	}
 
 	var srcErrs []*source.Diagnostic
-	for pm, spn := range diagLocations {
-		diag, err := s.goCommandDiagnostic(pm, spn, goCmdError)
+	for pm, le := range diagLocations {
+		diag, err := s.goCommandDiagnostic(pm, le.spn, le.msg)
 		if err != nil {
-			return nil, err
+			event.Error(ctx, "building go command diagnostic", err)
+			continue
 		}
 		srcErrs = append(srcErrs, diag)
 	}
-	return srcErrs, nil
+	return srcErrs
 }
 
 var moduleVersionInErrorRe = regexp.MustCompile(`[:\s]([+-._~0-9A-Za-z]+)@([+-._~0-9A-Za-z]+)[:\s]`)

internal/lsp/cache/snapshot.go

@@ -1485,14 +1485,14 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr
 	}
 
 	if err := s.reloadWorkspace(ctx); err != nil {
-		diags, _ := s.extractGoCommandErrors(ctx, err.Error())
+		diags := s.extractGoCommandErrors(ctx, err)
 		return &source.CriticalError{
 			MainError: err,
 			DiagList:  diags,
 		}
 	}
 	if err := s.reloadOrphanedFiles(ctx); err != nil {
-		diags, _ := s.extractGoCommandErrors(ctx, err.Error())
+		diags := s.extractGoCommandErrors(ctx, err)
 		return &source.CriticalError{
 			MainError: err,
 			DiagList:  diags,

internal/lsp/cache/view.go

@@ -671,7 +671,7 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) {
 		}
 	case err != nil:
 		event.Error(ctx, "initial workspace load failed", err)
-		extractedDiags, _ := s.extractGoCommandErrors(ctx, err.Error())
+		extractedDiags := s.extractGoCommandErrors(ctx, err)
 		criticalErr = &source.CriticalError{
 			MainError: err,
 			DiagList:  append(modDiagnostics, extractedDiags...),