go/doc: revert API change (per former discussion) and cleanup

Separating Method from Func made the code only more complicated
without adding much to the useability/readability of the API.
Reverted to where it was, but leaving the new method-specific
fields Orig and Level.

Former clients (godoc) of doc.Method only used the Func fields;
and because Func was embedded, no changes are needed with respect
to the removal of Method.

Changed type of Func.Recv from ast.Expr to string. This was a
long-standing TODO. Also implemented Func.Orig field (another TODO).

No further go/doc API changes are expected for Go 1.

R=rsc, r, r
CC=golang-dev
https://golang.org/cl/5577043

doc/go1.html

@@ -946,9 +946,8 @@ The type names of the <a href="go/doc/"><code>go/doc</code></a> package have bee
 streamlined by removing the <code>Doc</code> suffix: <code>PackageDoc</code>
 is now <code>Package</code>, <code>ValueDoc</code> is <code>Value</code>, etc.
 Also, all types now consistently have a <code>Name</code> field (or <code>Names</code>,
-in the case of type <code>Value</code>), <code>Type.Factories</code> has become
+in the case of type <code>Value</code>) and <code>Type.Factories</code> has become
-<code>Type.Funcs</code>, and there is a new type <code>Method</code> that describes
+<code>Type.Funcs</code>.
-methods in more detail.
 Instead of calling <code>doc.NewPackageDoc(pkg, importpath)</code>,
 documentation for a package is created with:
 </p>

doc/go1.tmpl

@@ -849,9 +849,8 @@ The type names of the <a href="go/doc/"><code>go/doc</code></a> package have bee
 streamlined by removing the <code>Doc</code> suffix: <code>PackageDoc</code>
 is now <code>Package</code>, <code>ValueDoc</code> is <code>Value</code>, etc.
 Also, all types now consistently have a <code>Name</code> field (or <code>Names</code>,
-in the case of type <code>Value</code>), <code>Type.Factories</code> has become
+in the case of type <code>Value</code>) and <code>Type.Factories</code> has become
-<code>Type.Funcs</code>, and there is a new type <code>Method</code> that describes
+<code>Type.Funcs</code>.
-methods in more detail.
 Instead of calling <code>doc.NewPackageDoc(pkg, importpath)</code>,
 documentation for a package is created with:
 </p>

lib/godoc/package.html

@@ -108,7 +108,7 @@
 		{{end}}
 		{{range .Methods}}
 			{{$name_html := html .Name}}
-			<h3 id="{{$tname_html}}.{{$name_html}}">func ({{node_html .Recv $.FSet}}) <a href="/{{posLink_url .Decl $.FSet}}">{{$name_html}}</a></h3>
+			<h3 id="{{$tname_html}}.{{$name_html}}">func ({{html .Recv}}) <a href="/{{posLink_url .Decl $.FSet}}">{{$name_html}}</a></h3>
 			<p><code>{{node_html .Decl $.FSet}}</code></p>
 			{{comment_html .Doc}}
 			{{$name := printf "%s_%s" $tname .Name}}

src/pkg/go/doc/doc.go

@@ -35,14 +35,6 @@ type Value struct {
 	order int
 }
 
