From bc1376d635b8cee2c14b9fde9b18ba91c5f631c8 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 12 Nov 2019 16:33:11 -0500 Subject: [PATCH] internal/lsp: look up files in packages by position instead of URI This change makes sure that we only return files that contain the given position. There are a few instances of needing to look up files by URI in the internal/lsp/cache package, so use an unexported package for that. This allows us to remove some code in the implementations code. Change-Id: Ifa7a62c67271826e6c632e4c88667d60f8b760c4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/206880 Run-TryBot: Rebecca Stambler Reviewed-by: Michael Matloob --- internal/lsp/cache/errors.go | 6 +- internal/lsp/cache/pkg.go | 23 ------- internal/lsp/cache/session.go | 2 +- internal/lsp/cache/view.go | 76 +++++++++++++++++++----- internal/lsp/implementation.go | 7 ++- internal/lsp/source/completion_format.go | 11 +--- internal/lsp/source/identifier.go | 10 +--- internal/lsp/source/implementation.go | 35 ++--------- internal/lsp/source/source_test.go | 6 +- internal/lsp/source/util.go | 12 +--- internal/lsp/source/view.go | 2 +- 11 files changed, 83 insertions(+), 107 deletions(-) diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index 984a8f428c..5c377aff43 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -163,7 +163,7 @@ func typeErrorRange(ctx context.Context, fset *token.FileSet, pkg *pkg, pos toke return span.Span{}, err } posn := fset.Position(pos) - ph, _, err := findFileInPackage(ctx, span.FileURI(posn.Filename), pkg) + ph, _, err := findFileInPackage(pkg, span.FileURI(posn.Filename)) if err != nil { return spn, nil // ignore errors } @@ -190,7 +190,7 @@ func typeErrorRange(ctx context.Context, fset *token.FileSet, pkg *pkg, pos toke } func scannerErrorRange(ctx context.Context, fset *token.FileSet, pkg *pkg, posn token.Position) (span.Span, error) { - ph, _, err := findFileInPackage(ctx, span.FileURI(posn.Filename), pkg) + ph, _, err := findFileInPackage(pkg, span.FileURI(posn.Filename)) if err != nil { return span.Span{}, err } @@ -209,7 +209,7 @@ func scannerErrorRange(ctx context.Context, fset *token.FileSet, pkg *pkg, posn // spanToRange converts a span.Span to a protocol.Range, // assuming that the span belongs to the package whose diagnostics are being computed. func spanToRange(ctx context.Context, pkg *pkg, spn span.Span) (protocol.Range, error) { - ph, _, err := findFileInPackage(ctx, spn.URI(), pkg) + ph, _, err := findFileInPackage(pkg, spn.URI()) if err != nil { return protocol.Range{}, err } diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index bf398a2da6..b5281edc8a 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -126,26 +126,3 @@ func (s *snapshot) FindAnalysisError(ctx context.Context, id string, diag protoc } return nil, errors.Errorf("no matching diagnostic for %v", diag) } - -func findFileInPackage(ctx context.Context, uri span.URI, pkg source.Package) (source.ParseGoHandle, source.Package, error) { - queue := []source.Package{pkg} - seen := make(map[string]bool) - - for len(queue) > 0 { - pkg := queue[0] - queue = queue[1:] - seen[pkg.ID()] = true - - for _, ph := range pkg.Files() { - if ph.File().Identity().URI == uri { - return ph, pkg, nil - } - } - for _, dep := range pkg.Imports() { - if !seen[dep.ID()] { - queue = append(queue, dep) - } - } - } - return nil, nil, errors.Errorf("no file for %s in package %s", uri, pkg.ID()) -} diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index e81ee9408f..a7b57a1931 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -141,7 +141,7 @@ func (s *session) createView(ctx context.Context, name string, folder span.URI, m, err := v.snapshot.load(ctx, source.DirectoryURI(folder)) var loadErr error if err != nil && err != errNoPackagesFound { - loadErr = fmt.Errorf("Error loading packages: %v", err) + loadErr = fmt.Errorf("error loading packages: %v", err) } // Prepare CheckPackageHandles for every package that's been loaded. diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 03a10ac363..d872ddddb5 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -301,16 +301,6 @@ func (v *view) Ignore(uri span.URI) bool { return ok } -func (v *view) findIgnoredFile(ctx context.Context, uri span.URI) (source.ParseGoHandle, source.Package, error) { - // Check the builtin package. - for _, h := range v.BuiltinPackage().Files() { - if h.File().Identity().URI == uri { - return h, nil, nil - } - } - return nil, nil, errors.Errorf("no ignored file for %s", uri) -} - func (v *view) BackgroundContext() context.Context { v.mu.Lock() defer v.mu.Unlock() @@ -455,12 +445,68 @@ func (v *view) openFiles(ctx context.Context, uris []span.URI) (results []source return results } -func (v *view) FindFileInPackage(ctx context.Context, uri span.URI, pkg source.Package) (source.ParseGoHandle, source.Package, error) { - // Special case for ignored files. - if v.Ignore(uri) { - return v.findIgnoredFile(ctx, uri) +func (v *view) FindPosInPackage(searchpkg source.Package, pos token.Pos) (*ast.File, *protocol.ColumnMapper, source.Package, error) { + tok := v.session.cache.fset.File(pos) + if tok == nil { + return nil, nil, nil, errors.Errorf("no file for pos in package %s", searchpkg.ID()) } - return findFileInPackage(ctx, uri, pkg) + uri := span.FileURI(tok.Name()) + + // Special case for ignored files. + var ( + ph source.ParseGoHandle + pkg source.Package + err error + ) + if v.Ignore(uri) { + ph, pkg, err = v.findIgnoredFile(uri) + } else { + ph, pkg, err = findFileInPackage(searchpkg, uri) + } + if err != nil { + return nil, nil, nil, err + } + file, m, _, err := ph.Cached() + if err != nil { + return nil, nil, nil, err + } + if !(file.Pos() <= pos && pos <= file.End()) { + return nil, nil, nil, err + } + return file, m, pkg, nil +} + +func (v *view) findIgnoredFile(uri span.URI) (source.ParseGoHandle, source.Package, error) { + // Check the builtin package. + for _, h := range v.BuiltinPackage().Files() { + if h.File().Identity().URI == uri { + return h, nil, nil + } + } + return nil, nil, errors.Errorf("no ignored file for %s", uri) +} + +func findFileInPackage(pkg source.Package, uri span.URI) (source.ParseGoHandle, source.Package, error) { + queue := []source.Package{pkg} + seen := make(map[string]bool) + + for len(queue) > 0 { + pkg := queue[0] + queue = queue[1:] + seen[pkg.ID()] = true + + for _, ph := range pkg.Files() { + if ph.File().Identity().URI == uri { + return ph, pkg, nil + } + } + for _, dep := range pkg.Imports() { + if !seen[dep.ID()] { + queue = append(queue, dep) + } + } + } + return nil, nil, errors.Errorf("no file for %s in package %s", uri, pkg.ID()) } type debugView struct{ *view } diff --git a/internal/lsp/implementation.go b/internal/lsp/implementation.go index dffc449289..daca99ff4e 100644 --- a/internal/lsp/implementation.go +++ b/internal/lsp/implementation.go @@ -19,6 +19,9 @@ func (s *Server) implementation(ctx context.Context, params *protocol.Implementa if err != nil { return nil, err } - - return source.Implementation(ctx, view, f, params.Position) + ident, err := source.Identifier(ctx, view, f, params.Position) + if err != nil { + return nil, err + } + return ident.Implementation(ctx) } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index d0683c3135..c39c342c5f 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -154,20 +154,11 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { if cand.imp != nil && cand.imp.pkg != nil { searchPkg = cand.imp.pkg } - ph, pkg, err := c.view.FindFileInPackage(c.ctx, uri, searchPkg) + file, _, pkg, err := c.view.FindPosInPackage(searchPkg, obj.Pos()) if err != nil { log.Error(c.ctx, "error finding file in package", err, telemetry.URI.Of(uri), telemetry.Package.Of(searchPkg.ID())) return item, nil } - file, _, _, err := ph.Cached() - if err != nil { - log.Error(c.ctx, "no cached file", err, telemetry.URI.Of(uri)) - return item, nil - } - if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) { - log.Error(c.ctx, "no file for object", errors.Errorf("no file for completion object %s", obj.Name()), telemetry.URI.Of(uri)) - return item, nil - } ident, err := findIdentifier(c.ctx, c.snapshot, pkg, file, obj.Pos()) if err != nil { log.Error(c.ctx, "failed to findIdentifier", err, telemetry.URI.Of(uri)) diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 4addaf3732..8cb11a6cc8 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -242,18 +242,10 @@ func hasErrorType(obj types.Object) bool { } func objToNode(ctx context.Context, v View, pkg Package, obj types.Object) (ast.Decl, error) { - uri := span.FileURI(v.Session().Cache().FileSet().Position(obj.Pos()).Filename) - ph, _, err := v.FindFileInPackage(ctx, uri, pkg) + declAST, _, _, err := v.FindPosInPackage(pkg, obj.Pos()) if err != nil { return nil, err } - declAST, _, _, err := ph.Cached() - if declAST == nil { - return nil, err - } - if !(declAST.Pos() <= obj.Pos() && obj.Pos() <= declAST.End()) { - return nil, errors.Errorf("no file for %s", obj.Name()) - } path, _ := astutil.PathEnclosingInterval(declAST, obj.Pos(), obj.Pos()) if path == nil { return nil, errors.Errorf("no path for object %v", obj.Name()) diff --git a/internal/lsp/source/implementation.go b/internal/lsp/source/implementation.go index f82f646acc..75208c8ce6 100644 --- a/internal/lsp/source/implementation.go +++ b/internal/lsp/source/implementation.go @@ -19,14 +19,8 @@ import ( "golang.org/x/tools/internal/lsp/protocol" ) -func Implementation(ctx context.Context, view View, f File, position protocol.Position) ([]protocol.Location, error) { - // Find all references to the identifier at the position. - ident, err := Identifier(ctx, view, f, position) - if err != nil { - return nil, err - } - - res, err := ident.implementations(ctx) +func (i *IdentifierInfo) Implementation(ctx context.Context) ([]protocol.Location, error) { + res, err := i.implementations(ctx) if err != nil { return nil, err } @@ -71,35 +65,14 @@ func Implementation(ctx context.Context, view View, f File, position protocol.Po if pkgs[obj] == nil || len(pkg.Files()) == 0 { continue } - // Search for the identifier in each of the package's files. - var ident *IdentifierInfo - - fset := view.Session().Cache().FileSet() - file := fset.File(obj.Pos()) - var containingFile FileHandle - for _, f := range pkg.Files() { - if f.File().Identity().URI.Filename() == file.Name() { - containingFile = f.File() - } - } - if containingFile == nil { - return nil, fmt.Errorf("failed to find file %q in package %v", file.Name(), pkg.PkgPath()) - } - - uri := containingFile.Identity().URI - ph, _, err := view.FindFileInPackage(ctx, uri, pkgs[obj]) + file, _, _, err := i.Snapshot.View().FindPosInPackage(pkgs[obj], obj.Pos()) if err != nil { return nil, err } - astFile, _, _, err := ph.Cached() + ident, err := findIdentifier(ctx, i.Snapshot, pkg, file, obj.Pos()) if err != nil { return nil, err } - ident, err = findIdentifier(ctx, view.Snapshot(), pkg, astFile, obj.Pos()) - if err != nil { - return nil, err - } - decRange, err := ident.Declaration.Range() if err != nil { return nil, err diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 4281635c77..4403d06dea 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -554,8 +554,12 @@ func (r *runner) Implementation(t *testing.T, spn span.Span, m tests.Implementat if err != nil { t.Fatalf("failed for %v: %v", m.Src, err) } + ident, err := source.Identifier(ctx, r.view, f, loc.Range.Start) + if err != nil { + t.Fatalf("failed for %v: %v", m.Src, err) + } var locs []protocol.Location - locs, err = source.Implementation(r.ctx, r.view, f, loc.Range.Start) + locs, err = ident.Implementation(r.ctx) if err != nil { t.Fatalf("failed for %v: %v", m.Src, err) } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index e056a6ef50..4e7138592b 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -155,7 +155,7 @@ func nodeToMappedRange(ctx context.Context, view View, m *protocol.ColumnMapper, } func posToMappedRange(ctx context.Context, v View, pkg Package, pos, end token.Pos) (mappedRange, error) { - m, err := posToMapper(ctx, v, pkg, pos) + _, m, _, err := v.FindPosInPackage(pkg, pos) if err != nil { return mappedRange{}, err } @@ -175,16 +175,6 @@ func posToRange(ctx context.Context, view View, m *protocol.ColumnMapper, pos, e }, nil } -func posToMapper(ctx context.Context, v View, pkg Package, pos token.Pos) (*protocol.ColumnMapper, error) { - posn := v.Session().Cache().FileSet().Position(pos) - ph, _, err := v.FindFileInPackage(ctx, span.FileURI(posn.Filename), pkg) - if err != nil { - return nil, err - } - _, m, _, err := ph.Cached() - return m, err -} - // Matches cgo generated comment as well as the proposed standard: // https://golang.org/s/generatedcode var generatedRx = regexp.MustCompile(`// .*DO NOT EDIT\.?`) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 568151afe7..934743a328 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -260,7 +260,7 @@ type View interface { // FindFileInPackage returns the AST and type information for a file that may // belong to or be part of a dependency of the given package. - FindFileInPackage(ctx context.Context, uri span.URI, pkg Package) (ParseGoHandle, Package, error) + FindPosInPackage(pkg Package, pos token.Pos) (*ast.File, *protocol.ColumnMapper, Package, error) // Snapshot returns the current snapshot for the view. Snapshot() Snapshot