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>
diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index b283cfa..5e5bcd1 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -705,7 +705,7 @@
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 @@
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 @@
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 @@
./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 (
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index f91c961..5341d5c 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -5,6 +5,7 @@
package cache
import (
+ "bytes"
"context"
"crypto/sha256"
"errors"
@@ -29,9 +30,16 @@
// 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 @@
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 @@
}
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 @@
// 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 {
diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go
index 03fdcbf..5ac199b 100644
--- a/internal/lsp/cache/mod.go
+++ b/internal/lsp/cache/mod.go
@@ -6,6 +6,7 @@
import (
"context"
+ "errors"
"fmt"
"path/filepath"
"regexp"
@@ -297,36 +298,68 @@
// 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
+ // Parsing errors are reported elsewhere
+ return nil
}
- spn, found, err := s.matchErrorToModule(ctx, pm, goCmdError)
- if err != nil {
- return nil, err
- }
- 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 @@
}
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]`)
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index c7cd337..b4fc69f 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1485,14 +1485,14 @@
}
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,
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 17bdc40..b0390a3f 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -671,7 +671,7 @@
}
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...),