-// Method is the documentation for a method declaration.
-type Method struct {
-	*Func
-	// TODO(gri) The following fields are not set at the moment. 
-	Origin *Type // original receiver base type
-	Level  int   // embedding level; 0 means Method is not embedded
-}
-
 // Type is the documentation for type declaration.
 type Type struct {
 	Doc  string
@@ -50,21 +42,23 @@ type Type struct {
 	Decl *ast.GenDecl
 
 	// associated declarations
-	Consts  []*Value  // sorted list of constants of (mostly) this type
+	Consts  []*Value // sorted list of constants of (mostly) this type
-	Vars    []*Value  // sorted list of variables of (mostly) this type
+	Vars    []*Value // sorted list of variables of (mostly) this type
-	Funcs   []*Func   // sorted list of functions returning this type
+	Funcs   []*Func  // sorted list of functions returning this type
-	Methods []*Method // sorted list of methods (including embedded ones) of this type
+	Methods []*Func  // sorted list of methods (including embedded ones) of this type
-
-	order int
 }
 
 // Func is the documentation for a func declaration.
 type Func struct {
 	Doc  string
 	Name string
-	// TODO(gri) remove Recv once we switch to new implementation
-	Recv ast.Expr // TODO(rsc): Would like string here
 	Decl *ast.FuncDecl
+
+	// methods
+	// (for functions, these fields have the respective zero value)
+	Recv  string // actual   receiver "T" or "*T"
+	Orig  string // original receiver "T" or "*T"
+	Level int    // embedding level; 0 means not embedded
 }
 
 // Mode values control the operation of New.
@@ -77,6 +71,8 @@ const (
 )
 
 // New computes the package documentation for the given package AST.
+// New takes ownership of the AST pkg and may edit or overwrite it.
+//
 func New(pkg *ast.Package, importPath string, mode Mode) *Package {
 	var r reader
 	r.readPackage(pkg, mode)
@@ -92,6 +88,6 @@ func New(pkg *ast.Package, importPath string, mode Mode) *Package {
 		Consts:     sortedValues(r.values, token.CONST),
 		Types:      sortedTypes(r.types),
 		Vars:       sortedValues(r.values, token.VAR),
-		Funcs:      r.funcs.sortedFuncs(),
+		Funcs:      sortedFuncs(r.funcs),
 	}
 }

src/pkg/go/doc/filter.go

@@ -71,17 +71,6 @@ func filterFuncs(a []*Func, f Filter) []*Func {
 	return a[0:w]
 }
 
-func filterMethods(a []*Method, f Filter) []*Method {
-	w := 0
-	for _, md := range a {
-		if f(md.Name) {
-			a[w] = md
-			w++
-		}
-	}
-	return a[0:w]
-}
-
 func filterTypes(a []*Type, f Filter) []*Type {
 	w := 0
 	for _, td := range a {
@@ -93,7 +82,7 @@ func filterTypes(a []*Type, f Filter) []*Type {
 			td.Consts = filterValues(td.Consts, f)
 			td.Vars = filterValues(td.Vars, f)
 			td.Funcs = filterFuncs(td.Funcs, f)
-			td.Methods = filterMethods(td.Methods, f)
+			td.Methods = filterFuncs(td.Methods, f)
 			n += len(td.Consts) + len(td.Vars) + len(td.Funcs) + len(td.Methods)
 		}
 		if n > 0 {

src/pkg/go/doc/reader.go

@@ -16,44 +16,55 @@ import (
 // function/method sets
 //
 // Internally, we treat functions like methods and collect them in method sets.
-// TODO(gri): Consider eliminating the external distinction. Doesn't really buy
-//            much and would simplify code and API.
 
-// methodSet describes a set of methods. Entries where Func == nil are conflict
+// methodSet describes a set of methods. Entries where Decl == nil are conflict
 // entries (more then one method with the same name at the same embedding level).
 //
-type methodSet map[string]*Method
+type methodSet map[string]*Func
 
-// set adds the function f to mset. If there are multiple f's with
+// recvString returns a string representation of recv of the
-// the same name, set keeps the first one with documentation.
+// form "T", "*T", or "BADRECV" (if not a proper receiver type).
+//
+func recvString(recv ast.Expr) string {
+	switch t := recv.(type) {
+	case *ast.Ident:
+		return t.Name
+	case *ast.StarExpr:
+		return "*" + recvString(t.X)
+	}
+	return "BADRECV"
+}
+
+// set creates the corresponding Func for f and adds it to mset.
+// If there are multiple f's with the same name, set keeps the first
+// one with documentation; conflicts are ignored.
 //
 func (mset methodSet) set(f *ast.FuncDecl) {
 	name := f.Name.Name
-	if g, found := mset[name]; found && g.Doc != "" {
+	if g := mset[name]; g != nil && g.Doc != "" {
 		// A function with the same name has already been registered;
 		// since it has documentation, assume f is simply another
 		// implementation and ignore it. This does not happen if the
-		// caller is using build.ScanDir to determine the list of files
+		// caller is using go/build.ScanDir to determine the list of
-		// implementing a package. 
+		// files implementing a package. 
-		// TODO(gri) consider collecting all functions, or at least
-		//           all comments
 		return
 	}
 	// function doesn't exist or has no documentation; use f
-	var recv ast.Expr
+	recv := ""
 	if f.Recv != nil {
+		var typ ast.Expr
 		// be careful in case of incorrect ASTs
 		if list := f.Recv.List; len(list) == 1 {
-			recv = list[0].Type
+			typ = list[0].Type
 		}
+		recv = recvString(typ)
 	}
-	mset[name] = &Method{
+	mset[name] = &Func{
-		Func: &Func{
+		Doc:  f.Doc.Text(),
-			Doc:  f.Doc.Text(),
+		Name: name,
-			Name: name,
+		Decl: f,
-			Decl: f,
+		Recv: recv,
-			Recv: recv,
+		Orig: recv,
-		},
 	}
 	f.Doc = nil // doc consumed - remove from AST
 }
@@ -62,57 +73,19 @@ func (mset methodSet) set(f *ast.FuncDecl) {
 // already contains a method with the same name at the same or a higher
 // level then m.
 //
-func (mset methodSet) add(m *Method) {
+func (mset methodSet) add(m *Func) {
 	old := mset[m.Name]
 	if old == nil || m.Level < old.Level {
 		mset[m.Name] = m
 		return
 	}
 	if old != nil && m.Level == old.Level {
-		// conflict - mark it using a method with nil Func
+		// conflict - mark it using a method with nil Decl
-		mset[m.Name] = &Method{Level: m.Level}
+		mset[m.Name] = &Func{
-	}
+			Name:  m.Name,
-}
+			Level: m.Level,
-
-func (mset methodSet) sortedFuncs() []*Func {
-	list := make([]*Func, len(mset))
-	i := 0
-	for _, m := range mset {
-		// exclude conflict entries
-		// (this should never happen for functions, but this code
-		// and the code in sortedMethods may be merged eventually,
-		// so leave it for symmetry).
-		if m.Func != nil {
-			list[i] = m.Func
-			i++
 		}
 	}
-	list = list[0:i]
-	sortBy(
-		func(i, j int) bool { return list[i].Name < list[j].Name },
-		func(i, j int) { list[i], list[j] = list[j], list[i] },
-		len(list),
-	)
-	return list
-}
-
-func (mset methodSet) sortedMethods() []*Method {
-	list := make([]*Method, len(mset))
-	i := 0
-	for _, m := range mset {
-		// exclude conflict entries
-		if m.Func != nil {
-			list[i] = m
-			i++
-		}
-	}
-	list = list[0:i]
-	sortBy(
-		func(i, j int) bool { return list[i].Name < list[j].Name },
-		func(i, j int) { list[i], list[j] = list[j], list[i] },
-		len(list),
-	)
-	return list
 }
 
 // ----------------------------------------------------------------------------
@@ -408,9 +381,6 @@ func (r *reader) readFunc(fun *ast.FuncDecl) {
 		return
 	}
 
-	// determine funcs map with which to associate the Func for this declaration
-	funcs := r.funcs
-
 	// perhaps a factory function
 	// determine result type, if any
 	if fun.Type.Results.NumFields() >= 1 {
@@ -422,15 +392,15 @@ func (r *reader) readFunc(fun *ast.FuncDecl) {
 			if n, imp := baseTypeName(res.Type); !imp && r.isVisible(n) {
 				if typ := r.lookupType(n); typ != nil {
 					// associate Func with typ
-					funcs = typ.funcs
+					typ.funcs.set(fun)
+					return
 				}
 			}
 		}
 	}
 
-	// associate the Func
+	// just an ordinary function
-	funcs.set(fun)
+	r.funcs.set(fun)
-	fun.Doc = nil // doc consumed - remove from AST
 }
 
 var (
@@ -552,11 +522,9 @@ var predeclaredTypes = map[string]bool{
 	"uintptr":    true,
 }
 
-func customizeRecv(m *Method, recvTypeName string, embeddedIsPtr bool, level int) *Method {
+func customizeRecv(f *Func, recvTypeName string, embeddedIsPtr bool, level int) *Func {
-	f := m.Func
-
 	if f == nil || f.Decl == nil || f.Decl.Recv == nil || len(f.Decl.Recv.List) != 1 {
-		return m // shouldn't happen, but be safe
+		return f // shouldn't happen, but be safe
 	}
 
 	// copy existing receiver field and set new type
@@ -579,13 +547,11 @@ func customizeRecv(m *Method, recvTypeName string, embeddedIsPtr bool, level int
 	// copy existing function documentation and set new declaration
 	newF := *f
 	newF.Decl = &newFuncDecl
-	newF.Recv = typ
+	newF.Recv = recvString(typ)
+	// the Orig field never changes
+	newF.Level = level
 
-	return &Method{
+	return &newF
-		Func:   &newF,
-		Origin: nil, // TODO(gri) set this
-		Level:  level,
-	}
 }
 
 // collectEmbeddedMethods collects the embedded methods from
@@ -690,15 +656,9 @@ func sortedKeys(m map[string]int) []string {
 // sortingName returns the name to use when sorting d into place.
 //
 func sortingName(d *ast.GenDecl) string {
-	// TODO(gri): Should actual grouping (presence of ()'s) rather
-	//            then the number of specs determine sort criteria?
-	//            (as is, a group w/ one element is sorted alphabetically)
 	if len(d.Specs) == 1 {
-		switch s := d.Specs[0].(type) {
+		if s, ok := d.Specs[0].(*ast.ValueSpec); ok {
-		case *ast.ValueSpec:
 			return s.Names[0].Name
-		case *ast.TypeSpec:
-			return s.Name.Name
 		}
 	}
 	return ""
@@ -739,22 +699,36 @@ func sortedTypes(m map[string]*baseType) []*Type {
 			Decl:    t.decl,
 			Consts:  sortedValues(t.values, token.CONST),
 			Vars:    sortedValues(t.values, token.VAR),
-			Funcs:   t.funcs.sortedFuncs(),
+			Funcs:   sortedFuncs(t.funcs),
-			Methods: t.methods.sortedMethods(),
+			Methods: sortedFuncs(t.methods),
 		}
 		i++
 	}
 
 	sortBy(
-		func(i, j int) bool {
+		func(i, j int) bool { return list[i].Name < list[j].Name },
-			if ni, nj := sortingName(list[i].Decl), sortingName(list[j].Decl); ni != nj {
-				return ni < nj
-			}
-			return list[i].order < list[j].order
-		},
 		func(i, j int) { list[i], list[j] = list[j], list[i] },
 		len(list),
 	)
 
 	return list
 }
+
+func sortedFuncs(m methodSet) []*Func {
+	list := make([]*Func, len(m))
+	i := 0
+	for _, m := range m {
+		// exclude conflict entries
+		if m.Decl != nil {
+			list[i] = m
+			i++
+		}
+	}
+	list = list[0:i]
+	sortBy(
+		func(i, j int) bool { return list[i].Name < list[j].Name },
+		func(i, j int) { list[i], list[j] = list[j], list[i] },
+		len(list),
+	)
+	return list
+}