gopls/internal/golang: RenderPackageDoc: emit anchors for var/const

This CL causes the package rendering to emit an <a id=...> anchor
for each name defined by a var or const declaration.

Also, choose the initial fragment id (e.g. "#Buffer.Len") based
on the selection.

+ Test that anchors are emitted.

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

Author: adonovan

gopls/doc/release/v0.16.0.md

@@ -10,11 +10,13 @@ go install golang.org/x/tools/[email protected]
 
 Gopls now offers a "View package documentation" code action that opens
 a local web page displaying the generated documentation for the
-current Go package in a form similar to https://pkg.go.dev. Use it to
-preview the marked-up documentation as you prepare API changes, or to
-read the documentation for locally edited packages, even ones that
-have not yet been saved. Reload the page after an edit to see updated
-documentation.
+current Go package in a form similar to https://pkg.go.dev.
+The page will be initially scrolled to the documentation for the
+declaration containing the cursor.
+Use this feature to preview the marked-up documentation as you prepare API
+changes, or to read the documentation for locally edited packages,
+even ones that have not yet been saved. Reload the page after an edit
+to see updated documentation.
 
 TODO: demo in VS Code.
 

gopls/internal/golang/pkgdoc.go

@@ -20,6 +20,8 @@ package golang
 // - abbreviate long signatures by replacing parameters 4 onwards with "...".
 // - style the <li> bullets in the index as invisible.
 // - add push notifications such as didChange -> reload.
