Skip to content

Commit 929a849

Browse files
stamblerrestamblerre
committed
internal/lsp: restructure the way we report critical errors
The fragmentation of the critical error reporting is getting in the way of small fixes. This change adds a GetCriticalError function that reports critical errors, while ModTidy and WorkspacePackages no longer report critical errors. Any function that wants to process critical errors should call AwaitLoaded. Some other smaller changes are made to account for these changes. One change is that we may report multiple *source.Errors, with the assumption that duplicate diagnostics will be caught by the diagnostic caching. Also, any `go list` error message that ends with "to add it" is now considered an error that gets the "add dependency" suggested fix. Fixes golang/go#43338 Fixes golang/go#43307 Change-Id: I056b32e0a0495d9a1dcc64f9f5103649a6102645 Reviewed-on: https://go-review.googlesource.com/c/tools/+/280093 Run-TryBot: Rebecca Stambler <rstambler@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Trust: Rebecca Stambler <rstambler@golang.org> Reviewed-by: Robert Findley <rfindley@google.com>
1 parent fbbba25 commit 929a849

File tree

11 files changed

+243
-175
lines changed

11 files changed

+243
-175
lines changed

‎gopls/internal/regtest/modfile_test.go

+54
Original file line numberDiff line numberDiff line change
@@ -937,5 +937,59 @@ require random.com v1.2.3
937937
}
938938
})
939939
})
940+
}
941+
942+
func TestSumUpdateQuickFix(t *testing.T) {
943+
// Error messages changed in 1.16 that changed the diagnostic positions.
944+
testenv.NeedsGo1Point(t, 16)
945+
946+
const mod = `
947+
-- go.mod --
948+
module mod.com
949+
950+
go 1.12
951+
952+
require (
953+
example.com v1.2.3
954+
)
955+
-- go.sum --
956+
-- main.go --
957+
package main
958+
959+
import (
960+
"example.com/blah"
961+
)
940962
963+
func main() {
964+
blah.Hello()
965+
}
966+
`
967+
withOptions(
968+
ProxyFiles(workspaceProxy),
969+
Modes(Singleton),
970+
).run(t, mod, func(t *testing.T, env *Env) {
971+
env.OpenFile("go.mod")
972+
pos := env.RegexpSearch("go.mod", "example.com")
973+
params := &protocol.PublishDiagnosticsParams{}
974+
env.Await(
975+
OnceMet(
976+
env.DiagnosticAtRegexp("go.mod", "example.com"),
977+
ReadDiagnostics("go.mod", params),
978+
),
979+
)
980+
var diagnostic protocol.Diagnostic
981+
for _, d := range params.Diagnostics {
982+
if d.Range.Start.Line == float64(pos.Line) {
983+
diagnostic = d
984+
break
985+
}
986+
}
987+
env.ApplyQuickFixes("go.mod", []protocol.Diagnostic{diagnostic})
988+
const want = `example.com v1.2.3 h1:Yryq11hF02fEf2JlOS2eph+ICE2/ceevGV3C9dl5V/c=
989+
example.com v1.2.3/go.mod h1:Y2Rc5rVWjWur0h3pd9aEvK5Pof8YKDANh9gHA2Maujo=
990+
`
991+
if got := env.ReadWorkspaceFile("go.sum"); got != want {
992+
t.Fatalf("unexpected go.sum contents:\n%s", tests.Diff(t, want, got))
993+
}
994+
})
941995
}

‎internal/lsp/cache/load.go

