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 }