From 8a1a3df092004fb36c16c80f6abee1271c2066bf Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 7 Jan 2020 18:00:43 -0500 Subject: [PATCH] internal/lsp: allow diagnostics to be computed per-file or per-package This change splits source.Diagnostics into source.FileDiagnostics and source.PackageDiagnostics. There isn't much difference in functionality, but this just simplifies the process of getting diagnostics if you only have a PackageHandle. Change-Id: I62c0de8778065a4c46cc5672e2834ce5c63fe012 Reviewed-on: https://go-review.googlesource.com/c/tools/+/213644 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/diagnostics.go | 30 ++++++++++-------------------- internal/lsp/lsp_test.go | 4 ++-- internal/lsp/source/diagnostics.go | 29 ++++++++++++++++++++--------- internal/lsp/source/source_test.go | 2 +- 4 files changed, 33 insertions(+), 32 deletions(-) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 5daf762650..cad56fbb21 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -37,30 +37,20 @@ func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot) return } for _, id := range wsPackages { - ph, err := snapshot.PackageHandle(ctx, id) - if err != nil { - log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id)) - continue - } - if len(ph.CompiledGoFiles()) == 0 { - continue - } - // Find a file on which to call diagnostics. - uri := ph.CompiledGoFiles()[0].File().Identity().URI - fh, err := snapshot.GetFile(ctx, uri) - if err != nil { - continue - } - // Run diagnostics on the workspace package. - go func(snapshot source.Snapshot, fh source.FileHandle) { - reports, _, err := source.Diagnostics(ctx, snapshot, fh, false, snapshot.View().Options().DisabledAnalyses) + go func(id string) { + ph, err := snapshot.PackageHandle(ctx, id) if err != nil { - log.Error(ctx, "no diagnostics", err, telemetry.URI.Of(fh.Identity().URI)) + log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id)) + return + } + reports, _, err := source.PackageDiagnostics(ctx, snapshot, ph, false, snapshot.View().Options().DisabledAnalyses) + if err != nil { + log.Error(ctx, "diagnoseSnapshot: no diagnostics", err, telemetry.Package.Of(ph.ID())) return } // Don't publish empty diagnostics. s.publishReports(ctx, reports, false) - }(snapshot, fh) + }(id) } // Run diagnostics on the go.mod file. s.diagnoseModfile(snapshot) @@ -73,7 +63,7 @@ func (s *Server) diagnoseFile(snapshot source.Snapshot, fh source.FileHandle) { ctx = telemetry.File.With(ctx, fh.Identity().URI) - reports, warningMsg, err := source.Diagnostics(ctx, snapshot, fh, true, snapshot.View().Options().DisabledAnalyses) + reports, warningMsg, err := source.FileDiagnostics(ctx, snapshot, fh, true, snapshot.View().Options().DisabledAnalyses) // Check the warning message first. if warningMsg != "" { s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 0e2e117258..c66c3d6ec3 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -96,7 +96,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti t.Fatal(err) } identity := fh.Identity() - results, _, err := source.Diagnostics(r.ctx, v.Snapshot(), fh, true, nil) + results, _, err := source.FileDiagnostics(r.ctx, v.Snapshot(), fh, true, nil) if err != nil { t.Fatal(err) } @@ -350,7 +350,7 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span) { t.Fatal(err) } fileID := fh.Identity() - diagnostics, _, err := source.Diagnostics(r.ctx, snapshot, fh, true, nil) + diagnostics, _, err := source.FileDiagnostics(r.ctx, snapshot, fh, true, nil) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 3a530f89a3..b52084d797 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -40,10 +40,7 @@ type RelatedInformation struct { Message string } -func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnalysis bool, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) { - ctx, done := trace.StartSpan(ctx, "source.Diagnostics", telemetry.File.Of(fh.Identity().URI)) - defer done() - +func FileDiagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnalysis bool, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) { if fh.Identity().Kind != Go { return nil, "", errors.Errorf("unexpected file type: %q", fh.Identity().URI.Filename) } @@ -63,14 +60,24 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnal log.Error(ctx, "error checking common errors", err, telemetry.File.Of(fh.Identity().URI)) } } + reports, msg, err := PackageDiagnostics(ctx, snapshot, ph, withAnalysis, disabledAnalyses) + if warningMsg == "" { + warningMsg = msg + } + return reports, warningMsg, err +} + +func PackageDiagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, withAnalysis bool, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) { pkg, err := ph.Check(ctx) if err != nil { return nil, "", err } + var warningMsg string // If we have a package with a single file and errors about "undeclared" symbols, // we may have an ad-hoc package with multiple files. Show a warning message. // TODO(golang/go#36416): Remove this when golang.org/cl/202277 is merged. - if warningMsg == "" && len(pkg.CompiledGoFiles()) == 1 && hasUndeclaredErrors(pkg) { + if len(pkg.CompiledGoFiles()) == 1 && hasUndeclaredErrors(pkg) { + fh := pkg.CompiledGoFiles()[0].File() if warningMsg, err = checkCommonErrors(ctx, snapshot.View(), fh.Identity().URI); err != nil { log.Error(ctx, "error checking common errors", err, telemetry.File.Of(fh.Identity().URI)) } @@ -86,9 +93,13 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnal if e.Kind != ListError { continue } - // If no file is associated with the error, default to the current file. + // If no file is associated with the error, pick an open file from the package. if e.File.URI.Filename() == "" { - e.File = fh.Identity() + for _, ph := range pkg.CompiledGoFiles() { + if snapshot.View().Session().IsOpen(ph.File().Identity().URI) { + e.File = ph.File().Identity() + } + } } clearReports(snapshot, reports, e.File) } @@ -98,9 +109,9 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnal if err := analyses(ctx, snapshot, ph, disabledAnalyses, reports); err != nil { // Exit early if the context has been canceled. if err == context.Canceled { - return nil, "", err + return nil, warningMsg, err } - log.Error(ctx, "failed to run analyses", err, telemetry.File.Of(fh.Identity().URI)) + log.Error(ctx, "failed to run analyses", err, telemetry.Package.Of(ph.ID())) } } // Updates to the diagnostics for this package may need to be propagated. diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 4a175e6972..9136dbcf11 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -90,7 +90,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti t.Fatal(err) } fileID := fh.Identity() - results, _, err := source.Diagnostics(r.ctx, snapshot, fh, true, nil) + results, _, err := source.FileDiagnostics(r.ctx, snapshot, fh, true, nil) if err != nil { t.Fatal(err) }