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: show full comments for constant or variable declaration #48200

Open
katsusan opened this issue Sep 5, 2021 · 3 comments
Open

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

katsusan opened this issue Sep 5, 2021 · 3 comments

Comments

@katsusan
Copy link

@katsusan katsusan commented Sep 5, 2021

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?

  1. Create file x.go in VSCode.
  2. 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.
)
@gopherbot gopherbot added this to the Unreleased milestone Sep 5, 2021
@findleyr findleyr removed this from the Unreleased milestone Sep 7, 2021
@findleyr findleyr added this to the gopls/unplanned milestone Sep 7, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Sep 7, 2021

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.

@stamblerre stamblerre changed the title x/tools/gopls/internal/lsp/source: show full comments for constant or variable declaration x/tools/gopls: show full comments for constant or variable declaration Sep 8, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Sep 17, 2021

We've just discussed this issue, and consensus was that formatting a truncated form of the full const decl (as shown in #48200 (comment)) makes sense. However, it isn't something we're likely to work on soon, so leaving this as 'help wanted'.

@katsusan
Copy link
Author

@katsusan katsusan commented Sep 21, 2021

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:

image


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants