From 004141db30c27a95e6dab434de556ebb55419cc5 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 21 Nov 2019 15:14:48 -0500 Subject: [PATCH] internal/lsp: delete the source.Diagnostic.File field Since diagnostics are published with the URI separately, there's no need for us to keep the FileIdentity around in two places. Change-Id: I5724b9582e5eee49f66fcf9f08625f14a69e3fc0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/208263 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cmd/test/check.go | 6 ++-- internal/lsp/lsp_test.go | 2 +- internal/lsp/source/diagnostics.go | 47 +++++++++++++++--------------- internal/lsp/source/source_test.go | 2 +- internal/lsp/tests/diagnostics.go | 25 +++++++--------- internal/lsp/tests/tests.go | 3 -- 6 files changed, 40 insertions(+), 45 deletions(-) diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go index 74b27cb5ee..98d390c253 100644 --- a/internal/lsp/cmd/test/check.go +++ b/internal/lsp/cmd/test/check.go @@ -51,12 +51,12 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti got[l] = struct{}{} } for _, diag := range want { - expect := fmt.Sprintf("%v:%v:%v: %v", diag.File.URI.Filename(), diag.Range.Start.Line+1, diag.Range.Start.Character+1, diag.Message) + expect := fmt.Sprintf("%v:%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Range.Start.Character+1, diag.Message) if diag.Range.Start.Character == 0 { - expect = fmt.Sprintf("%v:%v: %v", diag.File.URI.Filename(), diag.Range.Start.Line+1, diag.Message) + expect = fmt.Sprintf("%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Message) } // Skip the badimport test for now, until we do a better job with diagnostic ranges. - if strings.Contains(diag.File.URI.Filename(), "badimport") { + if strings.Contains(uri.Filename(), "badimport") { continue } _, found := got[expect] diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index e4cb28b546..938da4532e 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -91,7 +91,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti } return } - if diff := tests.DiffDiagnostics(want, got); diff != "" { + if diff := tests.DiffDiagnostics(uri, want, got); diff != "" { t.Error(diff) } } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index a3c8e8cd33..6019b63059 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -14,10 +14,10 @@ import ( "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/log" "golang.org/x/tools/internal/telemetry/trace" + errors "golang.org/x/xerrors" ) type Diagnostic struct { - File FileIdentity Range protocol.Range Message string Source string @@ -82,7 +82,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse } // Run diagnostics for the package that this URI belongs to. - if !diagnostics(ctx, pkg, reports) { + if !diagnostics(ctx, snapshot, pkg, reports) { // If we don't have any list, parse, or type errors, run analyses. if err := analyses(ctx, snapshot, cph, disabledAnalyses, reports); err != nil { log.Error(ctx, "failed to run analyses", err, telemetry.File.Of(f.URI())) @@ -98,7 +98,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, f File, disabledAnalyse for _, fh := range pkg.CompiledGoFiles() { clearReports(snapshot, reports, fh.File().Identity()) } - diagnostics(ctx, pkg, reports) + diagnostics(ctx, snapshot, pkg, reports) } return reports, warningMsg, nil } @@ -107,7 +107,7 @@ type diagnosticSet struct { listErrors, parseErrors, typeErrors []*Diagnostic } -func diagnostics(ctx context.Context, pkg Package, reports map[FileIdentity][]Diagnostic) bool { +func diagnostics(ctx context.Context, snapshot Snapshot, pkg Package, reports map[FileIdentity][]Diagnostic) bool { ctx, done := trace.StartSpan(ctx, "source.diagnostics", telemetry.Package.Of(pkg.ID())) _ = ctx // circumvent SA4006 defer done() @@ -115,7 +115,6 @@ func diagnostics(ctx context.Context, pkg Package, reports map[FileIdentity][]Di diagSets := make(map[FileIdentity]*diagnosticSet) for _, e := range pkg.GetErrors() { diag := &Diagnostic{ - File: e.File, Message: e.Message, Range: e.Range, Severity: protocol.SeverityError, @@ -149,11 +148,7 @@ func diagnostics(ctx context.Context, pkg Package, reports map[FileIdentity][]Di if len(diags) > 0 { nonEmptyDiagnostics = true } - for _, diag := range diags { - if _, ok := reports[fileID]; ok { - reports[fileID] = append(reports[fileID], *diag) - } - } + addReports(ctx, reports, snapshot, fileID, diags...) } return nonEmptyDiagnostics } @@ -181,8 +176,7 @@ func analyses(ctx context.Context, snapshot Snapshot, cph CheckPackageHandle, di if onlyDeletions(e.SuggestedFixes) { tags = append(tags, protocol.Unnecessary) } - addReport(ctx, snapshot, reports, Diagnostic{ - File: e.File, + addReports(ctx, reports, snapshot, e.File, &Diagnostic{ Range: e.Range, Message: e.Message, Source: e.Category, @@ -202,25 +196,32 @@ func clearReports(snapshot Snapshot, reports map[FileIdentity][]Diagnostic, file reports[fileID] = []Diagnostic{} } -func addReport(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, diagnostic Diagnostic) error { - if snapshot.View().Ignore(diagnostic.File.URI) { +func addReports(ctx context.Context, reports map[FileIdentity][]Diagnostic, snapshot Snapshot, fileID FileIdentity, diagnostics ...*Diagnostic) error { + if snapshot.View().Ignore(fileID.URI) { return nil } - if _, ok := reports[diagnostic.File]; ok { - reports[diagnostic.File] = append(reports[diagnostic.File], diagnostic) + if _, ok := reports[fileID]; !ok { + return errors.Errorf("diagnostics for unexpected file %s", fileID.URI) + } + for _, diag := range diagnostics { + if diag == nil { + continue + } + reports[fileID] = append(reports[fileID], *diag) } return nil } func singleDiagnostic(fileID FileIdentity, format string, a ...interface{}) map[FileIdentity][]Diagnostic { return map[FileIdentity][]Diagnostic{ - fileID: []Diagnostic{{ - File: fileID, - Source: "gopls", - Range: protocol.Range{}, - Message: fmt.Sprintf(format, a...), - Severity: protocol.SeverityError, - }}, + fileID: []Diagnostic{ + { + Source: "gopls", + Range: protocol.Range{}, + Message: fmt.Sprintf(format, a...), + Severity: protocol.SeverityError, + }, + }, } } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index b2c0f539a2..67b1aea987 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -85,7 +85,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti } return } - if diff := tests.DiffDiagnostics(want, got); diff != "" { + if diff := tests.DiffDiagnostics(fileID.URI, want, got); diff != "" { t.Error(diff) } } diff --git a/internal/lsp/tests/diagnostics.go b/internal/lsp/tests/diagnostics.go index 8ec1c6ede3..6b0c1cf1c5 100644 --- a/internal/lsp/tests/diagnostics.go +++ b/internal/lsp/tests/diagnostics.go @@ -13,34 +13,34 @@ import ( // DiffDiagnostics prints the diff between expected and actual diagnostics test // results. -func DiffDiagnostics(want, got []source.Diagnostic) string { +func DiffDiagnostics(uri span.URI, want, got []source.Diagnostic) string { sortDiagnostics(want) sortDiagnostics(got) if len(got) != len(want) { - return summarizeDiagnostics(-1, want, got, "different lengths got %v want %v", len(got), len(want)) + return summarizeDiagnostics(-1, uri, want, got, "different lengths got %v want %v", len(got), len(want)) } for i, w := range want { g := got[i] if w.Message != g.Message { - return summarizeDiagnostics(i, want, got, "incorrect Message got %v want %v", g.Message, w.Message) + return summarizeDiagnostics(i, uri, want, got, "incorrect Message got %v want %v", g.Message, w.Message) } if w.Severity != g.Severity { - return summarizeDiagnostics(i, want, got, "incorrect Severity got %v want %v", g.Severity, w.Severity) + return summarizeDiagnostics(i, uri, want, got, "incorrect Severity got %v want %v", g.Severity, w.Severity) } if w.Source != g.Source { - return summarizeDiagnostics(i, want, got, "incorrect Source got %v want %v", g.Source, w.Source) + return summarizeDiagnostics(i, uri, want, got, "incorrect Source got %v want %v", g.Source, w.Source) } // Don't check the range on the badimport test. - if strings.Contains(string(g.File.URI), "badimport") { + if strings.Contains(uri.Filename(), "badimport") { continue } if protocol.ComparePosition(w.Range.Start, g.Range.Start) != 0 { - return summarizeDiagnostics(i, want, got, "incorrect Start got %v want %v", g.Range.Start, w.Range.Start) + return summarizeDiagnostics(i, uri, want, got, "incorrect Start got %v want %v", g.Range.Start, w.Range.Start) } if !protocol.IsPoint(g.Range) { // Accept any 'want' range if the diagnostic returns a zero-length range. if protocol.ComparePosition(w.Range.End, g.Range.End) != 0 { - return summarizeDiagnostics(i, want, got, "incorrect End got %v want %v", g.Range.End, w.Range.End) + return summarizeDiagnostics(i, uri, want, got, "incorrect End got %v want %v", g.Range.End, w.Range.End) } } } @@ -54,9 +54,6 @@ func sortDiagnostics(d []source.Diagnostic) { } func compareDiagnostic(a, b source.Diagnostic) int { - if r := span.CompareURI(a.File.URI, b.File.URI); r != 0 { - return r - } if r := protocol.CompareRange(a.Range, b.Range); r != 0 { return r } @@ -70,7 +67,7 @@ func compareDiagnostic(a, b source.Diagnostic) int { } } -func summarizeDiagnostics(i int, want []source.Diagnostic, got []source.Diagnostic, reason string, args ...interface{}) string { +func summarizeDiagnostics(i int, uri span.URI, want []source.Diagnostic, got []source.Diagnostic, reason string, args ...interface{}) string { msg := &bytes.Buffer{} fmt.Fprint(msg, "diagnostics failed") if i >= 0 { @@ -80,11 +77,11 @@ func summarizeDiagnostics(i int, want []source.Diagnostic, got []source.Diagnost fmt.Fprintf(msg, reason, args...) fmt.Fprint(msg, ":\nexpected:\n") for _, d := range want { - fmt.Fprintf(msg, " %s:%v: %s\n", d.File.URI, d.Range, d.Message) + fmt.Fprintf(msg, " %s:%v: %s\n", uri, d.Range, d.Message) } fmt.Fprintf(msg, "got:\n") for _, d := range got { - fmt.Fprintf(msg, " %s:%v: %s\n", d.File.URI, d.Range, d.Message) + fmt.Fprintf(msg, " %s:%v: %s\n", uri, d.Range, d.Message) } return msg.String() } diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 6aeebbdfc4..7919440f9d 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -704,9 +704,6 @@ func (data *Data) collectDiagnostics(spn span.Span, msgSource, msg string) { // This is not the correct way to do this, // but it seems excessive to do the full conversion here. want := source.Diagnostic{ - File: source.FileIdentity{ - URI: spn.URI(), - }, Range: protocol.Range{ Start: protocol.Position{ Line: float64(spn.Start().Line()) - 1,