From 32950ab3be12acf6d472893021373669979907ab Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@golang.org>
Date: Tue, 17 Jul 2018 21:23:18 +0000
Subject: [PATCH] godoc: add version info for struct fields

Follow-up to CL 85396, which only did types, funcs, and methods.

This adds version info to struct fields (in the form of small
comments) if the struct field's version is different from the struct
itself, to minimize how often this fires.

Updates golang/go#5778

Change-Id: I34d60326cbef88c108d5c4ca487eeb98b039b16e
Reviewed-on: https://go-review.googlesource.com/124495
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Devon O'Dell <dhobsd@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
---
 cmd/godoc/godoc_test.go | 85 +++++++++++++++++++++++++++-----------
 godoc/godoc.go          | 90 ++++++++++++++++++++++++++++++++++++++---
 godoc/snippet.go        |  2 +-
 godoc/versions.go       | 33 ++++++++++-----
 godoc/versions_test.go  |  7 +++-
 5 files changed, 176 insertions(+), 41 deletions(-)

diff --git a/cmd/godoc/godoc_test.go b/cmd/godoc/godoc_test.go
index 67760fcb878..681818eca67 100644
--- a/cmd/godoc/godoc_test.go
+++ b/cmd/godoc/godoc_test.go
@@ -255,77 +255,106 @@ func testWeb(t *testing.T, withIndex bool) {
 	}
 
 	tests := []struct {
-		path      string
-		match     []string
-		dontmatch []string
-		needIndex bool
+		path        string
+		contains    []string // substring
+		match       []string // regexp
+		notContains []string
+		needIndex   bool
 	}{
 		{
-			path:  "/",
-			match: []string{"Go is an open source programming language"},
+			path:     "/",
+			contains: []string{"Go is an open source programming language"},
 		},
 		{
-			path:  "/pkg/fmt/",
-			match: []string{"Package fmt implements formatted I/O"},
+			path:     "/pkg/fmt/",
+			contains: []string{"Package fmt implements formatted I/O"},
 		},
 		{
-			path:  "/src/fmt/",
-			match: []string{"scan_test.go"},
+			path:     "/src/fmt/",
+			contains: []string{"scan_test.go"},
 		},
 		{
-			path:  "/src/fmt/print.go",
-			match: []string{"// Println formats using"},
+			path:     "/src/fmt/print.go",
+			contains: []string{"// Println formats using"},
 		},
 		{
 			path: "/pkg",
-			match: []string{
+			contains: []string{
 				"Standard library",
 				"Package fmt implements formatted I/O",
 			},
-			dontmatch: []string{
+			notContains: []string{
 				"internal/syscall",
 				"cmd/gc",
 			},
 		},
 		{
 			path: "/pkg/?m=all",
-			match: []string{
+			contains: []string{
 				"Standard library",
 				"Package fmt implements formatted I/O",
 				"internal/syscall/?m=all",
 			},
-			dontmatch: []string{
+			notContains: []string{
 				"cmd/gc",
 			},
 		},
 		{
 			path: "/search?q=ListenAndServe",
-			match: []string{
+			contains: []string{
 				"/src",
 			},
-			dontmatch: []string{
+			notContains: []string{
 				"/pkg/bootstrap",
 			},
 			needIndex: true,
 		},
 		{
 			path: "/pkg/strings/",
-			match: []string{
+			contains: []string{
 				`href="/src/strings/strings.go"`,
 			},
 		},
 		{
 			path: "/cmd/compile/internal/amd64/",
-			match: []string{
+			contains: []string{
 				`href="/src/cmd/compile/internal/amd64/ssa.go"`,
 			},
 		},
 		{
 			path: "/pkg/math/bits/",
-			match: []string{
+			contains: []string{
 				`Added in Go 1.9`,
 			},
 		},
+		{
+			path: "/pkg/net/",
+			contains: []string{
+				`// IPv6 scoped addressing zone; added in Go 1.1`,
+			},
+		},
+		{
+			path: "/pkg/net/http/httptrace/",
+			match: []string{
+				`Got1xxResponse.*// Go 1\.11`,
+			},
+		},
+		// Verify we don't add version info to a struct field added the same time
+		// as the struct itself:
+		{
+			path: "/pkg/net/http/httptrace/",
+			match: []string{
+				`(?m)GotFirstResponseByte func\(\)\s*$`,
+			},
+		},
+		// Remove trailing periods before adding semicolons:
+		{
+			path: "/pkg/database/sql/",
+			contains: []string{
+				"The number of connections currently in use; added in Go 1.11",
+				"The number of idle connections; added in Go 1.11",
+			},
+		},
 	}
 	for _, test := range tests {
 		if test.needIndex && !withIndex {
@@ -338,18 +367,28 @@ func testWeb(t *testing.T, withIndex bool) {
 			continue
 		}
 		body, err := ioutil.ReadAll(resp.Body)
+		strBody := string(body)
 		resp.Body.Close()
 		if err != nil {
 			t.Errorf("GET %s: failed to read body: %s (response: %v)", url, err, resp)
 		}
 		isErr := false
-		for _, substr := range test.match {
+		for _, substr := range test.contains {
 			if !bytes.Contains(body, []byte(substr)) {
 				t.Errorf("GET %s: wanted substring %q in body", url, substr)
 				isErr = true
 			}
 		}
-		for _, substr := range test.dontmatch {
+		for _, re := range test.match {
+			if ok, err := regexp.MatchString(re, strBody); !ok || err != nil {
+				if err != nil {
+					t.Fatalf("Bad regexp %q: %v", re, err)
+				}
+				t.Errorf("GET %s: wanted to match %s in body", url, re)
+				isErr = true
+			}
+		}
+		for _, substr := range test.notContains {
 			if bytes.Contains(body, []byte(substr)) {
 				t.Errorf("GET %s: didn't want substring %q in body", url, substr)
 				isErr = true
diff --git a/godoc/godoc.go b/godoc/godoc.go
index 1b0d5b9381c..f191a9d989a 100644
--- a/godoc/godoc.go
+++ b/godoc/godoc.go
@@ -10,6 +10,7 @@
 package godoc // import "golang.org/x/tools/godoc"
 
 import (
+	"bufio"
 	"bytes"
 	"fmt"
 	"go/ast"
@@ -188,13 +189,13 @@ func (p *Presentation) infoSnippet_htmlFunc(info SpotInfo) string {
 
 func (p *Presentation) nodeFunc(info *PageInfo, node interface{}) string {
 	var buf bytes.Buffer
-	p.writeNode(&buf, info.FSet, node)
+	p.writeNode(&buf, info, info.FSet, node)
 	return buf.String()
 }
 
 func (p *Presentation) node_htmlFunc(info *PageInfo, node interface{}, linkify bool) string {
 	var buf1 bytes.Buffer
-	p.writeNode(&buf1, info.FSet, node)
+	p.writeNode(&buf1, info, info.FSet, node)
 
 	var buf2 bytes.Buffer
 	if n, _ := node.(ast.Node); n != nil && linkify && p.DeclLinks {
@@ -891,8 +892,12 @@ func replaceLeadingIndentation(body, oldIndent, newIndent string) string {
 	return buf.String()
 }
 
-// Write an AST node to w.
-func (p *Presentation) writeNode(w io.Writer, fset *token.FileSet, x interface{}) {
+// writeNode writes the AST node x to w.
+//
+// The provided fset must be non-nil. The pageInfo is optional. If
+// present, the pageInfo is used to add comments to struct fields to
+// say which version of Go introduced them.
+func (p *Presentation) writeNode(w io.Writer, pageInfo *PageInfo, fset *token.FileSet, x interface{}) {
 	// convert trailing tabs into spaces using a tconv filter
 	// to ensure a good outcome in most browsers (there may still
 	// be tabs in comments and strings, but converting those into
@@ -901,15 +906,88 @@ func (p *Presentation) writeNode(w io.Writer, fset *token.FileSet, x interface{}
 	// TODO(gri) rethink printer flags - perhaps tconv can be eliminated
 	//           with an another printer mode (which is more efficiently
 	//           implemented in the printer than here with another layer)
+
+	var pkgName, structName string
+	var apiInfo pkgAPIVersions
+	if gd, ok := x.(*ast.GenDecl); ok && pageInfo != nil && pageInfo.PDoc != nil &&
+		p.Corpus != nil &&
+		gd.Tok == token.TYPE && len(gd.Specs) != 0 {
+		pkgName = pageInfo.PDoc.ImportPath
+		if ts, ok := gd.Specs[0].(*ast.TypeSpec); ok {
+			if _, ok := ts.Type.(*ast.StructType); ok {
+				structName = ts.Name.Name
+			}
+		}
+		apiInfo = p.Corpus.pkgAPIInfo[pkgName]
+	}
+
+	var out = w
+	var buf bytes.Buffer
+	if structName != "" {
+		out = &buf
+	}
+
 	mode := printer.TabIndent | printer.UseSpaces
-	err := (&printer.Config{Mode: mode, Tabwidth: p.TabWidth}).Fprint(&tconv{p: p, output: w}, fset, x)
+	err := (&printer.Config{Mode: mode, Tabwidth: p.TabWidth}).Fprint(&tconv{p: p, output: out}, fset, x)
 	if err != nil {
 		log.Print(err)
 	}
+
+	// Add comments to struct fields saying which Go version introducd them.
+	if structName != "" {
+		fieldSince := apiInfo.fieldSince[structName]
+		typeSince := apiInfo.typeSince[structName]
+		// Add/rewrite comments on struct fields to note which Go version added them.
+		var buf2 bytes.Buffer
+		buf2.Grow(buf.Len() + len(" // Added in Go 1.n")*10)
+		bs := bufio.NewScanner(&buf)
+		for bs.Scan() {
+			line := bs.Bytes()
+			field := firstIdent(line)
+			var since string
+			if field != "" {
+				since = fieldSince[field]
+				if since != "" && since == typeSince {
+					// Don't highlight field versions if they were the
+					// same as the struct itself.
+					since = ""
+				}
+			}
+			if since == "" {
+				buf2.Write(line)
+			} else {
+				if bytes.Contains(line, slashSlash) {
+					line = bytes.TrimRight(line, " \t.")
+					buf2.Write(line)
+					buf2.WriteString("; added in Go ")
+				} else {
+					buf2.Write(line)
+					buf2.WriteString(" // Go ")
+				}
+				buf2.WriteString(since)
+			}
+			buf2.WriteByte('\n')
+		}
+		w.Write(buf2.Bytes())
+	}
 }
 
+var slashSlash = []byte("//")
+
 // WriteNode writes x to w.
 // TODO(bgarcia) Is this method needed? It's just a wrapper for p.writeNode.
 func (p *Presentation) WriteNode(w io.Writer, fset *token.FileSet, x interface{}) {
-	p.writeNode(w, fset, x)
+	p.writeNode(w, nil, fset, x)
+}
+
+// firstIdent returns the first identifier in x.
+// This actually parses "identifiers" that begin with numbers too, but we
+// never feed it such input, so it's fine.
+func firstIdent(x []byte) string {
+	x = bytes.TrimSpace(x)
+	i := bytes.IndexFunc(x, func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsNumber(r) })
+	if i == -1 {
+		return string(x)
+	}
+	return string(x[:i])
 }
diff --git a/godoc/snippet.go b/godoc/snippet.go
index dd9c8225cc2..1750478606e 100644
--- a/godoc/snippet.go
+++ b/godoc/snippet.go
@@ -24,7 +24,7 @@ type Snippet struct {
 func (p *Presentation) newSnippet(fset *token.FileSet, decl ast.Decl, id *ast.Ident) *Snippet {
 	// TODO instead of pretty-printing the node, should use the original source instead
 	var buf1 bytes.Buffer
-	p.writeNode(&buf1, fset, decl)
+	p.writeNode(&buf1, nil, fset, decl)
 	// wrap text with <pre> tag
 	var buf2 bytes.Buffer
 	buf2.WriteString("<pre>")
diff --git a/godoc/versions.go b/godoc/versions.go
index e476ab16725..8a04905e845 100644
--- a/godoc/versions.go
+++ b/godoc/versions.go
@@ -31,8 +31,9 @@ type apiVersions map[string]pkgAPIVersions // keyed by Go package ("net/http")
 // form "1.1", "1.2", etc.
 type pkgAPIVersions struct {
 	typeSince   map[string]string            // "Server" -> "1.7"
-	methodSince map[string]map[string]string // "*Server"->"Shutdown"->1.8
+	methodSince map[string]map[string]string // "*Server" ->"Shutdown"->1.8
 	funcSince   map[string]string            // "NewServer" -> "1.7"
+	fieldSince  map[string]map[string]string // "ClientTrace" -> "Got1xxResponse" -> "1.11"
 }
 
 // sinceVersionFunc returns a string (such as "1.7") specifying which Go
@@ -62,10 +63,11 @@ func (v apiVersions) sinceVersionFunc(kind, receiver, name, pkg string) string {
 // versionedRow represents an API feature, a parsed line of a
 // $GOROOT/api/go.*txt file.
 type versionedRow struct {
-	pkg  string // "net/http"
-	kind string // "type", "func", "method", TODO: "const", "var"
-	recv string // for methods, the receiver type ("Server", "*Server")
-	name string // name of type, func, or method
+	pkg        string // "net/http"
+	kind       string // "type", "func", "method", "field" TODO: "const", "var"
+	recv       string // for methods, the receiver type ("Server", "*Server")
+	name       string // name of type, (struct) field, func, method
+	structName string // for struct fields, the outer struct name
 }
 
 // versionParser parses $GOROOT/api/go*.txt files and stores them in in its rows field.
@@ -100,6 +102,7 @@ func (vp *versionParser) parseFile(name string) error {
 				typeSince:   make(map[string]string),
 				methodSince: make(map[string]map[string]string),
 				funcSince:   make(map[string]string),
+				fieldSince:  make(map[string]map[string]string),
 			}
 			vp.res[row.pkg] = pkgi
 		}
@@ -113,6 +116,11 @@ func (vp *versionParser) parseFile(name string) error {
 				pkgi.methodSince[row.recv] = make(map[string]string)
 			}
 			pkgi.methodSince[row.recv][row.name] = ver
+		case "field":
+			if _, ok := pkgi.fieldSince[row.structName]; !ok {
+				pkgi.fieldSince[row.structName] = make(map[string]string)
+			}
+			pkgi.fieldSince[row.structName][row.name] = ver
 		}
 	}
 	return sc.Err()
@@ -139,18 +147,23 @@ func parseRow(s string) (vr versionedRow, ok bool) {
 
 	switch {
 	case strings.HasPrefix(rest, "type "):
-		vr.kind = "type"
 		rest = rest[len("type "):]
 		sp := strings.IndexByte(rest, ' ')
 		if sp == -1 {
 			return
 		}
 		vr.name, rest = rest[:sp], rest[sp+1:]
-		if strings.HasPrefix(rest, "struct, ") {
-			// TODO: handle struct fields
-			return
+		if !strings.HasPrefix(rest, "struct, ") {
+			vr.kind = "type"
+			return vr, true
+		}
+		rest = rest[len("struct, "):]
+		if i := strings.IndexByte(rest, ' '); i != -1 {
+			vr.kind = "field"
+			vr.structName = vr.name
+			vr.name = rest[:i]
+			return vr, true
 		}
-		return vr, true
 	case strings.HasPrefix(rest, "func "):
 		vr.kind = "func"
 		rest = rest[len("func "):]
diff --git a/godoc/versions_test.go b/godoc/versions_test.go
index 439fc0256c8..bb408514d00 100644
--- a/godoc/versions_test.go
+++ b/godoc/versions_test.go
@@ -27,7 +27,12 @@ func TestParseVersionRow(t *testing.T) {
 		},
 		{
 			row: "pkg archive/tar, type Header struct, AccessTime time.Time",
-			// TODO: implement; for now we expect nothing
+			want: versionedRow{
+				pkg:        "archive/tar",
+				kind:       "field",
+				structName: "Header",
+				name:       "AccessTime",
+			},
 		},
 		{
 			row: "pkg archive/tar, method (*Reader) Read([]uint8) (int, error)",