From 39188db5885871ea0132a6314eab0c1b5d179a8b Mon Sep 17 00:00:00 2001 From: Peter Weinbergr Date: Fri, 21 Aug 2020 08:50:11 -0400 Subject: [PATCH] internal/lsp: add support for RelatedInformation in diagnostics The type checker sometimes emits secondary diagnostics. For instance, if a function is defined twice, then when it sees the second definition it emits a diagnostic at the second definition and a secondary diagnostic pointing to the first diagnostic. Presently gopls treats these as two separate diagnostics. The changed code still produces two diagnostics, but now the secondary diagnostic is also converted into a RelatedInformation so the user sees a xpointer to the earlier definition. Updates https://github.com/golang/go/issues/39062. Change-Id: Ic421ec91d2b46c28681ab3ec010d5b02c0442e68 Reviewed-on: https://go-review.googlesource.com/c/tools/+/251617 Run-TryBot: Peter Weinberger TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- gopls/internal/regtest/diagnostics_test.go | 47 ++++++++++++++++++++-- internal/lsp/cache/check.go | 47 +++++++++++++++++++++- internal/lsp/cache/errors.go | 29 ++++++++++++- internal/lsp/diagnostics.go | 5 ++- internal/lsp/source/diagnostics.go | 1 + 5 files changed, 120 insertions(+), 9 deletions(-) diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index a7973fe29b..96d61b11a8 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -1026,21 +1026,21 @@ func main() { writeGoVim(env, "p/p.go", p) writeGoVim(env, "main.go", main) writeGoVim(env, "p/p_test.go", `package p - + import "testing" - + func TestDoIt(t *testing.T) { DoIt(5) } `) writeGoVim(env, "p/x_test.go", `package p_test - + import ( "testing" "mod.com/p" ) - + func TestDoIt(t *testing.T) { p.DoIt(5) } @@ -1274,3 +1274,42 @@ func main() { env.Await(env.DiagnosticAtRegexp("main.go", `t{"msg"}`)) }) } + +// Test some secondary diagnostics +func TestSecondaryDiagnostics(t *testing.T) { + const dir = ` +-- mod -- +module mod.com +-- main.go -- +package main +func main() { + panic("not here") +} +-- other.go -- +package main +func main() {} +` + runner.Run(t, dir, func(t *testing.T, env *Env) { + log.SetFlags(log.Lshortfile) + env.OpenFile("main.go") + env.OpenFile("other.go") + env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1)) + x := env.DiagnosticsFor("main.go") + if len(x.Diagnostics) != 1 { + t.Errorf("main.go, got %d diagnostics, expected 1", len(x.Diagnostics)) + } + keep := x.Diagnostics[0] + y := env.DiagnosticsFor("other.go") + if len(y.Diagnostics) != 1 { + t.Errorf("other.go: got %d diagnostics, expected 1", len(y.Diagnostics)) + } + if len(y.Diagnostics[0].RelatedInformation) != 1 { + t.Errorf("got %d RelatedInformations, expected 1", len(y.Diagnostics[0].RelatedInformation)) + } + // check that the RelatedInformation matches the error from main.go + c := y.Diagnostics[0].RelatedInformation[0] + if c.Location.Range != keep.Range { + t.Errorf("locations don't match. Got %v expected %v", c.Location.Range, keep.Range) + } + }) +} diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 75642c20c6..8311416c82 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -405,6 +405,7 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source // We don't care about a package's errors unless we have parsed it in full. if mode == source.ParseFull { + expandErrors(rawErrors) for _, e := range rawErrors { srcErr, err := sourceError(ctx, snapshot, pkg, e) if err != nil { @@ -412,8 +413,8 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source continue } pkg.errors = append(pkg.errors, srcErr) - if err, ok := e.(types.Error); ok { - pkg.typeErrors = append(pkg.typeErrors, err) + if err, ok := e.(extendedError); ok { + pkg.typeErrors = append(pkg.typeErrors, err.primary) } } } @@ -421,6 +422,48 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source return pkg, nil } +type extendedError struct { + primary types.Error + secondaries []types.Error +} + +func (e extendedError) Error() string { + return e.primary.Error() +} + +// expandErrors duplicates "secondary" errors by mapping them to their main +// error. Some errors returned by the type checker are followed by secondary +// errors which give more information about the error. These are errors in +// their own right, and they are marked by starting with \t. For instance, when +// there is a multiply-defined function, the secondary error points back to the +// definition first noticed. +// +// This code associates the secondary error with its primary error, which can +// then be used as RelatedInformation when the error becomes a diagnostic. +func expandErrors(errs []error) []error { + for i := 0; i < len(errs); { + e, ok := errs[i].(types.Error) + if !ok { + i++ + continue + } + enew := extendedError{ + primary: e, + } + j := i + 1 + for ; j < len(errs); j++ { + spl, ok := errs[j].(types.Error) + if !ok || len(spl.Msg) == 0 || spl.Msg[0] != '\t' { + break + } + enew.secondaries = append(enew.secondaries, spl) + } + errs[i] = enew + i = j + } + return errs +} + // resolveImportPath resolves an import path in pkg to a package from deps. // It should produce the same results as resolveImportPath: // https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/load/pkg.go;drc=641918ee09cb44d282a30ee8b66f99a0b63eaef9;l=990. diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index cf0241726a..1f55a586b7 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -94,7 +94,32 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ if err != nil { return nil, err } - + case extendedError: + perr := e.primary + msg = perr.Msg + kind = source.TypeError + if !perr.Pos.IsValid() { + return nil, fmt.Errorf("invalid position for type error %v", e) + } + spn, err = typeErrorRange(snapshot, fset, pkg, perr.Pos) + if err != nil { + return nil, err + } + for _, s := range e.secondaries { + var x source.RelatedInformation + x.Message = s.Msg + xspn, err := typeErrorRange(snapshot, fset, pkg, s.Pos) + if err != nil { + return nil, fmt.Errorf("invalid position for type error %v", s) + } + x.URI = xspn.URI() + rng, err := spanToRange(snapshot, pkg, xspn) + if err != nil { + return nil, err + } + x.Range = rng + related = append(related, x) + } case *analysis.Diagnostic: spn, err = span.NewRange(fset, e.Pos, e.End).Span() if err != nil { @@ -111,6 +136,8 @@ func sourceError(ctx context.Context, snapshot *snapshot, pkg *pkg, e interface{ if err != nil { return nil, err } + default: + panic(fmt.Sprintf("%T unexpected", e)) } rng, err := spanToRange(snapshot, pkg, spn) if err != nil { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 00bbc3be10..71662f32ac 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -156,7 +156,7 @@ If you believe this is a mistake, please file an issue: https://github.com/golan return } - // Add all reports to the global map, checking for duplciates. + // Add all reports to the global map, checking for duplicates. reportsMu.Lock() for id, diags := range pkgReports { key := idWithAnalysis{ @@ -307,7 +307,8 @@ func toProtocolDiagnostics(diagnostics []*source.Diagnostic) []protocol.Diagnost }) } reports = append(reports, protocol.Diagnostic{ - Message: strings.TrimSpace(diag.Message), // go list returns errors prefixed by newline + // diag.Message might start with \n or \t + Message: strings.TrimSpace(diag.Message), Range: diag.Range, Severity: diag.Severity, Source: diag.Source, diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 0ca38de8c2..583bb314fa 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -159,6 +159,7 @@ func diagnostics(ctx context.Context, snapshot Snapshot, reports map[VersionedFi Message: e.Message, Range: e.Range, Severity: protocol.SeverityError, + Related: e.Related, } set, ok := diagSets[e.URI] if !ok {