gopls/internal/golang: use correct imports in HTML pkg doc links

This CL fixes two bugs in the rendering of Doc Links in doc
comments in the web-based "Browse package documentation" feature:

1. Doc links were resolved based on an arbitrary file in
   the package, even though different files have different import
   mappings. Now, the correct file's mapping is used

2. In the presence of a renaming import, doc links often use the
   original package name, not the local package name.
   Now, the local name is preferred, but if none is found the
   original name is used.

+ tests of both fixes.

(This CL was an attempt to get some value out of a fruitless
attempt to fix golang/go#61677 that turned out to be more complex
than expected. The not-yet-common logic has moved out of
PackageDocHTML into comments.go.)

Updates golang/go#61677

Change-Id: Iebd1b11786759fbfad40c8c3b0107c80fb90ebf7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/623637
LUCI-TryBot-Result: Go LUCI <[email protected]>
Commit-Queue: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

Author: adonovan

gopls/internal/golang/comment.go

@@ -18,26 +18,29 @@ import (
 	"golang.org/x/tools/gopls/internal/cache/parsego"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/settings"
+	"golang.org/x/tools/gopls/internal/util/astutil"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 )
 
 var errNoCommentReference = errors.New("no comment reference found")
 
-// CommentToMarkdown converts comment text to formatted markdown.
-// The comment was prepared by DocReader,
-// so it is known not to have leading, trailing blank lines
-// nor to have trailing spaces at the end of lines.
-// The comment markers have already been removed.
-func CommentToMarkdown(text string, options *settings.Options) string {
-	var p comment.Parser
-	doc := p.Parse(text)
-	var pr comment.Printer
+// DocCommentToMarkdown converts the text of a [doc comment] to Markdown.
+//
+// TODO(adonovan): provide a package (or file imports) as context for
+// proper rendering of doc links; see [newDocCommentParser] and golang/go#61677.
+//
+// [doc comment]: https://go.dev/doc/comment
+func DocCommentToMarkdown(text string, options *settings.Options) string {
+	var parser comment.Parser
+	doc := parser.Parse(text)
+
+	var printer comment.Printer
 	// The default produces {#Hdr-...} tags for headings.
 	// vscode displays thems, which is undesirable.
 	// The godoc for comment.Printer says the tags
 	// avoid a security problem.
-	pr.HeadingID = func(*comment.Heading) string { return "" }
-	pr.DocLinkURL = func(link *comment.DocLink) string {
+	printer.HeadingID = func(*comment.Heading) string { return "" }
+	printer.DocLinkURL = func(link *comment.DocLink) string {
 		msg := fmt.Sprintf("https://%s/%s", options.LinkTarget, link.ImportPath)
 		if link.Name != "" {
 			msg += "#"
@@ -48,8 +51,8 @@ func CommentToMarkdown(text string, options *settings.Options) string {
 		}
 		return msg
 	}
-	easy := pr.Markdown(doc)
-	return string(easy)
+
+	return string(printer.Markdown(doc))
 }
 
 // docLinkDefinition finds the definition of the doc link in comments at pos.
@@ -199,3 +202,70 @@ func lookupDocLinkSymbol(pkg *cache.Package, pgf *parsego.File, name string) typ
 	// package-level symbol
 	return scope.Lookup(name)
 }
+
+// newDocCommentParser returns a function that parses [doc comments],
+// with context for Doc Links supplied by the specified package.
+//
+// Imported symbols are rendered using the import mapping for the file
+// that encloses fileNode.
+//
+// The resulting function is not concurrency safe.
+//
+// See issue #61677 for how this might be generalized to support
+// correct contextual parsing of doc comments in Hover too.
+//
+// [doc comment]: https://go.dev/doc/comment
+func newDocCommentParser(pkg *cache.Package) func(fileNode ast.Node, text string) *comment.Doc {
+	var currentFileNode ast.Node // node whose enclosing file's import mapping should be used
+	parser := &comment.Parser{
+		LookupPackage: func(name string) (importPath string, ok bool) {
+			for _, f := range pkg.Syntax() {
+				// Different files in the same package have
+				// different import mappings. Use the provided
+				// syntax node to find the correct file.
+				if astutil.NodeContains(f, currentFileNode.Pos()) {
+					// First try the actual imported package name.
+					for _, imp := range f.Imports {
+						pkgName := pkg.TypesInfo().PkgNameOf(imp)
+						if pkgName != nil && pkgName.Name() == name {
+							return pkgName.Imported().Path(), true
+						}
+					}
+
+					// Then try the imported package name, as some
+					// packages are typically imported under a
+					// non-default name (e.g. pathpkg "path") but
+					// may be referred to in doc links using their
+					// canonical name.
+					for _, imp := range f.Imports {
+						pkgName := pkg.TypesInfo().PkgNameOf(imp)
+						if pkgName != nil && pkgName.Imported().Name() == name {
+							return pkgName.Imported().Path(), true
+						}
+					}
+
+					break
+				}
+			}
+			return "", false
+		},
+		LookupSym: func(recv, name string) (ok bool) {
+			// package-level decl?
+			if recv == "" {
+				return pkg.Types().Scope().Lookup(name) != nil
+			}
+
+			// method?
+			tname, ok := pkg.Types().Scope().Lookup(recv).(*types.TypeName)
+			if !ok {
+				return false
+			}
+			m, _, _ := types.LookupFieldOrMethod(tname.Type(), true, pkg.Types(), name)
+			return is[*types.Func](m)
+		},
+	}
+	return func(fileNode ast.Node, text string) *comment.Doc {
+		currentFileNode = fileNode
+		return parser.Parse(text)
+	}
+}

gopls/internal/golang/hover.go

@@ -52,6 +52,10 @@ import (
 // TODO(adonovan): see if we can wean all clients of this interface.
 type hoverJSON struct {
 	// Synopsis is a single sentence synopsis of the symbol's documentation.
+	//
+	// TODO(adonovan): in what syntax? It (usually) comes from doc.Synopsis,
+	// which produces "Text" form, but it may be fed to
+	// DocCommentToMarkdown, which expects doc comment syntax.
 	Synopsis string `json:"synopsis"`
 
 	// FullDocumentation is the symbol's full documentation.
@@ -1318,7 +1322,7 @@ func formatDoc(h *hoverJSON, options *settings.Options) string {
 		doc = h.FullDocumentation
 	}
 	if options.PreferredContentFormat == protocol.Markdown {
-		return CommentToMarkdown(doc, options)
+		return DocCommentToMarkdown(doc, options)
 	}
 	return doc
 }

gopls/internal/golang/pkgdoc.go

@@ -326,68 +326,34 @@ func PackageDocHTML(viewID string, pkg *cache.Package, web Web) ([]byte, error)
 		})
 	}
 
-	var docHTML func(comment string) []byte
+	// docHTML renders the doc comment as Markdown.
+	// The fileNode is used to deduce the enclosing file
+	// for the correct import mapping.
+	//
+	// It is not concurrency-safe.
+	var docHTML func(fileNode ast.Node, comment string) []byte
 	{
 		// Adapt doc comment parser and printer
 		// to our representation of Go packages
 		// so that doc links (e.g. "[fmt.Println]")
 		// become valid links.
-
-		printer := docpkg.Printer()
-		printer.DocLinkURL = func(link *comment.DocLink) string {
-			path := pkg.Metadata().PkgPath
-			if link.ImportPath != "" {
-				path = PackagePath(link.ImportPath)
-			}
-			fragment := link.Name
-			if link.Recv != "" {
-				fragment = link.Recv + "." + link.Name
-			}
-			return web.PkgURL(viewID, path, fragment)
-		}
-		parser := docpkg.Parser()
-		parser.LookupPackage = func(name string) (importPath string, ok bool) {
-			// Ambiguous: different files in the same
-			// package may have different import mappings,
-			// but the hook doesn't provide the file context.
-			// TODO(adonovan): conspire with docHTML to
-			// pass the doc comment's enclosing file through
-			// a shared variable, so that we can compute
-			// the correct per-file mapping.
-			//
-			// TODO(adonovan): check for PkgName.Name
-			// matches, but also check for
-			// PkgName.Imported.Namer matches, since some
-			// packages are typically imported under a
-			// non-default name (e.g. pathpkg "path") but
-			// may be referred to in doc links using their
-			// canonical name.
-			for _, f := range pkg.Syntax() {
-				for _, imp := range f.Imports {
-					pkgName := pkg.TypesInfo().PkgNameOf(imp)
-					if pkgName != nil && pkgName.Name() == name {
-						return pkgName.Imported().Path(), true
-					}
+		printer := &comment.Printer{
+			DocLinkURL: func(link *comment.DocLink) string {
+				path := pkg.Metadata().PkgPath
+				if link.ImportPath != "" {
+					path = PackagePath(link.ImportPath)
 				}
-			}
-			return "", false
-		}
-		parser.LookupSym = func(recv, name string) (ok bool) {
-			// package-level decl?
-			if recv == "" {
-				return pkg.Types().Scope().Lookup(name) != nil
-			}
-
-			// method?
-			tname, ok := pkg.Types().Scope().Lookup(recv).(*types.TypeName)
-			if !ok {
-				return false
-			}
-			m, _, _ := types.LookupFieldOrMethod(tname.Type(), true, pkg.Types(), name)
-			return is[*types.Func](m)
+				fragment := link.Name
+				if link.Recv != "" {
+					fragment = link.Recv + "." + link.Name
+				}
+				return web.PkgURL(viewID, path, fragment)
+			},
 		}
-		docHTML = func(comment string) []byte {
-			return printer.HTML(parser.Parse(comment))
+		parse := newDocCommentParser(pkg)
+		docHTML = func(fileNode ast.Node, comment string) []byte {
+			doc := parse(fileNode, comment)
+			return printer.HTML(doc)
 		}
 	}
 
@@ -715,7 +681,12 @@ window.addEventListener('load', function() {
 		"https://pkg.go.dev/"+string(pkg.Types().Path()))
 
 	// package doc
-	fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(docpkg.Doc))
+	for _, f := range pkg.Syntax() {
+		if f.Doc != nil {
+			fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(f.Doc, docpkg.Doc))
+			break
+		}
+	}
 
 	// symbol index
 	fmt.Fprintf(&buf, "<h2 id='hdr-Index'>Index</h2>\n")
@@ -773,7 +744,7 @@ window.addEventListener('load', function() {
 			fmt.Fprintf(&buf, "<pre class='code'>%s</pre>\n", nodeHTML(&decl2))
 
 			// comment (if any)
-			fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(v.Doc))
+			fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(v.Decl, v.Doc))
 		}
 	}
 	fmt.Fprintf(&buf, "<h2 id='hdr-Constants'>Constants</h2>\n")