+// - there appears to be a maximum file size beyond which the
+//   "source.doc" code action is not offered. Remove that.
 
 import (
 	"bytes"
@@ -254,8 +256,6 @@ function httpGET(url) {
 						emit(n.Pos())
 						pos = n.End()
 						if url := linkify(n); url != "" {
-							// TODO(adonovan): emit anchors (not hrefs)
-							// for package-level defining idents.
 							fmt.Fprintf(&buf, "<a class='id' href='%s'>%s</a>", url, escape(n.Name))
 						} else {
 							buf.WriteString(escape(n.Name)) // plain
@@ -355,6 +355,11 @@ function httpGET(url) {
 	// constants and variables
 	values := func(vals []*doc.Value) {
 		for _, v := range vals {
+			// anchors
+			for _, name := range v.Names {
+				fmt.Fprintf(&buf, "<a id='%s'></a>\n", escape(name))
+			}
+
 			// declaration
 			decl2 := *v.Decl // shallow copy
 			decl2.Doc = nil

gopls/internal/server/command.go

@@ -10,6 +10,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"go/ast"
 	"io"
 	"os"
 	"path/filepath"
@@ -507,16 +508,16 @@ func (c *commandHandler) Doc(ctx context.Context, loc protocol.Location) error {
 		progress: "", // the operation should be fast
 		forURI:   loc.URI,
 	}, func(ctx context.Context, deps commandDeps) error {
-		pkg, _, err := golang.NarrowestPackageForFile(ctx, deps.snapshot, loc.URI)
+		pkg, pgf, err := golang.NarrowestPackageForFile(ctx, deps.snapshot, loc.URI)
 		if err != nil {
 			return err
 		}
 
 		// When invoked from a _test.go file, show the
 		// documentation of the package under test.
-		path := pkg.Metadata().PkgPath
+		pkgpath := pkg.Metadata().PkgPath
 		if pkg.Metadata().ForTest != "" {
-			path = pkg.Metadata().ForTest
+			pkgpath = pkg.Metadata().ForTest
 		}
 
 		// Start web server.
@@ -525,12 +526,59 @@ func (c *commandHandler) Doc(ctx context.Context, loc protocol.Location) error {
 			return err
 		}
 
+		// Compute fragment (e.g. "#Buffer.Len") based on
+		// enclosing top-level declaration, if exported.
+		var fragment string
+		pos, err := pgf.PositionPos(loc.Range.Start)
+		if err != nil {
+			return err
+		}
+		path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos)
+		if n := len(path); n > 1 {
+			switch decl := path[n-2].(type) {
+			case *ast.FuncDecl:
+				if decl.Name.IsExported() {
+					// e.g. "#Println"
+					fragment = decl.Name.Name
+
+					// method?
+					if decl.Recv != nil && len(decl.Recv.List) > 0 {
+						recv := decl.Recv.List[0].Type
+						if star, ok := recv.(*ast.StarExpr); ok {
+							recv = star.X // *N -> N
+						}
+						if id, ok := recv.(*ast.Ident); ok && id.IsExported() {
+							// e.g. "#Buffer.Len"
+							fragment = id.Name + "." + fragment
+						} else {
+							fragment = ""
+						}
+					}
+				}
+
+			case *ast.GenDecl:
+				// path=[... Spec? GenDecl File]
+				for _, spec := range decl.Specs {
+					if n > 2 && spec == path[n-3] {
+						var name *ast.Ident
+						switch spec := spec.(type) {
+						case *ast.ValueSpec:
+							// var, const: use first name
+							name = spec.Names[0]
+						case *ast.TypeSpec:
+							name = spec.Name
+						}
+						if name != nil && name.IsExported() {
+							fragment = name.Name
+						}
+						break
+					}
+				}
+			}
+		}
+
 		// Direct the client to open the /pkg page.
-		//
-		// TODO(adonovan): compute fragment (e.g. "#fmt.Println") based on loc.Range.
-		// (Should it document the selected symbol, or the enclosing decl?)
-		fragment := ""
-		url := web.pkgURL(deps.snapshot.View(), path, fragment)
+		url := web.pkgURL(deps.snapshot.View(), pkgpath, fragment)
 		openClientBrowser(ctx, c.s.client, url)
 
 		return nil

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

@@ -81,34 +81,61 @@ func TestRenderNoPanic66449(t *testing.T) {
 	// that would corrupt the AST while filtering out unexported
 	// symbols such as b, causing nodeHTML to panic.
 	// Now it doesn't crash.
+	//
+	// We also check cross-reference anchors for all symbols.
 	const files = `
 -- go.mod --
 module example.com
 
 -- a/a.go --
 package a
 
-var A, b = 0, 0
+// The '1' suffix is to reduce risk of spurious matches with other HTML substrings.
 
-func ExportedFunc()
-func unexportedFunc()
-type ExportedType int
-type unexportedType int
+var V1, v1 = 0, 0
+const C1, c1 = 0, 0
+
+func F1()
+func f1()
+
+type T1 int
+type t1 int
+
+func (T1) M1() {}
+func (T1) m1() {}
+
+func (t1) M1() {}
+func (t1) m1() {}
 `
 	Run(t, files, func(t *testing.T, env *Env) {
 		uri1 := viewPkgDoc(t, env, "a/a.go")
-		doc1 := get(t, uri1)
+		doc := get(t, uri1)
 		// (Ideally our code rendering would also
 		// eliminate unexported symbols...)
-		// TODO(adonovan): when we emit var anchors,
-		// check that #A exists and #b does not.
-		checkMatch(t, true, doc1, "var A, b = .*0.*0")
+		checkMatch(t, true, doc, "var V1, v1 = .*0.*0")
+		checkMatch(t, true, doc, "const C1, c1 = .*0.*0")
 
 		// Unexported funcs/types/... must still be discarded.
-		checkMatch(t, true, doc1, "ExportedFunc")
-		checkMatch(t, false, doc1, "unexportedFunc")
-		checkMatch(t, true, doc1, "ExportedType")
-		checkMatch(t, false, doc1, "unexportedType")
+		checkMatch(t, true, doc, "F1")
+		checkMatch(t, false, doc, "f1")
+		checkMatch(t, true, doc, "T1")
+		checkMatch(t, false, doc, "t1")
+
+		// Also, check that anchors exist (only) for exported symbols.
+		// exported:
+		checkMatch(t, true, doc, "<a id='V1'")
+		checkMatch(t, true, doc, "<a id='C1'")
+		checkMatch(t, true, doc, "<h3 id='T1'")
+		checkMatch(t, true, doc, "<h3 id='F1'")
+		checkMatch(t, true, doc, "<h4 id='T1.M1'")
+		// unexported:
+		checkMatch(t, false, doc, "<a id='v1'")
+		checkMatch(t, false, doc, "<a id='c1'")
+		checkMatch(t, false, doc, "<h3 id='t1'")
+		checkMatch(t, false, doc, "<h3 id='f1'")
+		checkMatch(t, false, doc, "<h4 id='T1.m1'")
+		checkMatch(t, false, doc, "<h4 id='t1.M1'")
+		checkMatch(t, false, doc, "<h4 id='t1.m1'")
 	})
 }