x/tools/cmd/godoc: fix broken links in composite literals

Previously, field names in composite literals were treated as normal
identifiers. For example, in Foo{X: 42}, X was treated link a normal
variable and was linked to "#X".

With this change, field links now include a prefix for their type
definition, for example, "#Foo.X".

Fixes golang/go#16031

Change-Id: I9cb501704f284fbbc05424555312307c61843e47
Reviewed-on: https://go-review.googlesource.com/36830
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

godoc/godoc17_test.go

@@ -29,7 +29,7 @@ func TestStructField(t *testing.T) {
 				fmt.Fprintf(&buf, "\t// Field%d is foo.\n\tField%d int\n\n", i, i)
 			}
 			fmt.Fprintf(&buf, "}\n")
-			linkifyStructFields(t, buf.Bytes())
+			linkifySource(t, buf.Bytes())
 		})
 	}
 }

godoc/godoc_test.go

@@ -5,7 +5,7 @@
 package godoc
 
 import (
-	"go/ast"
+	"bytes"
 	"go/parser"
 	"go/token"
 	"strings"
@@ -124,7 +124,7 @@ func TestSanitizeFunc(t *testing.T) {
 // Test that we add <span id="StructName.FieldName"> elements
 // to the HTML of struct fields.
 func TestStructFieldsIDAttributes(t *testing.T) {
-	got := linkifyStructFields(t, []byte(`
+	got := linkifySource(t, []byte(`
 package foo
 
 type T struct {
@@ -157,7 +157,25 @@ Opt *<a href="/pkg/builtin/#int">int</a>
 	}
 }
 
-func linkifyStructFields(t *testing.T, src []byte) string {
+func TestCompositeLitLinkFields(t *testing.T) {
+	got := linkifySource(t, []byte(`
+package foo
+
+type T struct {
+	X int
+}
+
+var S T = T{X: 12}`))
+	want := `type T struct {
+<span id="T.X"></span>X <a href="/pkg/builtin/#int">int</a>
+}
+var S <a href="#T">T</a> = <a href="#T">T</a>{<a href="#T.X">X</a>: 12}`
+	if got != want {
+		t.Errorf("got: %s\n\nwant: %s\n", got, want)
+	}
+}
+
+func linkifySource(t *testing.T, src []byte) string {
 	p := &Presentation{
 		DeclLinks: true,
 	}
@@ -166,11 +184,17 @@ func linkifyStructFields(t *testing.T, src []byte) string {
 	if err != nil {
 		t.Fatal(err)
 	}
-	genDecl := af.Decls[0].(*ast.GenDecl)
+	var buf bytes.Buffer
 	pi := &PageInfo{
 		FSet: fset,
 	}
-	return p.node_htmlFunc(pi, genDecl, true)
+	sep := ""
+	for _, decl := range af.Decls {
+		buf.WriteString(sep)
+		sep = "\n"
+		buf.WriteString(p.node_htmlFunc(pi, decl, true))
+	}
+	return buf.String()
 }
 
 func TestScanIdentifier(t *testing.T) {

godoc/linkify.go

@@ -78,61 +78,6 @@ type link struct {
 	path, name string // package path, identifier name
 }
 
-// linksFor returns the list of links for the identifiers used
-// by node in the same order as they appear in the source.
-//
-func linksFor(node ast.Node) (list []link) {
-	modes := identModesFor(node)
-
-	// NOTE: We are expecting ast.Inspect to call the
-	//       callback function in source text order.
-	ast.Inspect(node, func(node ast.Node) bool {
-		switch n := node.(type) {
-		case *ast.Ident:
-			m := modes[n]
-			info := link{mode: m}
-			switch m {
-			case identUse:
-				if n.Obj == nil && predeclared[n.Name] {
-					info.path = builtinPkgPath
-				}
-				info.name = n.Name
-			case identDef:
-				// any declaration expect const or var - empty link
-			case identVal:
-				// const or var declaration
-				info.name = n.Name
-			}
-			list = append(list, info)
-			return false
-		case *ast.SelectorExpr:
-			// Detect qualified identifiers of the form pkg.ident.
-			// If anything fails we return true and collect individual
-			// identifiers instead.
-			if x, _ := n.X.(*ast.Ident); x != nil {
-				// x must be a package for a qualified identifier
-				if obj := x.Obj; obj != nil && obj.Kind == ast.Pkg {
-					if spec, _ := obj.Decl.(*ast.ImportSpec); spec != nil {
-						// spec.Path.Value is the import path
-						if path, err := strconv.Unquote(spec.Path.Value); err == nil {
-							// Register two links, one for the package
-							// and one for the qualified identifier.
-							info := link{path: path}
-							list = append(list, info)
-							info.name = n.Sel.Name
-							list = append(list, info)
-							return false
-						}
-					}
-				}
-			}
-		}
-		return true
-	})
-
-	return
-}
-
 // The identMode describes how an identifier is "used" at its source location.
 type identMode int
 
@@ -142,28 +87,32 @@ const (
 	identVal                  // identifier is defined in a const or var declaration
 )
 
-// identModesFor returns a map providing the identMode for each identifier used by node.
+// linksFor returns the list of links for the identifiers used
-func identModesFor(node ast.Node) map[*ast.Ident]identMode {
+// by node in the same order as they appear in the source.
-	m := make(map[*ast.Ident]identMode)
+//
+func linksFor(node ast.Node) (links []link) {
+	// linkMap tracks link information for each ast.Ident node. Entries may
+	// be created out of source order (for example, when we visit a parent
+	// definition node). These links are appended to the returned slice when
+	// their ast.Ident nodes are visited.
+	linkMap := make(map[*ast.Ident]link)
 
 	ast.Inspect(node, func(node ast.Node) bool {
 		switch n := node.(type) {
 		case *ast.Field:
 			for _, n := range n.Names {
-				m[n] = identDef
+				linkMap[n] = link{mode: identDef}
 			}
 		case *ast.ImportSpec:
 			if name := n.Name; name != nil {
-				m[name] = identDef
+				linkMap[name] = link{mode: identDef}
 			}
 		case *ast.ValueSpec:
 			for _, n := range n.Names {
-				m[n] = identVal
+				linkMap[n] = link{mode: identVal}
 			}
 		case *ast.TypeSpec:
-			m[n.Name] = identDef
+			linkMap[n.Name] = link{mode: identDef}
-		case *ast.FuncDecl:
-			m[n.Name] = identDef
 		case *ast.AssignStmt:
 			// Short variable declarations only show up if we apply
 			// this code to all source code (as opposed to exported
@@ -176,15 +125,79 @@ func identModesFor(node ast.Node) map[*ast.Ident]identMode {
 					// Each lhs expression should be an
 					// ident, but we are conservative and check.
 					if n, _ := x.(*ast.Ident); n != nil {
-						m[n] = identVal
+						linkMap[n] = link{mode: identVal}
 					}
 				}
 			}
+		case *ast.SelectorExpr:
+			// Detect qualified identifiers of the form pkg.ident.
+			// If anything fails we return true and collect individual
+			// identifiers instead.
+			if x, _ := n.X.(*ast.Ident); x != nil {
+				// Create links only if x is a qualified identifier.
+				if obj := x.Obj; obj != nil && obj.Kind == ast.Pkg {
+					if spec, _ := obj.Decl.(*ast.ImportSpec); spec != nil {
+						// spec.Path.Value is the import path
+						if path, err := strconv.Unquote(spec.Path.Value); err == nil {
+							// Register two links, one for the package
+							// and one for the qualified identifier.
+							linkMap[x] = link{mode: identUse, path: path}
+							linkMap[n.Sel] = link{mode: identUse, path: path, name: n.Sel.Name}
+						}
+					}
+				}
+			}
+		case *ast.CompositeLit:
+			// Detect field names within composite literals. These links should
+			// be prefixed by the type name.
+			fieldPath := ""
+			prefix := ""
+			switch typ := n.Type.(type) {
+			case *ast.Ident:
+				prefix = typ.Name + "."
+			case *ast.SelectorExpr:
+				if x, _ := typ.X.(*ast.Ident); x != nil {
+					// Create links only if x is a qualified identifier.
+					if obj := x.Obj; obj != nil && obj.Kind == ast.Pkg {
+						if spec, _ := obj.Decl.(*ast.ImportSpec); spec != nil {
+							// spec.Path.Value is the import path
+							if path, err := strconv.Unquote(spec.Path.Value); err == nil {
+								// Register two links, one for the package
+								// and one for the qualified identifier.
+								linkMap[x] = link{mode: identUse, path: path}
+								linkMap[typ.Sel] = link{mode: identUse, path: path, name: typ.Sel.Name}
+								fieldPath = path
+								prefix = typ.Sel.Name + "."
+							}
+						}
+					}
+				}
+			}
+			for _, e := range n.Elts {
+				if kv, ok := e.(*ast.KeyValueExpr); ok {
+					if k, ok := kv.Key.(*ast.Ident); ok {
+						// Note: there is some syntactic ambiguity here. We cannot determine
+						// if this is a struct literal or a map literal without type
+						// information. We assume struct literal.
+						name := prefix + k.Name
+						linkMap[k] = link{mode: identUse, path: fieldPath, name: name}
+					}
+				}
+			}
+		case *ast.Ident:
+			if l, ok := linkMap[n]; ok {
+				links = append(links, l)
+			} else {
+				l := link{mode: identUse, name: n.Name}
+				if n.Obj == nil && predeclared[n.Name] {
+					l.path = builtinPkgPath
+				}
+				links = append(links, l)
+			}
 		}
 		return true
 	})
-
+	return
-	return m
 }
 
 // The predeclared map represents the set of all predeclared identifiers.