x/tools/cmd/godoc: enable links to fields of structs #16753

Closed
bradfitz opened this Issue Aug 16, 2016 · 30 comments

Comments

Projects
None yet
9 participants
Owner

bradfitz commented Aug 16, 2016

I would like to be able to link to:

https://golang.org/pkg/net/http/#Request.Cancel

And have the link go to the top of the comment immediately before the "Cancel" field in the Request struct.

Similarly, I would like to link the English docs at the bottom of Transport, after the fields:

https://golang.org/pkg/net/http/#Transport.doc

And have it go to...

"Transport is an implementation of RoundTripper ..."

Currently my only alternative links for both are quite some distance from the text I'd like people to read.

/cc @broady @adg @shurcooL

bradfitz added the HelpWanted label Aug 16, 2016

bradfitz added this to the Go1.8 milestone Aug 16, 2016

Owner

bradfitz commented Aug 16, 2016

/cc @odeke-em too

Member

broady commented Aug 16, 2016 edited

The Transport.doc one is pretty bad. I don't really like the solution. Maybe the doc should be above it, anyway? I'm unsure why godoc had it swapped (compared to how it is in source) in the first place.

Owner

bradfitz commented Aug 16, 2016

Yes. Perhaps. There is also #16728 open for a very similar case.

Member

shurcooL commented Aug 16, 2016

Since you didn't mention it, are you aware of the existing related behavior of godoc.org (gddo project)?

https://godoc.org/net/http#Request.Cancel

This looks like a request to backport that, plus tweaks to where on the page the anchors position you.

Member

shurcooL commented Aug 16, 2016

Maybe the doc should be above it, anyway? I'm unsure why godoc had it swapped (compared to how it is in source) in the first place.

That's a very good question and one I've been pondering about for some months now. I'd love to know some insight on that from anyone familiar, but it might be outside of scope of this issue.

Contributor

luigi-riefolo commented Sep 6, 2016

Is anyone already working on this issue/feat?

Owner

bradfitz commented Sep 6, 2016

Nobody has said so here. Feel free to work on it.

Contributor

appleby commented Sep 6, 2016

Hi @luigi-riefolo. I looked into this briefly last week, but have no
plans to submit a changelist. Here are a few observations that might
save you some time in case you are new to the code, as I am:

  • For adding the link to doc section, have a look at
    x/tools/godoc/static/package.html.
  • In order for changes to package.html to take effect, you will need
    to re-generate the file static.go in the same directory, either by
    running go generate or else by running the makestatic command
    (located in the same directory).
  • You might consider renaming the #Transport.doc link to something
    like #Transport-doc. The reason being that there is a small chance
    of a name collision if the struct has a field named doc and the
    client passes the ?m=all query parameter, which tells godoc to
    also show unexported identifiers. This occurs, for example, in the namedType struct of the go/doc pkg.
  • As for adding links to the struct fields, a good place to start is
    the file x/tools/godoc/linkify.go. Specifically the functions
    LinkifyText
    and
    linksFor.
    These functions are responsible for (among other things) adding
    anchor links for const and var declarations in the current
    implementation.

Hope this is helpful.

Contributor

luigi-riefolo commented Sep 6, 2016

Hi @appleby, thank you for the info, very useful!
I've started working on the links to the struct fields, here's a simple test:
page.html.zip

Have a look at file:///page.html#Test, file:///page.html#Test.What, file:///page.html#Test.Str.
Simply adding an id to the comment span seems enough.

I'll have a look at x/tools/godoc/linkify.go and identify a strategy to automatically add the id.

Please let me know if you guys have any comments or questions.

Contributor

appleby commented Sep 6, 2016

If you've figured out a way to add the id to the comment span, you should probably just go that route and ignore what I said about LinkifyText. LinkifyText is currently only responsible for adding links that anchor directly to an identifier. For example, when you link to https://golang.org/pkg/net/http/#ErrContentLength, it's anchored at the identifier, not the associated comment block.

I agree that adding the anchor at the top of the comment block is nicer from the user's perspective, but it wasn't clear to me how to get the context required to generate the id at the time the comments are rendered.

One problem with the approach of adding an id attribute to the first comment span tag is that it doesn't scale when multiple fields share the same doc comment. Consider, for example:

type Foo struct {
    // T1, T2, and T3 are templates.
    T1, T2, T3 *text.Template
}

In a case like this, if you go the route of adding an id attribute to the comment tag, you'd have to pick one of the identifiers to make linkable. Alternatively, you could possibly generate separate tags for the anchors prior to writing the comment block.

Another edge-case to keep in mind is fields without an associated doc comment. For example, see the first few fields of the godoc.Presentation struct.

Contributor

luigi-riefolo commented Sep 7, 2016

@appleby great examples. I'll keep them in mind.
If you can find more of those then please let us know.

Contributor

luigi-riefolo commented Sep 15, 2016 edited

Here's the proposal only for the adding bookmarks to IDENTs (i.e. structs, vars, etc.):
It's my first proposal ever, so any comment or advice is really appreciated.

  1. In LinkifyText (linkify.go) call BookmarkText located in a new file bookmark.go:
bookmarksWriter := BookmarkText(text)
idents := tokenSelection(text, token.IDENT)
comments := tokenSelection(text, token.COMMENT)
FormatSelections(w, text, linkWriter, idents, selectionTag, bookmarksWriter, comments)

Here's its signature:
func BookmarkText(text []byte) (bw BookmarkWriter)

  1. BookmarkText identifies all the IDENTs using a helper function:

func createBookmarks(text []byte) (bookmarkMap map[int]bookmarkList)
createBookmarks parses the source code and identifies every IDENT that needs a bookmark.
If the first line of a comment is found above its relative IDENT, then a bookmark struct is pushed into a map using the comment line number.
e.g.:

      // Comment
      n := 1

If there isn't a comment block above the IDENT, or the comment is on the right-side of the IDENT, then the IDENT line number is used as key.
e.g.:
n := 1 // Comment

  1. At this point in:
    func FormatSelections(w io.Writer, text []byte, lw LinkWriter, links Selection, sw SegmentWriter, bw BookmarkWriter, selections ...Selection)

each time a token is processed a call to:
bw(w, lastOffs, start)
is made. The BookmarkWriter function checks if a lastOffs key exists in the bookmarkMap and prints its related bookmark to w.

Here's a test page:
page.html.zip

Try:

file:///page.html#ErrBodyNotAllowed
file:///page.html#SingleBlock.T3
file:///page.html#Test.What

Note: the test page has each main comment above the its relative node as pointed in #16728. To achieve this changes have been made in package.html, such as:

<h2 id="pkg-constants">Constants</h2>
    {{range .}}
        {{comment_html .Doc}}
        <pre>{{node_html $ .Decl true}}</pre>
    {{end}}

Each {{comment_html .Doc}} has been moved above <pre>{{node_html $ .Decl true}}</pre>.

Please let me know if I need to file a formal proposal or this is sufficient.

/cc @bradfitz @broady @shurcooL @appleby @adg

Member

shurcooL commented Sep 16, 2016 edited

Here's a test page:
page.html.zip

That page had a Uncaught SyntaxError: Unexpected token ; error, and it wasn't being interpreted as UTF-8. I've fixed the JS error and added <meta charset="utf-8">.

I've uploaded it here for easier viewing:

http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html
http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html#ErrBodyNotAllowed
http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html#SingleBlock.T3
http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html#Test.What

I've only looked briefly, but what I can see so far, this looks very reasonable to me.

Contributor

luigi-riefolo commented Sep 18, 2016

This is my latest test page before uploading the code for review.

Please have a look at some of these examples:

file:///page.html#NestedStuct.Nest1
file:///page.html#Request.Proto
file:///page.html#Err2
file:///page.html#File.io.Reader

and let me know if I missed something or there's any error.

Thanks.

/cc @bradfitz @broady @shurcooL @appleby @adg

Member

shurcooL commented Sep 18, 2016 edited

I don't see any issues.

At first I thought #File.io.Reader looked questionable, and considered if it can be just #File.Reader. I wasn't sure if the package name where io.Reader comes from there was needed.

It's not needed for structs, because you can't do:

type Some struct {
    io.Reader
    foo.Reader
}

// Build error: duplicate field Reader

However, it's valid for interfaces, as long as the embedded interfaces contain non-repeating methods. So, this is possible:

type Some interface {
    io.Reader
    foo.Reader
}

// No build error (as long as there are no duplicate methods in those 2 interfaces)

So, unfortunately, it seems that it's needed to keep the package name io there.

Contributor

appleby commented Sep 18, 2016

I also noticed a minor thing about the #File example.

In the current docs, both the package and the type are linked separately. For example, the io.Reader in

type File interface {
    io.Closer
    io.Reader
    io.Seeker
    Readdir(count int) ([]os.FileInfo, error)
    Stat() (os.FileInfo, error)
}

"io" links to pkg io and "Reader" links to io.Reader.

https://golang.org/pkg/net/http/#File

In your example, the link to the package has been dropped, though honestly I'm not sure how useful it is to have that link to the package when you most likely to want to go straight to io.Reader anyway...

