From 189207f339b738deefb62dda45756f5f720e724a Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Mon, 13 Jan 2020 19:43:43 -0500 Subject: [PATCH] internal/lsp: use URIs instead of FileIdentity in errors This change modifies the source.Error type to have a URI instead of a FileIdentity associated with an error. Change-Id: I8056bdc881dd3ec43af02cca1366815c0bc6dfcd Reviewed-on: https://go-review.googlesource.com/c/tools/+/214586 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/errors.go | 7 +---- internal/lsp/diagnostics.go | 11 +------- internal/lsp/source/diagnostics.go | 44 ++++++++++++++++++++---------- internal/lsp/source/view.go | 4 +-- 4 files changed, 34 insertions(+), 32 deletions(-) diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index 9be98092531..56462e1e93d 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -7,7 +7,6 @@ package cache import ( "bytes" "context" - "fmt" "go/scanner" "go/token" "go/types" @@ -105,12 +104,8 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface if err != nil { return nil, err } - ph, err := pkg.File(spn.URI()) - if err != nil { - return nil, fmt.Errorf("finding file for error %q: %v", msg, err) - } return &source.Error{ - File: ph.File().Identity(), + URI: spn.URI(), Range: rng, Message: msg, Kind: kind, diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 6c41056ff96..07f05ee9ba9 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -89,20 +89,11 @@ func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, r s.deliveredMu.Lock() defer s.deliveredMu.Unlock() - for identity, diagnostics := range reports { + for fileID, diagnostics := range reports { // Don't deliver diagnostics if the context has already been canceled. if ctx.Err() != nil { break } - // Rather than using the identity provided in the report, - // get the FileHandle directly through the snapshot. - // This prevents us from using cached file versions. - fh, err := snapshot.GetFile(identity.URI) - if err != nil { - log.Error(ctx, "publishReports: failed to get FileHandle", err, telemetry.File.Of(identity.URI)) - continue - } - fileID := fh.Identity() // Pre-sort diagnostics to avoid extra work when we compare them. source.SortDiagnostics(diagnostics) diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 6929f9b779d..0f27b4381e1 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -94,19 +94,27 @@ func PackageDiagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle continue } // If no file is associated with the error, pick an open file from the package. - if e.File.URI.Filename() == "" { + if e.URI.Filename() == "" { for _, ph := range pkg.CompiledGoFiles() { if snapshot.View().Session().IsOpen(ph.File().Identity().URI) { - e.File = ph.File().Identity() + e.URI = ph.File().Identity().URI } } } - clearReports(snapshot, reports, e.File) + fh, err := snapshot.GetFile(e.URI) + if err != nil { + return nil, warningMsg, err + } + clearReports(snapshot, reports, fh.Identity()) } // Run diagnostics for the package that this URI belongs to. - if !diagnostics(ctx, snapshot, pkg, reports) && withAnalysis { + hadDiagnostics, err := diagnostics(ctx, snapshot, reports, pkg) + if err != nil { + return nil, warningMsg, err + } + if !hadDiagnostics && withAnalysis { // If we don't have any list, parse, or type errors, run analyses. - if err := analyses(ctx, snapshot, ph, disabledAnalyses, reports); err != nil { + if err := analyses(ctx, snapshot, reports, ph, disabledAnalyses); err != nil { // Exit early if the context has been canceled. if err == context.Canceled { return nil, warningMsg, err @@ -131,7 +139,7 @@ func PackageDiagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle for _, fh := range pkg.CompiledGoFiles() { clearReports(snapshot, reports, fh.File().Identity()) } - diagnostics(ctx, snapshot, pkg, reports) + diagnostics(ctx, snapshot, reports, pkg) } return reports, warningMsg, nil } @@ -140,7 +148,7 @@ type diagnosticSet struct { listErrors, parseErrors, typeErrors []*Diagnostic } -func diagnostics(ctx context.Context, snapshot Snapshot, pkg Package, reports map[FileIdentity][]Diagnostic) bool { +func diagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, pkg Package) (bool, error) { ctx, done := trace.StartSpan(ctx, "source.diagnostics", telemetry.Package.Of(pkg.ID())) _ = ctx // circumvent SA4006 defer done() @@ -152,10 +160,14 @@ func diagnostics(ctx context.Context, snapshot Snapshot, pkg Package, reports ma Range: e.Range, Severity: protocol.SeverityError, } - set, ok := diagSets[e.File] + fh, err := snapshot.GetFile(e.URI) + if err != nil { + return false, err + } + set, ok := diagSets[fh.Identity()] if !ok { set = &diagnosticSet{} - diagSets[e.File] = set + diagSets[fh.Identity()] = set } switch e.Kind { case ParseError: @@ -181,12 +193,12 @@ func diagnostics(ctx context.Context, snapshot Snapshot, pkg Package, reports ma if len(diags) > 0 { nonEmptyDiagnostics = true } - addReports(ctx, reports, snapshot, fileID, diags...) + addReports(ctx, snapshot, reports, fileID, diags...) } - return nonEmptyDiagnostics + return nonEmptyDiagnostics, nil } -func analyses(ctx context.Context, snapshot Snapshot, ph PackageHandle, disabledAnalyses map[string]struct{}, reports map[FileIdentity][]Diagnostic) error { +func analyses(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, ph PackageHandle, disabledAnalyses map[string]struct{}) error { var analyzers []*analysis.Analyzer for _, a := range snapshot.View().Options().Analyzers { if _, ok := disabledAnalyses[a.Name]; ok { @@ -209,7 +221,11 @@ func analyses(ctx context.Context, snapshot Snapshot, ph PackageHandle, disabled if onlyDeletions(e.SuggestedFixes) { tags = append(tags, protocol.Unnecessary) } - addReports(ctx, reports, snapshot, e.File, &Diagnostic{ + fh, err := snapshot.GetFile(e.URI) + if err != nil { + return err + } + addReports(ctx, snapshot, reports, fh.Identity(), &Diagnostic{ Range: e.Range, Message: e.Message, Source: e.Category, @@ -229,7 +245,7 @@ func clearReports(snapshot Snapshot, reports map[FileIdentity][]Diagnostic, file reports[fileID] = []Diagnostic{} } -func addReports(ctx context.Context, reports map[FileIdentity][]Diagnostic, snapshot Snapshot, fileID FileIdentity, diagnostics ...*Diagnostic) error { +func addReports(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]Diagnostic, fileID FileIdentity, diagnostics ...*Diagnostic) error { if snapshot.View().Ignore(fileID.URI) { return nil } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 14b7d1380cf..83e659ffef7 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -334,7 +334,7 @@ type Package interface { } type Error struct { - File FileIdentity + URI span.URI Range protocol.Range Kind ErrorKind Message string @@ -354,7 +354,7 @@ const ( ) func (e *Error) Error() string { - return fmt.Sprintf("%s:%s: %s", e.File, e.Range, e.Message) + return fmt.Sprintf("%s:%s: %s", e.URI, e.Range, e.Message) } type BuiltinPackage interface {