@@ -814,7 +785,7 @@ window.addEventListener('load', function() {
 				nodeHTML(docfn.Decl.Type))
 
 			// comment (if any)
-			fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(docfn.Doc))
+			fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(docfn.Decl, docfn.Doc))
 		}
 	}
 	funcs(docpkg.Funcs)
@@ -835,7 +806,7 @@ window.addEventListener('load', function() {
 		fmt.Fprintf(&buf, "<pre class='code'>%s</pre>\n", nodeHTML(&decl2))
 
 		// comment (if any)
-		fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(doctype.Doc))
+		fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docHTML(doctype.Decl, doctype.Doc))
 
 		// subelements
 		values(doctype.Consts) // constants of type T
@@ -856,7 +827,7 @@ window.addEventListener('load', function() {
 
 			// comment (if any)
 			fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n",
-				docHTML(docmethod.Doc))
+				docHTML(docmethod.Decl, docmethod.Doc))
 		}
 	}
 

gopls/internal/golang/signature_help.go

@@ -223,7 +223,7 @@ func stringToSigInfoDocumentation(s string, options *settings.Options) *protocol
 	v := s
 	k := protocol.PlainText
 	if options.PreferredContentFormat == protocol.Markdown {
-		v = CommentToMarkdown(s, options)
+		v = DocCommentToMarkdown(s, options)
 		// whether or not content is newline terminated may not matter for LSP clients,
 		// but our tests expect trailing newlines to be stripped.
 		v = strings.TrimSuffix(v, "\n") // TODO(pjw): change the golden files

gopls/internal/server/completion.go

@@ -143,7 +143,7 @@ func toProtocolCompletionItems(candidates []completion.CompletionItem, surroundi
 		doc := &protocol.Or_CompletionItem_documentation{
 			Value: protocol.MarkupContent{
 				Kind:  protocol.Markdown,
-				Value: golang.CommentToMarkdown(candidate.Documentation, options),
+				Value: golang.DocCommentToMarkdown(candidate.Documentation, options),
 			},
 		}
 		if options.PreferredContentFormat != protocol.Markdown {

gopls/internal/test/integration/misc/webserver_test.go

@@ -21,6 +21,8 @@ import (
 	"golang.org/x/tools/internal/testenv"
 )
 
+// TODO(adonovan): define marker test verbs for checking package docs.
+
 // TestWebServer exercises the web server created on demand
 // for code actions such as "Browse package documentation".
 func TestWebServer(t *testing.T) {
@@ -267,6 +269,71 @@ func (*T) M() { /*in T.M*/}
 	})
 }
 
+// TestPkgDocFileImports tests that the doc links are rendered
+// as URLs based on the correct import mapping for the file in
+// which they appear.
+func TestPkgDocFileImports(t *testing.T) {
+	const files = `
+-- go.mod --
+module mod.com
+go 1.20
+
+-- a/a1.go --
+package a
+
+import "b"
+import alias "d"
+
+// [b.T] indeed refers to b.T.
+//
+// [alias.D] refers to d.D
+// but [d.D] also refers to d.D.
+type A1 int
+
+-- a/a2.go --
+package a
+
+import b "c"
+
+// [b.U] actually refers to c.U.
+type A2 int
+
+-- b/b.go --
+package b
+
+type T int
+type U int
+
+-- c/c.go --
+package c
+
+type T int
+type U int
+
+-- d/d.go --
+package d
+
+type D int
+`
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("a/a1.go")
+		uri1 := viewPkgDoc(t, env, env.Sandbox.Workdir.EntireFile("a/a1.go"))
+		doc := get(t, uri1)
+
+		// Check that the doc links are resolved using the
+		// appropriate import mapping for the file in which
+		// they appear.
+		checkMatch(t, true, doc, `pkg/b\?.*#T">b.T</a> indeed refers to b.T`)
+		checkMatch(t, true, doc, `pkg/c\?.*#U">b.U</a> actually refers to c.U`)
+
+		// Check that doc links can be resolved using either
+		// the original or the local name when they refer to a
+		// renaming import. (Local names are preferred.)
+		checkMatch(t, true, doc, `pkg/d\?.*#D">alias.D</a> refers to d.D`)
+		checkMatch(t, true, doc, `pkg/d\?.*#D">d.D</a> also refers to d.D`)
+	})
+}
+
 // viewPkgDoc invokes the "Browse package documentation" code action
 // at the specified location. It returns the URI of the document, or
 // fails the test.