The case of nested structs is something I considered when I looked into this as well. My conclusion was that I would punt and only add links for the first-level fields of a named type. My reasoning was that nested structs are probably rare (didn't verify this), and you can get into nasty edge cases (admittedly contrived) like the following:

type Nested struct {
    Nest1, Nest2 struct {
        X int
    }
}

Would X in the above example get labeled as both Nested.Nest1.X and Nested.Nest2.X?

What about (god forbid):

type Nested struct {
    Nest1, Nest2 struct {
        Nest3, Nest4 struct {
            X int
        }
    }
}

How is X labeled in this case?

To be clear, I'm not advocating that you actually need to handle such unlikely cases. Just throwing it out there for consideration.

Contributor

appleby commented Sep 18, 2016

Thinking about this a bit more, I guess you can safely ignore my nested structs examples from my last comment. The point I was trying to make was that nested struct fields don't really have a canonical name and therefore it might not make sense to link to them, but I guess there is no real problem with a field having multiple ways to link to it. Especially since you are unlikely to encounter multiple levels of nesting in any real example.

Contributor

luigi-riefolo commented Sep 26, 2016

@appleby, @shurcooL thank you for the feedback, it was very helpful.

In your example, the link to the package has been dropped

That's because I did not import all the requested packages in my source example. In this case the io package.

In this new test page, a struct or interface like:

type Nested struct {
    Nest1, Nest2 struct {
        X int
    }
}

will have two bookmarks respectively for Nest1 and Nest2:
Nested.Nest1, Nested.Nest2 ;
and two bookmarks for X:
Nested.Nest1.X, Nested.Nest2.X.

We limit the nesting up to 2 levels, as higher levels would be a very bad practice, thus the struct:

type Nested struct {
    Nest1, Nest2 struct {
        Nest3, Nest4 struct {
            X int
        }
    }
}

will only have bookmarks up to Nest4, excluding X because it has a nesting level of 3.

Please let me know what you think or if you want to make any changes.

Contributor

appleby commented Sep 26, 2016

For what it's worth, the latest test page LGTM. However, I am not a maintainer and not someone you need to convince. Sorry if that was not clear from my previous comments.

Nice work getting the links to point to the associated comment blocks. Seems like a usability win for const/var declarations as well.

Contributor

rsc commented Oct 10, 2016

I guess the decision here is whether to the godoc changes in the file mock are OK? That's probably up to @alandonovan or @griesemer.

Contributor

alandonovan commented Oct 13, 2016

In @luigi-riefolo's file mock from Sep 25, the empty spans with ids preceding each struct field doc comment look good to me. However, the page seems to lack all the functions and methods of the HTTP package. Were these removed for expedience?

Contributor

luigi-riefolo commented Oct 14, 2016

@alandonovan that is just a test page that includes non-existing data structures and part of the HTTP Request. I've tried to include as many examples as possible. Let me know if you have any additional ones.

Contributor

alandonovan commented Oct 14, 2016

Thanks Luigi. All of the fragment IDs look good, with the following exception.

I disagree with Dmitri about the need for links such as #File.io.Closer. Despite their similar syntaxes, there's a crucial difference between embedding in structs and in interfaces. In a struct, an embedded field type T struct { pkg.X } is both a use of a type pkg.X and a declaration of a field X, and it is the second of these two facts that warrants a fragment #T.X. In contrast, an embedded interface type I interface { pkg.X } is still a use of the type pkg.X but it is not a declaration of anything; it doesn't introduce any new names. I think Godoc should define URLs only for things that have names in Go.

Contributor

luigi-riefolo commented Oct 14, 2016

Thank you Alan for the feedback.
If anyone else agree then I'll remove the tag from the inner IDENTs within interfaces.

If you have any additional comments/questions, then please let me know.

Note: the code for bookmarking IDENTs is ready and I'm just polishing it for the peer review, but I need some more time as I'm very busy job hunting.

Member

shurcooL commented Oct 15, 2016 edited

I agree with what Alan said above. That rationale is very good.

I disagree with Dmitri about the need for links such as #File.io.Closer.

My comment was not about the need for that link, it was about the need for io component if that link were to be included. I think you have described in a good way why the whole thing should not be linked. That's an even better solution.

CL https://golang.org/cl/32153 mentions this issue.

CL https://golang.org/cl/32154 mentions this issue.

Contributor

luigi-riefolo commented Oct 27, 2016

I've just committed the changes (5 mod files, 1 new file) and don't why got two different revision number 32153 and 32154. Does anyone know why?
Furthermore I couldn't push some refs:

# git push origin HEAD:refs/for/master
Counting objects: 9, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.52 KiB | 0 bytes/s, done.
Total 9 (delta 8), reused 0 (delta 0)
remote: Resolving deltas: 100% (8/8)
remote: Processing changes: new: 1, done    
remote: 
remote: New Changes:
remote:   https://go-review.googlesource.com/32154 x/tools/cmd/godoc: add links to fields
remote: 
To https://go.googlesource.com/tools
 * [new branch]      HEAD -> refs/for/master
Counting objects: 16868, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (5268/5268), done.
Writing objects: 100% (16868/16868), 7.65 MiB | 89.00 KiB/s, done.
Total 16868 (delta 11370), reused 16734 (delta 11255)
remote: Resolving deltas: 100% (11370/11370)
remote: Counting objects: 16718, done
remote: Processing changes: refs: 1, done    
To https://go-review.googlesource.com/go
 ! [remote rejected] HEAD -> refs/for/master (no common ancestry)
error: failed to push some refs to 'https://go-review.googlesource.com/go'

Any ideas?

@rsc rsc modified the milestone: Go1.9, Go1.8 Nov 11, 2016

bradfitz self-assigned this Nov 29, 2016

CL https://golang.org/cl/33690 mentions this issue.

CL https://golang.org/cl/33755 mentions this issue.

@gopherbot gopherbot pushed a commit to golang/tools that referenced this issue Dec 1, 2016

@bradfitz bradfitz godoc: update struct field anchor code
Now without regexps and allocations.

And also match comments like:

    // Foo, if non-nil, ...

The comma confused the old pattern.

Updates golang/go#16753

Change-Id: I9016ee7b5933ea343950a39989952804c74a598b
Reviewed-on: https://go-review.googlesource.com/33755
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
e5f9a3d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment