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 <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

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),

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()

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()

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)
 		}

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)
 		}

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

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)
 	}

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
 	}

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)
 		}

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

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)

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)
 }

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,
 	}
 }
 

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
 }