+2-30
Original file line numberDiff line numberDiff line change
@@ -128,13 +128,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
128128
event.Log(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
129129
}
130130
if len(pkgs) == 0 {
131-
if err != nil {
132-
// Try to extract the load error into a structured error with
133-
// diagnostics.
134-
if criticalErr := s.parseLoadError(ctx, err); criticalErr != nil {
135-
return criticalErr
136-
}
137-
} else {
131+
if err == nil {
138132
err = fmt.Errorf("no packages returned")
139133
}
140134
return errors.Errorf("%v: %w", err, source.PackagesLoadError)
@@ -182,31 +176,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf
182176
return nil
183177
}
184178

185-
func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) *source.CriticalError {
186-
if strings.Contains(loadErr.Error(), "cannot find main module") {
187-
return s.WorkspaceLayoutError(ctx)
188-
}
189-
criticalErr := &source.CriticalError{
190-
MainError: loadErr,
191-
}
192-
// Attempt to place diagnostics in the relevant go.mod files, if any.
193-
for _, uri := range s.ModFiles() {
194-
fh, err := s.GetFile(ctx, uri)
195-
if err != nil {
196-
continue
197-
}
198-
srcErr := s.extractGoCommandError(ctx, s, fh, loadErr.Error())
199-
if srcErr == nil {
200-
continue
201-
}
202-
criticalErr.ErrorList = append(criticalErr.ErrorList, srcErr)
203-
}
204-
return criticalErr
205-
}
206-
207179
// workspaceLayoutErrors returns a diagnostic for every open file, as well as
208180
// an error message if there are no open files.
209-
func (s *snapshot) WorkspaceLayoutError(ctx context.Context) *source.CriticalError {
181+
func (s *snapshot) workspaceLayoutError(ctx context.Context) *source.CriticalError {
210182
if len(s.workspace.getKnownModFiles()) == 0 {
211183
return nil
212184
}

‎internal/lsp/cache/mod.go

+43-26
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func (s *snapshot) ParseMod(ctx context.Context, modFH source.FileHandle) (*sour
7373
// Attempt to convert the error to a standardized parse error.
7474
var parseErrors []*source.Error
7575
if err != nil {
76-
if parseErr, extractErr := extractErrorWithPosition(ctx, err.Error(), s); extractErr == nil {
76+
if parseErr := extractErrorWithPosition(ctx, err.Error(), s); parseErr != nil {
7777
parseErrors = []*source.Error{parseErr}
7878
}
7979
}
@@ -341,29 +341,39 @@ var moduleAtVersionRe = regexp.MustCompile(`^(?P<module>.*)@(?P<version>.*)$`)
341341

342342
// extractGoCommandError tries to parse errors that come from the go command
343343
// and shape them into go.mod diagnostics.
344-
func (s *snapshot) extractGoCommandError(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, goCmdError string) *source.Error {
344+
func (s *snapshot) extractGoCommandErrors(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, goCmdError string) []*source.Error {
345+
var srcErrs []*source.Error
346+
if srcErr := s.parseModError(ctx, fh, goCmdError); srcErr != nil {
347+
srcErrs = append(srcErrs, srcErr)
348+
}
345349
// If the error message contains a position, use that. Don't pass a file
346350
// handle in, as it might not be the file associated with the error.
347-
if srcErr, err := extractErrorWithPosition(ctx, goCmdError, s); err == nil {
348-
return srcErr
351+
if srcErr := extractErrorWithPosition(ctx, goCmdError, s); srcErr != nil {
352+
srcErrs = append(srcErrs, srcErr)
353+
} else if srcErr := s.matchErrorToModule(ctx, fh, goCmdError); srcErr != nil {
354+
srcErrs = append(srcErrs, srcErr)
349355
}
350-
// We try to match module versions in error messages. Some examples:
351-
//
352-
// example.com@v1.2.2: reading example.com/@v/v1.2.2.mod: no such file or directory
353-
// go: github.com/cockroachdb/apd/v2@v2.0.72: reading github.com/cockroachdb/apd/go.mod at revision v2.0.72: unknown revision v2.0.72
354-
// go: example.com@v1.2.3 requires\n\trandom.org@v1.2.3: parsing go.mod:\n\tmodule declares its path as: bob.org\n\tbut was required as: random.org
355-
//
356-
// We split on colons and whitespace, and attempt to match on something
357-
// that matches module@version. If we're able to find a match, we try to
358-
// find anything that matches it in the go.mod file.
356+
return srcErrs
357+
}
358+
359+
// matchErrorToModule attempts to match module version in error messages.
360+
// Some examples:
361+
//
362+
// example.com@v1.2.2: reading example.com/@v/v1.2.2.mod: no such file or directory
363+
// go: github.com/cockroachdb/apd/v2@v2.0.72: reading github.com/cockroachdb/apd/go.mod at revision v2.0.72: unknown revision v2.0.72
364+
// go: example.com@v1.2.3 requires\n\trandom.org@v1.2.3: parsing go.mod:\n\tmodule declares its path as: bob.org\n\tbut was required as: random.org
365+
//
366+
// We split on colons and whitespace, and attempt to match on something
367+
// that matches module@version. If we're able to find a match, we try to
368+
// find anything that matches it in the go.mod file.
369+
func (s *snapshot) matchErrorToModule(ctx context.Context, fh source.FileHandle, goCmdError string) *source.Error {
359370
var v module.Version
360371
fields := strings.FieldsFunc(goCmdError, func(r rune) bool {
361372
return unicode.IsSpace(r) || r == ':'
362373
})
363-
for _, s := range fields {
364-
s = strings.TrimSpace(s)
365-
match := moduleAtVersionRe.FindStringSubmatch(s)
366-
if match == nil || len(match) < 3 {
374+
for _, field := range fields {
375+
match := moduleAtVersionRe.FindStringSubmatch(field)
376+
if match == nil {
367377
continue
368378
}
369379
path, version := match[1], match[2]
@@ -378,7 +388,7 @@ func (s *snapshot) extractGoCommandError(ctx context.Context, snapshot source.Sn
378388
v.Path, v.Version = path, version
379389
break
380390
}
381-
pm, err := snapshot.ParseMod(ctx, fh)
391+
pm, err := s.ParseMod(ctx, fh)
382392
if err != nil {
383393
return nil
384394
}
@@ -387,13 +397,19 @@ func (s *snapshot) extractGoCommandError(ctx context.Context, snapshot source.Sn
387397
if err != nil {
388398
return nil
389399
}
390-
if v.Path != "" && strings.Contains(goCmdError, "disabled by GOPROXY=off") {
400+
disabledByGOPROXY := strings.Contains(goCmdError, "disabled by GOPROXY=off")
401+
shouldAddDep := strings.Contains(goCmdError, "to add it")
402+
if v.Path != "" && (disabledByGOPROXY || shouldAddDep) {
391403
args, err := source.MarshalArgs(fh.URI(), false, []string{fmt.Sprintf("%v@%v", v.Path, v.Version)})
392404
if err != nil {
393405
return nil
394406
}
407+
msg := goCmdError
408+
if disabledByGOPROXY {
409+
msg = fmt.Sprintf("%v@%v has not been downloaded", v.Path, v.Version)
410+
}
395411
return &source.Error{
396-
Message: fmt.Sprintf("%v@%v has not been downloaded", v.Path, v.Version),
412+
Message: msg,
397413
Kind: source.ListError,
398414
Range: rng,
399415
URI: fh.URI(),
@@ -411,6 +427,7 @@ func (s *snapshot) extractGoCommandError(ctx context.Context, snapshot source.Sn
411427
Message: goCmdError,
412428
Range: rng,
413429
URI: fh.URI(),
430+
Kind: source.ListError,
414431
}
415432
}
416433
// Check if there are any require, exclude, or replace statements that
@@ -449,10 +466,10 @@ var errorPositionRe = regexp.MustCompile(`(?P<pos>.*:([\d]+)(:([\d]+))?): (?P<ms
449466
// information for the given unstructured error. If a file handle is provided,
450467
// the error position will be on that file. This is useful for parse errors,
451468
// where we already know the file with the error.
452-
func extractErrorWithPosition(ctx context.Context, goCmdError string, src source.FileSource) (*source.Error, error) {
469+
func extractErrorWithPosition(ctx context.Context, goCmdError string, src source.FileSource) *source.Error {
453470
matches := errorPositionRe.FindStringSubmatch(strings.TrimSpace(goCmdError))
454471
if len(matches) == 0 {
455-
return nil, fmt.Errorf("error message doesn't contain a position")
472+
return nil
456473
}
457474
var pos, msg string
458475
for i, name := range errorPositionRe.SubexpNames() {
@@ -466,11 +483,11 @@ func extractErrorWithPosition(ctx context.Context, goCmdError string, src source
466483
spn := span.Parse(pos)
467484
fh, err := src.GetFile(ctx, spn.URI())
468485
if err != nil {
469-
return nil, err
486+
return nil
470487
}
471488
content, err := fh.Read()
472489
if err != nil {
473-
return nil, err
490+
return nil
474491
}
475492
m := &protocol.ColumnMapper{
476493
URI: spn.URI(),
@@ -479,7 +496,7 @@ func extractErrorWithPosition(ctx context.Context, goCmdError string, src source
479496
}
480497
rng, err := m.Range(spn)
481498
if err != nil {
482-
return nil, err
499+
return nil
483500
}
484501
category := GoCommandError
485502
if fh != nil {
@@ -490,5 +507,5 @@ func extractErrorWithPosition(ctx context.Context, goCmdError string, src source
490507
Message: msg,
491508
Range: rng,
492509
URI: spn.URI(),
493-
}, nil
510+
}
494511
}

0 commit comments

Comments
 (0)