Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/doc: factory methods of a type are not being shown with other methods #27928

Closed
DeedleFake opened this issue Sep 28, 2018 · 6 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@DeedleFake
Copy link

What version of Go are you using (go version)?

go version go1.11 windows/amd64

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOCACHE=$HOME\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=$HOME\Devel\Go
set GOPROXY=
set GORACE=
set GOROOT=$HOME\scoop\apps\go\current
set GOTMPDIR=
set GOTOOLDIR=$HOME\scoop\apps\go\current\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$HOME\AppData\Local\Temp\go-build398922042=/tmp/go-build -gno-record-gcc-switches

What did you do?

Ran go doc http.request.

What did you expect to see?

Snippet from go1.10.4 doc http.request's output:

    A Request represents an HTTP request received by a server or to be sent by a
    client.

    The field semantics differ slightly between client and server usage. In
    addition to the notes on the fields below, see the documentation for
    Request.Write and RoundTripper.


func NewRequest(method, url string, body io.Reader) (*Request, error)
func ReadRequest(b *bufio.Reader) (*Request, error)
func (r *Request) AddCookie(c *Cookie)
func (r *Request) BasicAuth() (username, password string, ok bool)

What did you see instead?

The same snippet with Go 1.11:

    A Request represents an HTTP request received by a server or to be sent by a
    client.

    The field semantics differ slightly between client and server usage. In
    addition to the notes on the fields below, see the documentation for
    Request.Write and RoundTripper.


func (r *Request) AddCookie(c *Cookie)
func (r *Request) BasicAuth() (username, password string, ok bool)
func (r *Request) Context() context.Context
func (r *Request) Cookie(name string) (*Cookie, error)
@agnivade
Copy link
Contributor

You mean they are not being shown at all.

@agnivade agnivade changed the title cmd/doc: constructors are not being sorted in with methods cmd/doc: factory methods of a type are not being shown with other methods Sep 29, 2018
@agnivade agnivade added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 29, 2018
@agnivade agnivade added this to the Go1.12 milestone Sep 29, 2018
@agnivade
Copy link
Contributor

There is something funky going on here. The go/doc package is not returning the factory methods of the Request type at all. Whereas the 1.10 version does. This seems to be a go/doc issue rather than cmd/doc issue.

Here is a sample reproducer -

package main

import (
	"fmt"
	"go/doc"
	"go/parser"
	"go/token"
	"log"
	"os"
)

func main() {
	include := func(info os.FileInfo) bool {
		return true
	}
	fs := token.NewFileSet()
	pkgs, err := parser.ParseDir(fs, "/usr/local/go/src/net/http", include, parser.ParseComments)
	if err != nil {
		log.Fatal(err)
	}
	astPkg := pkgs["http"]
	docPkg := doc.New(astPkg, "net/http", doc.AllDecls)
	for _, f := range docPkg.Funcs {
		if f.Name == "NewRequest" {
			fmt.Printf("got NewRequest: %v\n", f.Decl)
		}
	}
	for _, t := range docPkg.Types {
		if t.Name == "Request" {
			fmt.Printf("got type Request %v\n", t.Funcs)
			for _, f := range t.Funcs {
				fmt.Printf("func %s\n", f.Name)
			}
		}
	}
}

Output with 1.11 -

got NewRequest: &{<nil> <nil> NewRequest 0xc000cd8e40 <nil>} <-- This got moved to a Func 
got type Request [0xc000f11a90 0xc000f11ae0 0xc000688960 0xc0006889b0 0xc000688a00 0xc000ed8c80]
func dummyReq <---- Only internal methods 
func dummyReq11
func dummyRequest
func dummyRequestWithBody
func dummyRequestWithBodyNoGetBody
func http2requestWithContext

Output with 1.10.3

got type Request [0xc4210eef00 0xc4210ef180 0xc4210eff90 0xc4210fa000 0xc4211361e0 0xc421136230 0xc421136280 0xc420f7f180 0xc42105ee60 0xc4210ef1d0]
func NewRequest  <--- factory methods get matched
func ReadRequest
func dummyReq
func dummyReq11
func dummyRequest
func dummyRequestWithBody
func dummyRequestWithBodyNoGetBody
func http2requestWithContext
func http2shouldRetryRequest
func readRequest

So, it seems in 1.11, the factory methods detach themselves from the method and remain as a top-level Func.

But then how does godoc work ? https://tip.golang.org/pkg/net/http/#NewRequest

Both are consumers of go/doc.

@griesemer - Any ideas here ?

@griesemer
Copy link
Contributor

@agnivade It seems to me this is because of your change https://golang.org/cl/105575 (commit 6079536) which doesn't associate functions with more than one return value to the receiver type.

607953609c7 src/go/doc/reader.go     (Agniva De Sarker        2018-04-08 16:07:25 +0530 396)            var typ *namedType // type to associate the function with
607953609c7 src/go/doc/reader.go     (Agniva De Sarker        2018-04-08 16:07:25 +0530 397)            numResultTypes := 0
607953609c7 src/go/doc/reader.go     (Agniva De Sarker        2018-04-08 16:07:25 +0530 398)            for _, res := range fun.Type.Results.List {

Haven't investigated godoc. Feel free to investigate.

It seems to me that change https://golang.org/cl/105575 was too aggressive and that it probably shouldn't consider an error return when making the decision whether to associate a method or not.

@agnivade
Copy link
Contributor

agnivade commented Oct 2, 2018

Yes, I had thought about that. But then godoc uses the same thing, so it should also have broken. Somehow that doesn't seem to be the case. I will check.

@agnivade
Copy link
Contributor

Finally figured it out. This is an issue with the doc.Mode that gets passed to doc.New. cmd/doc uses doc.AllDecls, and godoc uses 0. On passing 0, the factory methods get attached, but on passing doc.AllDecls, they do not get attached because r.Visible(n) evaluates to true and the logic breaks.

So we need to fine tune the logic a bit. But at least the mystery is solved on why cmd/doc and godoc were behaving differently.

@gopherbot
Copy link

Change https://golang.org/cl/141617 mentions this issue: go/doc: tune factory method association logic

@golang golang locked and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants