Skip to content

Commit a3d129c

Browse files
findleyrfindleyr
committed
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>
1 parent 6bfd3a4 commit a3d129c

5 files changed

Lines changed: 137 additions & 50 deletions

File tree

‎gopls/internal/regtest/workspace/workspace_test.go‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,7 @@ use (
705705
WithOptions(
706706
ProxyFiles(workspaceModuleProxy),
707707
).Run(t, multiModule, func(t *testing.T, env *Env) {
708-
// Initially, the gopls.mod should cause only the a.com module to be
708+
// Initially, the go.work should cause only the a.com module to be
709709
// loaded. Validate this by jumping to a definition in b.com and ensuring
710710
// that we go to the module cache.
711711
env.OpenFile("moda/a/a.go")
@@ -726,7 +726,7 @@ use (
726726
t.Fatal(err)
727727
}
728728

729-
// Now, modify the gopls.mod file on disk to activate the b.com module in
729+
// Now, modify the go.work file on disk to activate the b.com module in
730730
// the workspace.
731731
env.WriteWorkspaceFile("go.work", `
732732
go 1.17
@@ -742,26 +742,22 @@ use (
742742
env.OpenFile("modb/go.mod")
743743
env.Await(env.DoneWithOpen())
744744

745-
// TODO(golang/go#50862): the go command drops error messages when using
746-
// go.work, so we need to build our go.mod diagnostics in a different way.
747-
if testenv.Go1Point() < 18 {
748-
var d protocol.PublishDiagnosticsParams
749-
env.Await(
750-
OnceMet(
751-
env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"),
752-
ReadDiagnostics("modb/go.mod", &d),
753-
),
754-
)
755-
env.ApplyQuickFixes("modb/go.mod", d.Diagnostics)
756-
env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x"))
757-
}
745+
var d protocol.PublishDiagnosticsParams
746+
env.Await(
747+
OnceMet(
748+
env.DiagnosticAtRegexpWithMessage("modb/go.mod", `require example.com v1.2.3`, "has not been downloaded"),
749+
ReadDiagnostics("modb/go.mod", &d),
750+
),
751+
)
752+
env.ApplyQuickFixes("modb/go.mod", d.Diagnostics)
753+
env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x"))
758754

759755
// Jumping to definition should now go to b.com in the workspace.
760756
if err := checkHelloLocation("modb/b/b.go"); err != nil {
761757
t.Fatal(err)
762758
}
763759

764-
// Now, let's modify the gopls.mod *overlay* (not on disk), and verify that
760+
// Now, let's modify the go.work *overlay* (not on disk), and verify that
765761
// this change is only picked up once it is saved.
766762
env.OpenFile("go.work")
767763
env.Await(env.DoneWithOpen())
@@ -771,22 +767,26 @@ use (
771767
./moda/a
772768
)`)
773769

774-
// Editing the gopls.mod removes modb from the workspace modules, and so
775-
// should clear outstanding diagnostics...
776-
env.Await(OnceMet(
777-
env.DoneWithChange(),
778-
EmptyOrNoDiagnostics("modb/go.mod"),
779-
))
780-
// ...but does not yet cause a workspace reload, so we should still jump to modb.
770+
// Simply modifying the go.work file does not cause a reload, so we should
771+
// still jump within the workspace.
772+
//
773+
// TODO: should editing the go.work above cause modb diagnostics to be
774+
// suppressed?
775+
env.Await(env.DoneWithChange())
781776
if err := checkHelloLocation("modb/b/b.go"); err != nil {
782777
t.Fatal(err)
783778
}
779+
784780
// Saving should reload the workspace.
785781
env.SaveBufferWithoutActions("go.work")
786782
if err := checkHelloLocation("b.com@v1.2.3/b/b.go"); err != nil {
787783
t.Fatal(err)
788784
}
789785

786+
// This fails if guarded with a OnceMet(DoneWithSave(), ...), because it is
787+
// debounced (and therefore not synchronous with the change).
788+
env.Await(EmptyOrNoDiagnostics("modb/go.mod"))
789+
790790
// Test Formatting.
791791
env.SetBufferContent("go.work", `go 1.18
792792
use (

‎internal/lsp/cache/load.go‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package cache
66

77
import (
8+
"bytes"
89
"context"
910
"crypto/sha256"
1011
"errors"
@@ -29,9 +30,16 @@ import (
2930

3031
// load calls packages.Load for the given scopes, updating package metadata,
3132
// import graph, and mapped files with the result.
33+
//
34+
// The resulting error may wrap the moduleErrorMap error type, representing
35+
// errors associated with specific modules.
3236
func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) (err error) {
3337
var query []string
3438
var containsDir bool // for logging
39+
40+
// Keep track of module query -> module path so that we can later correlate query
41+
// errors with errors.
42+
moduleQueries := make(map[string]string)
3543
for _, scope := range scopes {
3644
if !s.shouldLoad(scope) {
3745
continue
@@ -66,7 +74,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
6674
case "std", "cmd":
6775
query = append(query, string(scope))
6876
default:
69-
query = append(query, fmt.Sprintf("%s/...", scope))
77+
modQuery := fmt.Sprintf("%s/...", scope)
78+
query = append(query, modQuery)
79+
moduleQueries[modQuery] = string(scope)
7080
}
7181
case viewLoadScope:
7282
// 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
137147
}
138148
return fmt.Errorf("%v: %w", err, source.PackagesLoadError)
139149
}
150+
151+
moduleErrs := make(map[string][]packages.Error) // module path -> errors
140152
for _, pkg := range pkgs {
153+
// The Go command returns synthetic list results for module queries that
154+
// encountered module errors.
155+
//
156+
// For example, given a module path a.mod, we'll query for "a.mod/..." and
157+
// the go command will return a package named "a.mod/..." holding this
158+
// error. Save it for later interpretation.
159+
//
160+
// See golang/go#50862 for more details.
161+
if mod := moduleQueries[pkg.PkgPath]; mod != "" { // a synthetic result for the unloadable module
162+
if len(pkg.Errors) > 0 {
163+
moduleErrs[mod] = pkg.Errors
164+
}
165+
continue
166+
}
167+
141168
if !containsDir || s.view.Options().VerboseOutput {
142169
event.Log(ctx, "go/packages.Load",
143170
tag.Snapshot.Of(s.ID()),
@@ -180,9 +207,35 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
180207
// Rebuild the import graph when the metadata is updated.
181208
s.clearAndRebuildImportGraph()
182209

210+
if len(moduleErrs) > 0 {
211+
return &moduleErrorMap{moduleErrs}
212+
}
213+
183214
return nil
184215
}
185216

217+
type moduleErrorMap struct {
218+
errs map[string][]packages.Error // module path -> errors
219+
}
220+
221+
func (m *moduleErrorMap) Error() string {
222+
var paths []string // sort for stability
223+
for path, errs := range m.errs {
224+
if len(errs) > 0 { // should always be true, but be cautious
225+
paths = append(paths, path)
226+
}
227+
}
228+
sort.Strings(paths)
229+
230+
var buf bytes.Buffer
231+
fmt.Fprintf(&buf, "%d modules have errors:\n", len(paths))
232+
for _, path := range paths {
233+
fmt.Fprintf(&buf, "\t%s", m.errs[path][0].Msg)
234+
}
235+
236+
return buf.String()
237+
}
238+
186239
// workspaceLayoutErrors returns a diagnostic for every open file, as well as
187240
// an error message if there are no open files.
188241
func (s *snapshot) workspaceLayoutError(ctx context.Context) *source.CriticalError {

‎internal/lsp/cache/mod.go‎

Lines changed: 57 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package cache
66

77
import (
88
"context"
9+
"errors"
910
"fmt"
1011
"path/filepath"
1112
"regexp"
@@ -297,36 +298,68 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string
297298

298299
// extractGoCommandError tries to parse errors that come from the go command
299300
// and shape them into go.mod diagnostics.
300-
func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError string) ([]*source.Diagnostic, error) {
301-
diagLocations := map[*source.ParsedModule]span.Span{}
302-
backupDiagLocations := map[*source.ParsedModule]span.Span{}
303-
304-
// The go command emits parse errors for completely invalid go.mod files.
305-
// Those are reported by our own diagnostics and can be ignored here.
306-
// As of writing, we are not aware of any other errors that include
307-
// file/position information, so don't even try to find it.
308-
if strings.Contains(goCmdError, "errors parsing go.mod") {
309-
return nil, nil
301+
// TODO: rename this to 'load errors'
302+
func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError error) []*source.Diagnostic {
303+
if goCmdError == nil {
304+
return nil
305+
}
306+
307+
type locatedErr struct {
308+
spn span.Span
309+
msg string
310310
}
311+
diagLocations := map[*source.ParsedModule]locatedErr{}
312+
backupDiagLocations := map[*source.ParsedModule]locatedErr{}
313+
314+
// If moduleErrs is non-nil, go command errors are scoped to specific
315+
// modules.
316+
var moduleErrs *moduleErrorMap
317+
_ = errors.As(goCmdError, &moduleErrs)
311318

312319
// Match the error against all the mod files in the workspace.
313320
for _, uri := range s.ModFiles() {
314321
fh, err := s.GetFile(ctx, uri)
315322
if err != nil {
316-
return nil, err
323+
event.Error(ctx, "getting modfile for Go command error", err)
324+
continue
317325
}
318326
pm, err := s.ParseMod(ctx, fh)
319327
if err != nil {
320-
return nil, err
321-
}
322-
spn, found, err := s.matchErrorToModule(ctx, pm, goCmdError)
323-
if err != nil {
324-
return nil, err
328+
// Parsing errors are reported elsewhere
329+
return nil
325330
}
326-
if found {
327-
diagLocations[pm] = spn
331+
var msgs []string // error messages to consider
332+
if moduleErrs != nil {
333+
if pm.File.Module != nil {
334+
for _, mes := range moduleErrs.errs[pm.File.Module.Mod.Path] {
335+
msgs = append(msgs, mes.Error())
336+
}
337+
}
328338
} else {
329-
backupDiagLocations[pm] = spn
339+
msgs = append(msgs, goCmdError.Error())
340+
}
341+
for _, msg := range msgs {
342+
if strings.Contains(goCmdError.Error(), "errors parsing go.mod") {
343+
// The go command emits parse errors for completely invalid go.mod files.
344+
// Those are reported by our own diagnostics and can be ignored here.
345+
// As of writing, we are not aware of any other errors that include
346+
// file/position information, so don't even try to find it.
347+
continue
348+
}
349+
spn, found, err := s.matchErrorToModule(ctx, pm, msg)
350+
if err != nil {
351+
event.Error(ctx, "matching error to module", err)
352+
continue
353+
}
354+
le := locatedErr{
355+
spn: spn,
356+
msg: msg,
357+
}
358+
if found {
359+
diagLocations[pm] = le
360+
} else {
361+
backupDiagLocations[pm] = le
362+
}
330363
}
331364
}
332365

@@ -336,14 +369,15 @@ func (s *snapshot) extractGoCommandErrors(ctx context.Context, goCmdError string
336369
}
337370

338371
var srcErrs []*source.Diagnostic
339-
for pm, spn := range diagLocations {
340-
diag, err := s.goCommandDiagnostic(pm, spn, goCmdError)
372+
for pm, le := range diagLocations {
373+
diag, err := s.goCommandDiagnostic(pm, le.spn, le.msg)
341374
if err != nil {
342-
return nil, err
375+
event.Error(ctx, "building go command diagnostic", err)
376+
continue
343377
}
344378
srcErrs = append(srcErrs, diag)
345379
}
346-
return srcErrs, nil
380+
return srcErrs
347381
}
348382

349383
var moduleVersionInErrorRe = regexp.MustCompile(`[:\s]([+-._~0-9A-Za-z]+)@([+-._~0-9A-Za-z]+)[:\s]`)

‎internal/lsp/cache/snapshot.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1485,14 +1485,14 @@ func (s *snapshot) awaitLoadedAllErrors(ctx context.Context) *source.CriticalErr
14851485
}
14861486

14871487
if err := s.reloadWorkspace(ctx); err != nil {
1488-
diags, _ := s.extractGoCommandErrors(ctx, err.Error())
1488+
diags := s.extractGoCommandErrors(ctx, err)
14891489
return &source.CriticalError{
14901490
MainError: err,
14911491
DiagList: diags,
14921492
}
14931493
}
14941494
if err := s.reloadOrphanedFiles(ctx); err != nil {
1495-
diags, _ := s.extractGoCommandErrors(ctx, err.Error())
1495+
diags := s.extractGoCommandErrors(ctx, err)
14961496
return &source.CriticalError{
14971497
MainError: err,
14981498
DiagList: diags,

‎internal/lsp/cache/view.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -671,7 +671,7 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) {
671671
}
672672
case err != nil:
673673
event.Error(ctx, "initial workspace load failed", err)
674-
extractedDiags, _ := s.extractGoCommandErrors(ctx, err.Error())
674+
extractedDiags := s.extractGoCommandErrors(ctx, err)
675675
criticalErr = &source.CriticalError{
676676
MainError: err,
677677
DiagList: append(modDiagnostics, extractedDiags...),

0 commit comments

Comments
 (0)