x/tools/gopls: show full comments for constant or variable declaration

katsusan commented 3 years ago

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

➜  / go version
go version go1.17 linux/amd64
➜  / gopls version
golang.org/x/tools/gopls v0.7.1
    golang.org/x/tools/gopls@v0.7.1 h1:Mh3Z8Xcoq3Zy7ksSlwDV/nzQSbjFf06A+L+F8YHq55U=

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="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/katsu/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/katsu/go"
GOPRIVATE=""
GOPROXY="https://goproxy.cn"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build1623744664=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Create file x.go in VSCode.
Hover for O_TRUNC in following code.

package main
import "os"

func main() {
    os.OpenFile("notes.txt", os.O_RDWR|os.O_TRUNC, 0755)
}

What did you expect to see?

Show truncate regular writable file when opened, which tells more than the general comments for all flags.

const O_TRUNC int = 512
os.O_TRUNC on pkg.go.dev

Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system.

truncate regular writable file when opened.

What did you see instead?

const O_TRUNC int = 512
os.O_TRUNC on pkg.go.dev

Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system.

Appendix:

1. rpc trace log

[Trace - 17:54:02.715 PM] Sending request 'textDocument/hover - (2576)'.
Params: {"textDocument":{"uri":"file:///d%3A/Projects/leetcode/0warn.go"},"position":{"line":21,"character":42}}

[Trace - 17:54:02.719 PM] Received response 'textDocument/hover - (2576)' in 4ms.
Result: {"contents":{"kind":"markdown","value":"```go\nconst os.O_TRUNC int = 512\n```\n\n[`os.O_TRUNC` on pkg.go.dev](https://pkg.go.dev/os?utm_source=gopls#O_TRUNC)\n\nFlags to OpenFile wrapping those of the underlying system\\. Not all\nflags may be implemented on a given system\\.\n"},"range":{"start":{"line":21,"character":39},"end":{"line":21,"character":46}}}

2. possible reason

In following code it seems that currently gopls prefers spec.Doc > decl.Doc > spec.Comment, which means they will not coexist and gopls will lose some key information in hover result.

https://github.com/golang/tools/blob/master/internal/lsp/source/hover.go#L476-L498

func formatVar(node ast.Spec, obj types.Object, decl *ast.GenDecl) *HoverInformation {
    ...
    case *ast.ValueSpec:
        // Try to extract the field list of an anonymous struct
        if fieldList = extractFieldList(spec.Type); fieldList != nil {
            break
        }

        comment := spec.Doc
        if comment == nil {
            comment = decl.Doc
        }
        if comment == nil {
            comment = spec.Comment
        }

// some evidence from debugger
>>> p spec.Doc
$7 = (go/ast.CommentGroup *) 0x0
>>> p decl.Doc.List.array[0].Text
$9 = "// Flags to OpenFile wrapping those of the underlying system. Not all"
>>> p decl.Doc.List.array[1].Text
$10 = "// flags may be implemented on a given system."
>>> p spec.Comment.List.array[0].Text
$13 = "// truncate regular writable file when opened."

3. inconsistent behaviour

If you hover for O_RDONLY or O_APPEND, gopls will only return the comment of previous line, which doesn't tell useful information and leads to an inconsistent behaviour with other flags.

https://github.com/golang/go/blob/master/src/os/file.go#L71-L84

// Flags to OpenFile wrapping those of the underlying system. Not all
// flags may be implemented on a given system.
const (
    // Exactly one of O_RDONLY, O_WRONLY, or O_RDWR must be specified.
    O_RDONLY int = syscall.O_RDONLY // open the file read-only.
    O_WRONLY int = syscall.O_WRONLY // open the file write-only.
    O_RDWR   int = syscall.O_RDWR   // open the file read-write.
    // The remaining values may be or'ed in to control behavior.
    O_APPEND int = syscall.O_APPEND // append data to the file when writing.
    O_CREATE int = syscall.O_CREAT  // create a new file if none exists.
    O_EXCL   int = syscall.O_EXCL   // used with O_CREATE, file must not exist.
    O_SYNC   int = syscall.O_SYNC   // open for synchronous I/O.
    O_TRUNC  int = syscall.O_TRUNC  // truncate regular writable file when opened.
)

findleyr commented 3 years ago

Thanks for the request, and the analysis.

I think if there's a way of presenting all relevant doc comments such that no context is lost, we'd welcome it.

Considering the suggestion:

const O_TRUNC int = 512
os.O_TRUNC on pkg.go.dev

Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system.

truncate regular writable file when opened.

One problem I see with this is that the last sentence looks out of place: absent context it's not entirely clear what it's modifying. This isn't that egregious an example, but one can imagine other cases where the spec.Comment is even more out of place.

I wonder if we should format an abbreviated declaration. Perhaps something like this?

// Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system.
const (
  O_TRUNC int = 512 // truncate regular writable file when opened.
  // ...and N other values
)

We're unlikely to have time to prioritize this soon, but we'd welcome a contribution. However, before contributing we should decide on an ideal presentation here.

findleyr commented 3 years ago

We've just discussed this issue, and consensus was that formatting a truncated form of the full const decl (as shown in https://github.com/golang/go/issues/48200#issuecomment-914417728) makes sense. However, it isn't something we're likely to work on soon, so leaving this as 'help wanted'.

katsusan commented 3 years ago

Hi @findleyr, thanks for the reply. With respect to your suggestion, I think it's reasonable but there are some problems around:

// Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system. const ( O_TRUNC int = 512 // truncate regular writable file when opened. // ...and N other values )

1) if the ...and N other values means the extra declaration, the hover results may exceed what users want to see, especially when there are hundreds of declarations, eg. https://github.com/golang/tools/blob/master/internal/lsp/protocol/tsprotocol.go#L4914-L5366.

2) currently the hover results are presented with the format of [signature]\n[link]\n[documentation], do we need to keep following this style? For example, move the general comments down and keep the rest comments as they are, like this: // this may still lose some context but more descriptive than original one.

const (
    O_TRUNC int = 512 // truncate regular writable file when opened.
)
os.O_TRUNC on pkg.go.dev
Flags to OpenFile wrapping those of the underlying system. Not all flags may be implemented on a given system.

As there seems no relevant documentation which explicitly defines commentary specification, I use this picture to explain the corresponding comment scope in my view:

Based on above, I think we can change the signature of the const/var declaration to make it's presentation more comprehensive.

It's unrealistic to restraint user's comment behaviour without mechanical check but currently gopls can try to make all context present.

9/23 update: The old deisgn is too miscellaneous and may not deserve too much work to implement. In order to unify the presentation style, make decl.Doc always(if has) at the position of documentation and the rest like findleyr suggested.

I will work on it next few days and paste the capture of hover result here.

~~#### 1. only one of the three comments(decl.Doc/spec.Doc/spec.Comment) are added. In this case we can keep the original presentation.~~

~~#### 2. two of the three comments are added.~~ ~~#### 2.1 decl.Doc and one of the rest two are added. In this case like findleyr suggested, use abbreviated declaration with decl.Doc as documentation part.~~

~~#### 2.2 spec.Doc and spec.Comment are added. In this case we use spec.Doc as documentation part and keep spec.Comment in signature.~~

~~#### 3 all three comments are added. Similar to 2.2, use decl.Doc as Documentation and the rest in signature.~~

golang / go