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.

Sign up for GitHub

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

Jump to bottom

go/doc: CLI go doc makes a mess of code blocks in interface method docs #43188

Closed
benjaminjkraft opened this issue Dec 15, 2020 · 5 comments
Closed

go/doc: CLI go doc makes a mess of code blocks in interface method docs #43188

benjaminjkraft opened this issue Dec 15, 2020 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@benjaminjkraft
Copy link

benjaminjkraft commented Dec 15, 2020 •
edited

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

$ go version
go version go1.15.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/benkraft/.cache/go-build"
GOENV="/home/benkraft/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/benkraft/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/benkraft/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/benkraft/sdk/go1.15.5"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/benkraft/sdk/go1.15.5/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build758459235=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I ran go doc on an interface method with a code block, e.g. go doc context.Context.Done.

What did you expect to see?

I expected to see the relevant lines from the godoc, with the code block formatted reasonably:

// [...]
// Done is provided for use in select statements:
//
//	// Stream generates values with DoSomething and sends them to out
//	// until DoSomething returns an error or ctx.Done is closed.
//	func Stream(ctx context.Context, out chan<- Value) error {
//		for {
//			v, err := DoSomething(ctx)
//			if err != nil {
//				return err
//			}
//			select {
//			case <-ctx.Done():
//				return ctx.Err()
//			case out <- v:
//			}
//		}
//	}
//
// See https://blog.golang.org/pipelines for more examples of how to use
// a Done channel for cancellation.
func Done() <-chan struct{}

What did you see instead?

Instead, go doc made a mess of the formatting:

// [...]
// Done is provided for use in select statements:
//
// // Stream generates values with DoSomething and sends them to out
// // until DoSomething returns an error or ctx.Done is closed.
// func Stream(ctx context.Context, out chan<- Value) error {
// for {
// v, err := DoSomething(ctx)
// if err != nil {
// return err
// }
// select {
// case <-ctx.Done():
// return ctx.Err()
// case out <- v:
// }
// }
// }
//
// See https://blog.golang.org/pipelines for more examples of how to use
// a Done channel for cancellation.
func Done() <-chan struct{}

This also happens for user-defined interfaces, of course. It does not happen if you ask for the whole interface's doc (go doc context.Context) or in a struct field (go doc http.Request.Header) or method (go doc http.Request.Write). The web view, and pkg.go.dev, display the whole interface doc, and don't run into this issue either.
@agnivade
Copy link
Contributor

Thanks for filing! I will take a look.

@agnivade agnivade added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 15, 2020
@agnivade agnivade added this to the Backlog milestone Dec 15, 2020
@agnivade agnivade self-assigned this Dec 15, 2020
@agnivade
Copy link
Contributor

The issue is happening because struct methods return a *go/doc.Func but interface methods are just a *go/ast.Field. So the full comment of a struct method is contained in the Doc field, but interface comments gets split into individual *ast.Comments, per line, from the main CommentGroup which contains all comment lines.

So in the case of a interface method, a single line gets passed repeatedly to doc.ToText. And inside that, lineWrapper gets a single line, so the block returned is a para, which calls l.write, which gobbles up any tab characters using strings.Fields.

A full interface is also handled by doc.Type and gets printed properly by format.Node.

@griesemer - Do you think we should handle this in the doc.ToText side, or add some custom logic in cmd/doc to handle interface method comments better? This can get a bit hairy if a single ast.Comment.Text has multiple lines inside a CommentGroup.

@griesemer
Copy link
Contributor

So the problem here is that comments in an interface are not attached to the Doc field of an *ast.Field that represents the interface method?

Also, when I do go doc context.Context the output looks ok. Maybe, for go doc context.Context.Done we should actually show the Done method within it's interface (with the other methods stripped) and it might work as expected. It would also make it clear that Done belongs to an interface. As is, go doc context.Context.Done looks like it's a stand-alone function of sorts (at least in the output) which is misleading.

@agnivade
Copy link
Contributor

So the problem here is that comments in an interface are not attached to the Doc field of an *ast.Field that represents the interface method?

No that's not entirely it. The comments are attached, but they are split into individual ast.Comments for every line. And so, for every line, we call doc.ToText which misses the fact that there may be code blocks and prints everything with the same indent.

Also, when I do go doc context.Context the output looks ok.

Yep, that is because go/doc does not attach interface method to the doc.Type. And so trying to dig around the AST leads to different code paths. Maybe filtering the fields inside the full interface will lead to the unification of some code paths. I'll see what I can do.

@gopherbot
Copy link

Change https://golang.org/cl/279433 mentions this issue: cmd/doc: properly display interface methods

@golang golang locked and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@benjaminjkraft @agnivade @griesemer @gopherbot