diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index b283cfa7274..5e5bcd13b5d 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/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 ( diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index f91c961bc97..5341d5c6243 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 @@ 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 { diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 03fdcbf0863..5ac199bd96b 100644 --- a/internal/lsp/cache/mod.go +++ b/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]`) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index c7cd337184c..b4fc69f1b54 100644 --- a/internal/lsp/cache/snapshot.go +++ b/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, diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 17bdc40065d..b0390a3fbde 100644 --- a/internal/lsp/cache/view.go +++ b/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...),