search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.62k stars 17.61k forks source link

x/tools/godoc: Anchors for variables and constants no longer added #19894

Closed Kapeli closed 7 years ago

Kapeli commented 7 years ago

Before 1.8.1, I was able to go to https://golang.org/pkg/archive/tar/#TypeBlock and it would scroll to the relevant constant. This no longer works.

Somewhat related: this makes it impossible for Dash to index constants and variables in the Go docs.

bradfitz commented 7 years ago

There was nothing in Go 1.8.1 changing godoc.

Or do you mean before Go 1.8?

Even then, I don't think this is true. We added #anchors in Go 1.8 but didn't change or remove any.

Kapeli commented 7 years ago

Ah, you're right. The change seems to have happened with the 1.8 release, not 1.8.1.

If you go to this snapshot from Feb 2017 (Go 1.7.5) you'll be able to jump to the TypeBlock constant: https://web.archive.org/web/20170211005446/https://golang.org/pkg/archive/tar/#TypeBlock.

This doesn't work with the live version anymore: https://golang.org/pkg/archive/tar/#TypeBlock.

Notice the span id in the screenshot below:

screen shot 2017-04-08 at 19 34 23

There's no more span id in the latest docs:

screen shot 2017-04-08 at 19 35 08

bradfitz commented 7 years ago

Hm, strange.

It'd be nice if somebody could bisect (or manually review godoc commits--- there aren't many) and figure out when we (I?) broke it.

/cc @odeke-em @shurcooL

dmitshur commented 7 years ago

I bisected x/tools/cmd/godoc.

The problem starts with https://github.com/golang/tools/commit/e1bdc76d0461f7041136c70e0a1356ea0f5e01d0.

With the parent commit, the generated HTML looks like this:

image

With https://github.com/golang/tools/commit/e1bdc76d0461f7041136c70e0a1356ea0f5e01d0, you get:

image

I'm not up to date with how cmd/godoc handles anchor links in the latest version. I recall seeing some changes that pushed their generation on the frontend via JavaScript. So at this point, I'm not sure if the anchor is supposed to be generated on backend or frontend. I might be misremembering this (perhaps that was for other pages of golang.org, not /pkg/ pages).

/cc @jayconrod

dmitshur commented 7 years ago

Here's a diff of the generated HTML at localhost:6060/pkg/archive/tar/:

```diff @@ -333,40 +333,40 @@

Header type flags. 

const (
-    TypeReg           = '0'    // regular file
-    TypeRegA          = '\x00' // regular file
-    TypeLink          = '1'    // hard link
-    TypeSymlink       = '2'    // symbolic link
-    TypeChar          = '3'    // character device node
-    TypeBlock         = '4'    // block device node
-    TypeDir           = '5'    // directory
-    TypeFifo          = '6'    // fifo node
-    TypeCont          = '7'    // reserved
-    TypeXHeader       = 'x'    // extended header
-    TypeXGlobalHeader = 'g'    // global extended header
-    TypeGNULongName   = 'L'    // Next file has a long name
-    TypeGNULongLink   = 'K'    // Next file symlinks to a file w/ a long name
-    TypeGNUSparse     = 'S'    // sparse file
+    TypeReg           = '0'    // regular file
+    TypeRegA          = '\x00' // regular file
+    TypeLink          = '1'    // hard link
+    TypeSymlink       = '2'    // symbolic link
+    TypeChar          = '3'    // character device node
+    TypeBlock         = '4'    // block device node
+    TypeDir           = '5'    // directory
+    TypeFifo          = '6'    // fifo node
+    TypeCont          = '7'    // reserved
+    TypeXHeader       = 'x'    // extended header
+    TypeXGlobalHeader = 'g'    // global extended header
+    TypeGNULongName   = 'L'    // Next file has a long name
+    TypeGNULongLink   = 'K'    // Next file symlinks to a file w/ a long name
+    TypeGNUSparse     = 'S'    // sparse file
 )

Variables

var (
-    ErrWriteTooLong    = errors.New("archive/tar: write too long")
-    ErrFieldTooLong    = errors.New("archive/tar: header field too long")
-    ErrWriteAfterClose = errors.New("archive/tar: write after close")
+    ErrWriteTooLong    = errors.New("archive/tar: write too long")
+    ErrFieldTooLong    = errors.New("archive/tar: header field too long")
+    ErrWriteAfterClose = errors.New("archive/tar: write after close")
 )
var (
-    ErrHeader = errors.New("archive/tar: invalid tar header")
+    ErrHeader = errors.New("archive/tar: invalid tar header")
 )
@@ -409,11 +409,11 @@

func FileInfoHeader

- 
func FileInfoHeader(fi os.FileInfo, link string) (*Header, error)
+ 
func FileInfoHeader(fi os.FileInfo, link string) (*Header, error)

FileInfoHeader creates a partially-populated Header from fi. If fi describes a symlink, FileInfoHeader records link as the link target. If fi describes a directory, a slash is appended to the name. Because os.FileInfo's Name method returns only the base name of @@ -428,11 +428,11 @@

func (*Header) FileInfo

- 
func (h *Header) FileInfo() os.FileInfo
+ 
func (h *Header) FileInfo() os.FileInfo

FileInfo returns an os.FileInfo for the Header.

@@ -467,11 +467,11 @@

func NewReader

- 
func NewReader(r io.Reader) *Reader
+ 
func NewReader(r io.Reader) *Reader

NewReader creates a new Reader reading from r.

@@ -481,11 +481,11 @@

func (*Reader) Next

- 
func (tr *Reader) Next() (*Header, error)
+ 
func (tr *Reader) Next() (*Header, error)

Next advances to the next entry in the tar archive.

io.EOF is returned at the end of the input. @@ -497,11 +497,11 @@

func (*Reader) Read

- 
func (tr *Reader) Read(b []byte) (int, error)
+ 
func (tr *Reader) Read(b []byte) (int, error)

Read reads from the current entry in the tar archive. It returns 0, io.EOF when it reaches the end of that entry, until Next is called to advance to the next entry.

@@ -543,11 +543,11 @@

func NewWriter

- 
func NewWriter(w io.Writer) *Writer
+ 
func NewWriter(w io.Writer) *Writer

NewWriter creates a new Writer writing to w.

@@ -557,11 +557,11 @@

func (*Writer) Close

- 
func (tw *Writer) Close() error
+ 
func (tw *Writer) Close() error

Close closes the tar archive, flushing any unwritten data to the underlying writer.

@@ -571,11 +571,11 @@

func (*Writer) Flush

- 
func (tw *Writer) Flush() error
+ 
func (tw *Writer) Flush() error

Flush finishes writing the current file (optional).

@@ -584,11 +584,11 @@

func (*Writer) Write

- 
func (tw *Writer) Write(b []byte) (n int, err error)
+ 
func (tw *Writer) Write(b []byte) (n int, err error)

Write writes to the current entry in the tar archive. Write returns the error ErrWriteTooLong if more than hdr.Size bytes are written after WriteHeader.

@@ -599,11 +599,11 @@

func (*Writer) WriteHeader

- 
func (tw *Writer) WriteHeader(hdr *Header) error
+ 
func (tw *Writer) WriteHeader(hdr *Header) error

WriteHeader writes hdr and prepares to accept the file's contents. WriteHeader calls Flush if it is not the first header. Calling after a Close will return ErrWriteAfterClose.

```

It affects quite a few things, not just constants. **Edit:** Some of the changed lines have added `a href`s, those are fine. It's the missing `span id`s that are the problem.
bradfitz commented 7 years ago

@shurcooL, thanks!

/cc @jayconrod @alandonovan

jayconrod commented 7 years ago

I looked into this today. After my change, linksFor generates empty links for const and var declarations. Previously, it included the identifier name. LinkifyText skips over these, since it doesn't know what id to write.

I should have a fix for this tomorrow.

bradfitz commented 7 years ago

Thanks!

gopherbot commented 7 years ago

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

bradfitz commented 7 years ago

Reopening for cherry-pick.

Kapeli commented 7 years ago

Any chance the golang.org docs could be regenerated using the fixed version of godoc?

bradfitz commented 7 years ago

@Kapeli, yes, that's what I mean by "reopening for cherry-pick".

dmitshur commented 7 years ago

@bradfitz, you said "reopening", but the issue continues to be closed. Is there a different meaning to the word "reopening", or did you forget to actually reopen it?

bradfitz commented 7 years ago

Forgot.

broady commented 7 years ago

Cherry-pick doesn't apply cleanly. I've asked @jayconrod to take a look.

dmitshur commented 7 years ago

Cherry-pick doesn't apply cleanly.

https://github.com/golang/tools/commit/3bcb6efb399c5289003b99a1c165bd9797ff406c (the commit that closed this issue) is a minor patch/bug fix for https://github.com/golang/tools/commit/e1bdc76d0461f7041136c70e0a1356ea0f5e01d0, where this regression was introduced. But the corresponding issue for that commit is milestoned for Go 1.9. If it's not a part of 1.8 release, then this fixup shouldn't be needed either.

bradfitz commented 7 years ago

@shurcooL, thanks!

jayconrod commented 7 years ago

Confirming what @shurcooL said, this bug is not on the 1.8 branch and does not need to be cherry-picked.