From 660eba4da30b912b1dd4b8279a97ff2a6ea1da25 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Wed, 4 Dec 2019 13:45:53 -0500 Subject: [PATCH] internal/lsp/source: extract helper, improve error messages Lack of context in error messages is making my life difficult. Add context to a few, refactoring out some duplicate code along the way. Change-Id: I3a940b12ec7c82b1ae1fc477694a2b8b45f6ff71 Reviewed-on: https://go-review.googlesource.com/c/tools/+/209860 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/errors.go | 3 +- internal/lsp/source/completion.go | 18 ++---------- internal/lsp/source/completion_format.go | 2 +- internal/lsp/source/format.go | 36 ++++-------------------- internal/lsp/source/highlight.go | 18 ++---------- internal/lsp/source/identifier.go | 18 ++---------- internal/lsp/source/rename.go | 3 +- internal/lsp/source/signature_help.go | 18 ++---------- internal/lsp/source/symbols.go | 17 ++--------- internal/lsp/source/util.go | 20 +++++++++++++ 10 files changed, 45 insertions(+), 108 deletions(-) diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index 6fdee56f26..9afdf6de35 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -3,6 +3,7 @@ package cache import ( "bytes" "context" + "fmt" "go/scanner" "go/token" "go/types" @@ -89,7 +90,7 @@ func sourceError(ctx context.Context, fset *token.FileSet, pkg *pkg, e interface } ph, err := pkg.File(spn.URI()) if err != nil { - return nil, err + return nil, fmt.Errorf("finding file for error %q: %v", msg, err) } return &source.Error{ File: ph.File().Identity(), diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index b06c6346a9..52d0bbefa5 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -6,6 +6,7 @@ package source import ( "context" + "fmt" "go/ast" "go/constant" "go/token" @@ -394,22 +395,9 @@ func Completion(ctx context.Context, snapshot Snapshot, f File, pos protocol.Pos startTime := time.Now() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, NarrowestCheckPackageHandle) if err != nil { - return nil, nil, err - } - ph, err := NarrowestCheckPackageHandle(phs) - if err != nil { - return nil, nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, nil, err - } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("getting file for Completion: %v", err) } file, m, _, err := pgh.Cached() if err != nil { diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index b637e41fe4..7f965beedb 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -194,7 +194,7 @@ func (c *completer) importEdits(imp *importInfo) ([]protocol.TextEdit, error) { } } if ph == nil { - return nil, errors.Errorf("no ParseGoHandle for %s", c.filename) + return nil, errors.Errorf("building import completion for %v: no ParseGoHandle for %s", imp.importPath, c.filename) } return computeOneImportFixEdits(c.ctx, c.snapshot.View(), ph, &imports.ImportFix{ diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index 797b09368b..0e49f9a9ed 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -8,6 +8,7 @@ package source import ( "bytes" "context" + "fmt" "go/ast" "go/format" "go/parser" @@ -27,23 +28,11 @@ func Format(ctx context.Context, snapshot Snapshot, f File) ([]protocol.TextEdit ctx, done := trace.StartSpan(ctx, "source.Format") defer done() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, NarrowestCheckPackageHandle) if err != nil { - return nil, err - } - ph, err := NarrowestCheckPackageHandle(phs) - if err != nil { - return nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, err - } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, err + return nil, fmt.Errorf("getting file for Format: %v", err) } + // Be extra careful that the file's ParseMode is correct, // otherwise we might replace the user's code with a trimmed AST. if pgh.Mode() != ParseFull { @@ -102,26 +91,13 @@ func AllImportsFixes(ctx context.Context, snapshot Snapshot, f File) (allFixEdit ctx, done := trace.StartSpan(ctx, "source.AllImportsFixes") defer done() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, NarrowestCheckPackageHandle) if err != nil { - return nil, nil, err - } - ph, err := NarrowestCheckPackageHandle(phs) - if err != nil { - return nil, nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("getting file for AllImportsFixes: %v", err) } if hasListErrors(pkg) { return nil, nil, errors.Errorf("%s has list errors, not running goimports", f.URI()) } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, nil, err - } options := &imports.Options{ // Defaults. AllErrors: true, diff --git a/internal/lsp/source/highlight.go b/internal/lsp/source/highlight.go index 849bc8063b..d8a72bdac9 100644 --- a/internal/lsp/source/highlight.go +++ b/internal/lsp/source/highlight.go @@ -6,6 +6,7 @@ package source import ( "context" + "fmt" "go/ast" "go/token" @@ -20,22 +21,9 @@ func Highlight(ctx context.Context, snapshot Snapshot, f File, pos protocol.Posi ctx, done := trace.StartSpan(ctx, "source.Highlight") defer done() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, WidestCheckPackageHandle) if err != nil { - return nil, err - } - ph, err := WidestCheckPackageHandle(phs) - if err != nil { - return nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, err - } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, err + return nil, fmt.Errorf("getting file for Highlight: %v", err) } file, m, _, err := pgh.Parse(ctx) if err != nil { diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index dc91d05ea5..a0720c07dc 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -6,6 +6,7 @@ package source import ( "context" + "fmt" "go/ast" "go/token" "go/types" @@ -50,22 +51,9 @@ func Identifier(ctx context.Context, snapshot Snapshot, f File, pos protocol.Pos ctx, done := trace.StartSpan(ctx, "source.Identifier") defer done() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, WidestCheckPackageHandle) if err != nil { - return nil, err - } - ph, err := WidestCheckPackageHandle(phs) - if err != nil { - return nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, err - } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, err + return nil, fmt.Errorf("getting file for Identifier: %v", err) } file, m, _, err := pgh.Cached() if err != nil { diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index 1df62d001b..29a7d8c43a 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -7,6 +7,7 @@ package source import ( "bytes" "context" + "fmt" "go/ast" "go/format" "go/token" @@ -182,7 +183,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, newName string) (map[span.U func (i *IdentifierInfo) getPkgName(ctx context.Context) (*IdentifierInfo, error) { ph, err := i.pkg.File(i.URI()) if err != nil { - return nil, err + return nil, fmt.Errorf("finding file for identifier %v: %v", i.Name, err) } file, _, _, err := ph.Cached() if err != nil { diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 471d1e53f3..c8199562c7 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -6,6 +6,7 @@ package source import ( "context" + "fmt" "go/ast" "go/doc" "go/token" @@ -31,22 +32,9 @@ func SignatureHelp(ctx context.Context, snapshot Snapshot, f File, pos protocol. ctx, done := trace.StartSpan(ctx, "source.SignatureHelp") defer done() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, NarrowestCheckPackageHandle) if err != nil { - return nil, err - } - ph, err := NarrowestCheckPackageHandle(phs) - if err != nil { - return nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, err - } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, err + return nil, fmt.Errorf("getting file for SignatureHelp: %v", err) } file, m, _, err := pgh.Cached() if err != nil { diff --git a/internal/lsp/source/symbols.go b/internal/lsp/source/symbols.go index 0124b6fff5..8acbdd4d46 100644 --- a/internal/lsp/source/symbols.go +++ b/internal/lsp/source/symbols.go @@ -18,22 +18,9 @@ func DocumentSymbols(ctx context.Context, snapshot Snapshot, f File) ([]protocol ctx, done := trace.StartSpan(ctx, "source.DocumentSymbols") defer done() - fh := snapshot.Handle(ctx, f) - phs, err := snapshot.PackageHandles(ctx, fh) + pkg, pgh, err := getParsedFile(ctx, snapshot, f, NarrowestCheckPackageHandle) if err != nil { - return nil, err - } - ph, err := NarrowestCheckPackageHandle(phs) - if err != nil { - return nil, err - } - pkg, err := ph.Check(ctx) - if err != nil { - return nil, err - } - pgh, err := pkg.File(f.URI()) - if err != nil { - return nil, err + return nil, fmt.Errorf("getting file for DocumentSymbols: %v", err) } file, m, _, err := pgh.Cached() if err != nil { diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 16c1960b93..191e529da5 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -65,6 +65,26 @@ func (s mappedRange) URI() span.URI { return s.m.URI } +// getParsedFile is a convenience function that extracts the Package and ParseGoHandle for a File in a Snapshot. +// selectPackage is typically Narrowest/WidestCheckPackageHandle below. +func getParsedFile(ctx context.Context, snapshot Snapshot, f File, selectPackage func([]PackageHandle) (PackageHandle, error)) (Package, ParseGoHandle, error) { + fh := snapshot.Handle(ctx, f) + phs, err := snapshot.PackageHandles(ctx, fh) + if err != nil { + return nil, nil, err + } + ph, err := selectPackage(phs) + if err != nil { + return nil, nil, err + } + pkg, err := ph.Check(ctx) + if err != nil { + return nil, nil, err + } + pgh, err := pkg.File(f.URI()) + return pkg, pgh, err +} + // NarrowestCheckPackageHandle picks the "narrowest" package for a given file. // // By "narrowest" package, we mean the package with the fewest number of files