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

x/tools/gopls: render doc links properly in hover #58352

Closed
ian-h-chamberlain opened this issue Feb 6, 2023 · 6 comments
Closed

x/tools/gopls: render doc links properly in hover #58352

ian-h-chamberlain opened this issue Feb 6, 2023 · 6 comments
Assignees
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@ian-h-chamberlain
Copy link

gopls version

Build info
----------
golang.org/x/tools/gopls v0.11.0
    golang.org/x/tools/gopls@v0.11.0 h1:/nvKHdTtePQmrv9XN3gIUN9MOdUrKzO/dcqgbG6x8EY=
    github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
    github.com/google/go-cmp@v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/exp@v0.0.0-20221031165847-c99f073a8326 h1:QfTh0HpN6hlw6D3vu8DAwC8pBIwikq0AI1evdm+FksE=
    golang.org/x/exp/typeparams@v0.0.0-20221031165847-c99f073a8326 h1:fl8k2zg28yA23264d82M4dp+YlJ3ngDcpuB1bewkQi4=
    golang.org/x/mod@v0.7.0 h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA=
    golang.org/x/sync@v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
    golang.org/x/sys@v0.2.0 h1:ljd4t30dBnAvMZaQCevtY0xLLD0A+bRZXbgLMLU1F/A=
    golang.org/x/text@v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
    golang.org/x/tools@v0.3.1-0.20221213193459-ca17b2c27ca8 h1:7/HkGkN/2ktghBCSRRgp31wAww4syfsW52tj7yirjWk=
    golang.org/x/vuln@v0.0.0-20221109205719-3af8368ee4fe h1:qptQiQwEpETwDiz85LKtChqif9xhVkAm8Nhxs0xnTww=
    honnef.co/go/tools@v0.3.3 h1:oDx7VAwstgpYpb3wv0oxiZlxY+foCpRAwY7Vk6XpAgA=
    mvdan.cc/gofumpt@v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
    mvdan.cc/xurls/v2@v2.4.0 h1:tzxjVAj+wSBmDcF6zBB7/myTy3gX9xvi8Tyr28AuQgc=
go: go1.19.1

go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ichamberlain/Library/Caches/go-build"
GOENV="/Users/ichamberlain/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ichamberlain/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/ichamberlain/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org"
GOROOT="/usr/local/Cellar/go/1.19.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.19.5/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/ichamberlain/Documents/go-sandbox/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wb/7n13y1cx0v1d28zb304bw8bm0000gn/T/go-build1610262310=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The new go1.19 doc comments are rendered to markdown, as mentioned in #54260, but the implementation seems to be incomplete to me. Mostly, linking to anything (package-local or otherwise) seems broken, and markdown links don't render as expected.

I believe all of these formats should be supported per https://go.dev/doc/comment#links

Sample program:

package main

import (
	"fmt"
)

// Struct is a struct.
//
// # Unexpected link behavior
//
//   - [fmt.Println] is linked to, but the link doesn't work (leads to non-existent "fmt")
//   - [fmt] same as above
//   - [Struct] isn't linked to
//   - [absolute markdown links] don't seem to work
//   - [relative markdown links] don't seem to work, although I'm
//     not entirely sure what the expected behavior here is
//
// [absolute markdown links]: https://example.com
// [relative markdown links]: #Example
//
//nolint:ignored
type Struct struct{}

func main() {
	fmt.Println()
}

What did you expect to see?

I would expect a hover comment to render something like this (using GitHub to render the expected markdown):

Struct is a struct.

Unexpected link behavior

Whether fmt.Println should link to pkg.go.dev or be a "go to definition" link is up for debate, but maybe making it configurable would be nice?

For local links like Struct I think the same question applies but I would lean towards "go to definition", it seems there has been some tangentially related discussion in #44890.

What did you see instead?

Screen Shot 2023-02-06 at 11 20 52

Clicking the fmt links leads to this:

Screen Shot 2023-02-06 at 11 18 54

Editor and settings

Version: 1.75.0
Commit: e2816fe719a4026ffa1ee0189dc89bdfdbafb164
Date: 2023-02-01T15:24:42.903Z
Electron: 19.1.9
Chromium: 102.0.5005.194
Node.js: 16.14.2
V8: 10.2.154.23-electron.0
OS: Darwin x64 21.6.0
Sandboxed: No

Relevant config:

{
    "go.buildOnSave": "off",
    "go.formatTool": "gofumpt",
    "go.lintOnSave": "file",
    "go.terminal.activateEnvironment": false,
    "go.toolsManagement.autoUpdate": true,
    "go.vetOnSave": "off",
    "go.testExplorer.enable": true,
    "go.useLanguageServer": true,
    "go.inlayHints.assignVariableTypes": false,
    "go.inlayHints.compositeLiteralFields": true,
    "go.inlayHints.compositeLiteralTypes": true,
    "go.inlayHints.constantValues": true,
    "go.inlayHints.functionTypeParameters": true,
    "go.inlayHints.parameterNames": true,
    "go.inlayHints.rangeVariableTypes": false,
    "gopls": {
        "ui.completion.usePlaceholders": true,
        "ui.navigation.importShortcut": "Definition",
        "ui.semanticTokens": true,
        "noSemanticString": true,
        "noSemanticNumber": true,
    },
}

Logs

N/A, I think — but I can capture some if necessary.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Feb 6, 2023
@gopherbot gopherbot added this to the Unreleased milestone Feb 6, 2023
@suzmue suzmue added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2023
@suzmue suzmue modified the milestones: Unreleased, gopls/later Feb 6, 2023
@findleyr
Copy link
Contributor

findleyr commented Feb 14, 2023

CC @pjweinb

Thank you for the report. I agree we should fix those links.

@findleyr findleyr modified the milestones: gopls/later, gopls/v0.12.0 Feb 14, 2023
@pjweinb
Copy link

pjweinb commented Feb 19, 2023

There are several different things going on in the example, some of which we can fix.

  1. The reason [absolute markdown links] doesn't work is that the block containing its definition is not recognized as a link definition block, because #Example is not recognized as a URL. Removing that line produces links for [absolute markdown links]
  2. We can generate pkg.go.dev links for fmt.Println and fmt.
  3. go doc does not seem to generate any kind of link for [Block]. What link might be generated, remembering that links are to URLs? ('go to definition' isn't implemented by a URL)

I don't think it matters, but vscode does not implement the same version of markdown as GitHub.

@gopherbot
Copy link

Change https://go.dev/cl/469715 mentions this issue: gopls/hove: add documentation links to hover

@ian-h-chamberlain
Copy link
Author

  1. The reason [absolute markdown links] doesn't work is that the block containing its definition is not recognized as a link definition block, because #Example is not recognized as a URL. Removing that line produces links for [absolute markdown links]

Is this expected behavior? I would think it should be possible to link to another anchor on the same page in the generated docs for https://pkg.go.dev (for example, something in fmt might link to #hdr-Printing or #example-Stringer)? I guess maybe this deserves a separate issue for go doc and/or pkgsite, since that behavior would determine how gopls should behave?

  1. go doc does not seem to generate any kind of link for [Block]. What link might be generated, remembering that links are to URLs? ('go to definition' isn't implemented by a URL)

This is a good point — in the context of the editor, "go to definition" would be convenient, but linking to https://pkg.go.dev for the current package would probably also work for packages that have docs published there. I would expect that https://pkg.go.dev normally generates these as URL fragments like mentioned above, but in the editor that wouldn't work as expected.

I looked into how another language server I use implemented "go to definition" for this particular use case, and it looks like they use a nonstandard LSP extension to provide the client with a clickable "go to definition" command 😞. So I guess short of implementing something like that, just using https://pkg.go.dev is probably the most reasonable option.

@pjweinb
Copy link

pjweinb commented Feb 21, 2023 via email

@ian-h-chamberlain
Copy link
Author

It is expected behavior. from tip.golang.org/doc/comment A span of unindented non-blank lines defines link targets when every line is of the form “[Text]: URL”.

Hmm, I guess more precisely what I meant was: is it expected that #Example is not recognized as a URL?

I guess technically it is a URI but not a URL (according to RFCs 3986 and 1738), but I think common usage would indicate that fragments can be considered as URLs (in fact, in the docs you linked the href attribute is used as an example of the ways these URLs can be used. <a href="#frag"> is a fairly common usage of this attribute).

All that to say — if fragments like this are never intended to be supported, then perhaps the documentation should be updated to explicitly indicate this (something like relative URLs without a scheme are not supported, or just an explicit mention of what definition is being used for "URL").

Or, if these kinds of links should be supported, then perhaps the behavior of go doc and gopls should be adjusted to support them.

@golang golang locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants