From f07d81a593a5888a079a2082e0c4fb9d28b98ded Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 6 Aug 2019 18:51:17 -0400 Subject: [PATCH] internal/lsp: fix documentation for completion items This change fixes documentation for completion items by using cached package and AST information to derive the documentation. We also add testing for documentation in completion items. Change-Id: I911fb80f5cef88640fc06a9fe474e5da403657e3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/189237 Run-TryBot: Rebecca Stambler Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 1 + internal/lsp/cache/gofile.go | 41 ++++++++++++++++--- internal/lsp/cache/parse.go | 9 ++++ internal/lsp/cache/pkg.go | 12 +++--- internal/lsp/lsp_test.go | 12 +++++- internal/lsp/source/completion_format.go | 22 ++++++---- internal/lsp/source/diagnostics.go | 8 ++-- internal/lsp/source/identifier.go | 33 ++++++++++----- internal/lsp/source/source_test.go | 8 +++- internal/lsp/source/view.go | 14 +++++-- internal/lsp/testdata/bar/bar.go.in | 3 +- .../testdata/deepcomplete/deep_complete.go | 6 +-- internal/lsp/tests/tests.go | 19 ++++++--- internal/lsp/text_synchronization.go | 6 +-- 14 files changed, 142 insertions(+), 52 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 3bc6f05ad2..16a05934eb 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -102,6 +102,7 @@ func (imp *importer) typeCheck(ctx context.Context, id packageID) (*pkg, error) return nil, errors.Errorf("no metadata for %v", id) } pkg := &pkg{ + view: imp.view, id: meta.id, pkgPath: meta.pkgPath, imports: make(map[packagePath]*pkg), diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index 0aae9c9ef7..f8948c16b1 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -63,6 +63,7 @@ func (f *goFile) GetToken(ctx context.Context) (*token.File, error) { func (f *goFile) GetAST(ctx context.Context, mode source.ParseMode) (*ast.File, error) { f.view.mu.Lock() defer f.view.mu.Unlock() + ctx = telemetry.File.With(ctx, f.URI()) if f.isDirty(ctx) || f.wrongParseMode(ctx, mode) { @@ -70,8 +71,17 @@ func (f *goFile) GetAST(ctx context.Context, mode source.ParseMode) (*ast.File, return nil, errors.Errorf("GetAST: unable to check package for %s: %v", f.URI(), err) } } - fh := f.Handle(ctx) // Check for a cached AST first, in case getting a trimmed version would actually cause a re-parse. + fh := f.Handle(ctx) + cached, err := f.view.session.cache.cachedAST(fh, mode) + if cached != nil || err != nil { + return cached, err + } + ph := f.view.session.cache.ParseGoHandle(fh, mode) + return ph.Parse(ctx) +} + +func (cache *cache) cachedAST(fh source.FileHandle, mode source.ParseMode) (*ast.File, error) { for _, m := range []source.ParseMode{ source.ParseHeader, source.ParseExported, @@ -80,15 +90,14 @@ func (f *goFile) GetAST(ctx context.Context, mode source.ParseMode) (*ast.File, if m < mode { continue } - if v, ok := f.view.session.cache.store.Cached(parseKey{ + if v, ok := cache.store.Cached(parseKey{ file: fh.Identity(), mode: m, }).(*parseGoData); ok { return v.ast, v.err } } - ph := f.view.session.cache.ParseGoHandle(fh, mode) - return ph.Parse(ctx) + return nil, nil } func (f *goFile) GetPackages(ctx context.Context) []source.Package { @@ -126,13 +135,35 @@ func (f *goFile) GetPackage(ctx context.Context) source.Package { // This solves the problem of test variants, // as the test will have more files than the non-test package. for _, pkg := range pkgs { - if result == nil || len(pkg.GetFilenames()) < len(result.GetFilenames()) { + if result == nil || len(pkg.GetHandles()) < len(result.GetHandles()) { result = pkg } } return result } +func (f *goFile) GetCachedPackage(ctx context.Context) (source.Package, error) { + f.view.mu.Lock() + defer f.view.mu.Unlock() + + f.mu.Lock() + defer f.mu.Unlock() + + var result source.Package + // Pick the "narrowest" package, i.e. the package with the fewest number of files. + // This solves the problem of test variants, + // as the test will have more files than the non-test package. + for _, pkg := range f.pkgs { + if result == nil || len(pkg.GetHandles()) < len(result.GetHandles()) { + result = pkg + } + } + if result == nil { + return nil, errors.Errorf("no cached package for %s", f.URI()) + } + return result, nil +} + func (f *goFile) wrongParseMode(ctx context.Context, mode source.ParseMode) bool { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index b842f8452c..6f0f3edfd8 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -74,6 +74,15 @@ func (h *parseGoHandle) Parse(ctx context.Context) (*ast.File, error) { return data.ast, data.err } +func (h *parseGoHandle) Cached(ctx context.Context) (*ast.File, error) { + v := h.handle.Cached() + if v == nil { + return nil, errors.Errorf("no cached value for %s", h.file.Identity().URI) + } + data := v.(*parseGoData) + return data.ast, data.err +} + func parseGo(ctx context.Context, c *cache, fh source.FileHandle, mode source.ParseMode) (*ast.File, error) { ctx, done := trace.StartSpan(ctx, "cache.parseGo", telemetry.File.Of(fh.Identity().URI.Filename())) defer done() diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index c261079d40..6d357c6221 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -18,6 +18,8 @@ import ( // pkg contains the type information needed by the source package. type pkg struct { + view *view + // ID and package path have their own types to avoid being used interchangeably. id packageID pkgPath packagePath @@ -147,18 +149,14 @@ func (pkg *pkg) PkgPath() string { return string(pkg.pkgPath) } -func (pkg *pkg) GetFilenames() []string { - filenames := make([]string, 0, len(pkg.files)) - for _, ph := range pkg.files { - filenames = append(filenames, ph.File().Identity().URI.Filename()) - } - return filenames +func (pkg *pkg) GetHandles() []source.ParseGoHandle { + return pkg.files } func (pkg *pkg) GetSyntax(ctx context.Context) []*ast.File { var syntax []*ast.File for _, ph := range pkg.files { - file, _ := ph.Parse(ctx) + file, _ := ph.Cached(ctx) if file != nil { syntax = append(syntax, file) } diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index a1588d46fb..df629f6ace 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -162,7 +162,12 @@ func summarizeDiagnostics(i int, want []source.Diagnostic, got []source.Diagnost } func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests.CompletionSnippets, items tests.CompletionItems) { - defer func() { r.server.useDeepCompletions = false }() + defer func() { + r.server.useDeepCompletions = false + r.server.wantCompletionDocumentation = false + }() + + r.server.wantCompletionDocumentation = true for src, itemList := range data { var want []source.CompletionItem @@ -279,6 +284,11 @@ func diffCompletionItems(t *testing.T, spn span.Span, want []source.CompletionIt if w.Detail != g.Detail { return summarizeCompletionItems(i, want, got, "incorrect Detail got %v want %v", g.Detail, w.Detail) } + if w.Documentation != "" && !strings.HasPrefix(w.Documentation, "@") { + if w.Documentation != g.Documentation { + return summarizeCompletionItems(i, want, got, "incorrect Documentation got %v want %v", g.Documentation, w.Documentation) + } + } if wkind := toProtocolCompletionItemKind(w.Kind); wkind != g.Kind { return summarizeCompletionItems(i, want, got, "incorrect Kind got %v want %v", g.Kind, wkind) } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index a07d8091cb..b724c77585 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -96,36 +96,44 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { plainSnippet: plainSnippet, placeholderSnippet: placeholderSnippet, } + // TODO(rstambler): Log errors when this feature is enabled. if c.opts.WantDocumentaton { declRange, err := objToRange(c.ctx, c.view.Session().Cache().FileSet(), obj) if err != nil { - log.Error(c.ctx, "failed to get declaration range for object", err, tag.Of("Name", obj.Name())) goto Return } pos := declRange.FileSet.Position(declRange.Start) if !pos.IsValid() { - log.Error(c.ctx, "invalid declaration position", err, tag.Of("Label", item.Label)) goto Return } uri := span.FileURI(pos.Filename) f, err := c.view.GetFile(c.ctx, uri) if err != nil { - log.Error(c.ctx, "unable to get file", err, tag.Of("URI", uri)) goto Return } gof, ok := f.(GoFile) if !ok { - log.Error(c.ctx, "declaration in a Go file", err, tag.Of("Label", item.Label)) goto Return } - ident, err := Identifier(c.ctx, gof, declRange.Start) + pkg, err := gof.GetCachedPackage(c.ctx) + if err != nil { + goto Return + } + var file *ast.File + for _, ph := range pkg.GetHandles() { + if ph.File().Identity().URI == gof.URI() { + file, _ = ph.Cached(c.ctx) + } + } + if file == nil { + goto Return + } + ident, err := findIdentifier(c.ctx, gof, pkg, file, declRange.Start) if err != nil { - log.Error(c.ctx, "no identifier", err, tag.Of("Name", obj.Name())) goto Return } documentation, err := ident.Documentation(c.ctx, SynopsisDocumentation) if err != nil { - log.Error(c.ctx, "no documentation", err, tag.Of("Name", obj.Name())) goto Return } item.Documentation = documentation diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 51cc41dbcc..0d9bf9d15f 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -70,8 +70,8 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ } // Prepare the reports we will send for the files in this package. reports := make(map[span.URI][]Diagnostic) - for _, filename := range pkg.GetFilenames() { - clearReports(view, reports, span.FileURI(filename)) + for _, fh := range pkg.GetHandles() { + clearReports(view, reports, fh.File().Identity().URI) } // Prepare any additional reports for the errors in this package. @@ -96,8 +96,8 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ if pkg == nil { continue } - for _, filename := range pkg.GetFilenames() { - clearReports(view, reports, span.FileURI(filename)) + for _, fh := range pkg.GetHandles() { + clearReports(view, reports, fh.File().Identity().URI) } diagnostics(ctx, view, pkg, reports) } diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 42bb93bb75..0df7cf6fa2 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -48,14 +48,22 @@ func (i *IdentifierInfo) DeclarationRange() span.Range { // Identifier returns identifier information for a position // in a file, accounting for a potentially incomplete selector. func Identifier(ctx context.Context, f GoFile, pos token.Pos) (*IdentifierInfo, error) { - file, err := f.GetAST(ctx, ParseFull) - if file == nil { - return nil, err - } pkg := f.GetPackage(ctx) if pkg == nil || pkg.IsIllTyped() { return nil, errors.Errorf("pkg for %s is ill-typed", f.URI()) } + var ( + file *ast.File + err error + ) + for _, ph := range pkg.GetHandles() { + if ph.File().Identity().URI == f.URI() { + file, err = ph.Cached(ctx) + } + } + if file == nil { + return nil, err + } return findIdentifier(ctx, f, pkg, file, pos) } @@ -68,7 +76,7 @@ func findIdentifier(ctx context.Context, f GoFile, pkg Package, file *ast.File, // requesting a completion), use the path to the preceding node. result, err := identifier(ctx, f, pkg, file, pos-1) if result == nil && err == nil { - err = errors.Errorf("no identifier found") + err = errors.Errorf("no identifier found for %s", f.FileSet().Position(pos)) } return result, err } @@ -238,13 +246,16 @@ func objToNode(ctx context.Context, view View, originPkg *types.Package, obj typ if !ok { return nil, errors.Errorf("%s is not a Go file", s.URI()) } - // If the object is exported from a different package, - // we don't need its full AST to find the definition. - mode := ParseFull - if obj.Exported() && obj.Pkg() != originPkg { - mode = ParseExported + declPkg, err := declFile.GetCachedPackage(ctx) + if err != nil { + return nil, err + } + var declAST *ast.File + for _, ph := range declPkg.GetHandles() { + if ph.File().Identity().URI == f.URI() { + declAST, err = ph.Cached(ctx) + } } - declAST, err := declFile.GetAST(ctx, mode) if declAST == nil { return nil, err } diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 8da36c9852..b6ce7d5b4d 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -156,7 +156,8 @@ func (r *runner) Completion(t *testing.T, data tests.Completions, snippets tests } pos := tok.Pos(src.Start().Offset()) list, surrounding, err := source.Completion(ctx, r.view, f.(source.GoFile), pos, source.CompletionOptions{ - DeepComplete: strings.Contains(string(src.URI()), "deepcomplete"), + DeepComplete: strings.Contains(string(src.URI()), "deepcomplete"), + WantDocumentaton: true, }) if err != nil { t.Fatalf("failed for %v: %v", src, err) @@ -273,6 +274,11 @@ func diffCompletionItems(t *testing.T, spn span.Span, want []source.CompletionIt if w.Detail != g.Detail { return summarizeCompletionItems(i, want, got, "incorrect Detail got %v want %v", g.Detail, w.Detail) } + if w.Documentation != "" && !strings.HasPrefix(w.Documentation, "@") { + if w.Documentation != g.Documentation { + return summarizeCompletionItems(i, want, got, "incorrect Documentation got %v want %v", g.Documentation, w.Documentation) + } + } if w.Kind != g.Kind { return summarizeCompletionItems(i, want, got, "incorrect Kind got %v want %v", g.Kind, w.Kind) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 826921e8fd..c66f104329 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -78,6 +78,9 @@ type ParseGoHandle interface { // Parse returns the parsed AST for the file. // If the file is not available, returns nil and an error. Parse(ctx context.Context) (*ast.File, error) + + // Cached returns the AST for this handle, if it has already been stored. + Cached(ctx context.Context) (*ast.File, error) } // ParseMode controls the content of the AST produced when parsing a source file. @@ -118,10 +121,10 @@ type Cache interface { FileSet() *token.FileSet // Token returns a TokenHandle for the given file handle. - TokenHandle(FileHandle) TokenHandle + TokenHandle(fh FileHandle) TokenHandle // ParseGo returns a ParseGoHandle for the given file handle. - ParseGoHandle(FileHandle, ParseMode) ParseGoHandle + ParseGoHandle(fh FileHandle, mode ParseMode) ParseGoHandle } // Session represents a single connection from a client. @@ -228,9 +231,12 @@ type File interface { type GoFile interface { File - // GetAST returns the full AST for the file. + // GetAST returns the AST for the file, at or above the given mode. GetAST(ctx context.Context, mode ParseMode) (*ast.File, error) + // GetCachedPackage returns the cached package for the file, if any. + GetCachedPackage(ctx context.Context) (Package, error) + // GetPackage returns the package that this file belongs to. GetPackage(ctx context.Context) Package @@ -255,7 +261,7 @@ type SumFile interface { type Package interface { ID() string PkgPath() string - GetFilenames() []string + GetHandles() []ParseGoHandle GetSyntax(context.Context) []*ast.File GetErrors() []packages.Error GetTypes() *types.Package diff --git a/internal/lsp/testdata/bar/bar.go.in b/internal/lsp/testdata/bar/bar.go.in index c53facf03e..67df955646 100644 --- a/internal/lsp/testdata/bar/bar.go.in +++ b/internal/lsp/testdata/bar/bar.go.in @@ -13,7 +13,8 @@ func _() { _ = foo.StructFoo{} //@complete("S", Foo, IntFoo, StructFoo) } -func Bar() { //@item(Bar, "Bar", "func()", "func") +// Bar is a function. +func Bar() { //@item(Bar, "Bar", "func()", "func", "Bar is a function.") foo.Foo() //@complete("F", Foo, IntFoo, StructFoo) var _ foo.IntFoo //@complete("I", Foo, IntFoo, StructFoo) foo.() //@complete("(", Foo, IntFoo, StructFoo) diff --git a/internal/lsp/testdata/deepcomplete/deep_complete.go b/internal/lsp/testdata/deepcomplete/deep_complete.go index 970ec65788..bfde660554 100644 --- a/internal/lsp/testdata/deepcomplete/deep_complete.go +++ b/internal/lsp/testdata/deepcomplete/deep_complete.go @@ -26,9 +26,9 @@ func _() { func wantsContext(context.Context) {} func _() { - context.Background() //@item(ctxBackground, "context.Background", "func() context.Context", "func") - context.TODO() //@item(ctxTODO, "context.TODO", "func() context.Context", "func") - /* context.WithValue(parent context.Context, key interface{}, val interface{}) */ //@item(ctxWithValue, "context.WithValue", "func(parent context.Context, key interface{}, val interface{}) context.Context", "func") + context.Background() //@item(ctxBackground, "context.Background", "func() context.Context", "func", "Background returns a non-nil, empty Context.") + context.TODO() //@item(ctxTODO, "context.TODO", "func() context.Context", "func", "TODO returns a non-nil, empty Context.") + context.WithValue(nil, nil, nil) //@item(ctxWithValue, "context.WithValue", "func(parent context.Context, key interface{}, val interface{}) context.Context", "func", "WithValue returns a copy of parent in which the value associated with key is val.") wantsContext(c) //@complete(")", ctxBackground, ctxTODO, ctxWithValue, ctxPackage) } diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 73063c833f..88fb2c36ef 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -208,7 +208,7 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data { // Do a first pass to collect special markers for completion. if err := data.Exported.Expect(map[string]interface{}{ - "item": func(name string, r packagestest.Range, _, _ string) { + "item": func(name string, r packagestest.Range, _ []string) { data.Exported.Mark(name, r) }, }); err != nil { @@ -437,11 +437,20 @@ func (data *Data) collectCompletions(src span.Span, expected []token.Pos) { data.Completions[src] = expected } -func (data *Data) collectCompletionItems(pos token.Pos, label, detail, kind string) { +func (data *Data) collectCompletionItems(pos token.Pos, args []string) { + if len(args) < 3 { + return + } + label, detail, kind := args[0], args[1], args[2] + var documentation string + if len(args) == 4 { + documentation = args[3] + } data.CompletionItems[pos] = &source.CompletionItem{ - Label: label, - Detail: detail, - Kind: source.ParseCompletionItemKind(kind), + Label: label, + Detail: detail, + Kind: source.ParseCompletionItemKind(kind), + Documentation: documentation, } } diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 68abec0eb9..14a823b150 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -169,13 +169,13 @@ func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocu log.Error(ctx, "no package available", nil, telemetry.File) return nil } - for _, filename := range pkg.GetFilenames() { + for _, ph := range pkg.GetHandles() { // If other files from this package are open, don't clear. - if s.session.IsOpen(span.NewURI(filename)) { + if s.session.IsOpen(ph.File().Identity().URI) { clear = nil return nil } - clear = append(clear, span.FileURI(filename)) + clear = append(clear, ph.File().Identity().URI) } return nil }