search

golang / go

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

x/tools/gopls/internal/lsp/source: different hover for same type #47098

Closed katsusan closed 2 years ago

katsusan commented 3 years ago

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

➜  ~ go version
go version go1.16.5 linux/amd64

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
➜  ~ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/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.16.5"
GCCGO="/usr/local/bin/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-build3210144513=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Version of gopls:

➜  ~ gopls version
golang.org/x/tools/gopls v0.7.0
    golang.org/x/tools/gopls@v0.7.0 h1:JQBHW81Gsyim6iDjUwGoPeSpXrSqwen3isPJLfDfaYU=

Editor: VSCode Reproducing steps:

  1. Open /usr/local/go/src/runtime/panic.go in VSCode.
  2. Hover for _panic in following code. https://github.com/golang/go/blob/7677616a263e8ded606cc8297cb67ddc667a876e/src/runtime/panic.go#L871
  3. Hover for _panic in another line. https://github.com/golang/go/blob/7677616a263e8ded606cc8297cb67ddc667a876e/src/runtime/panic.go#L916

What did you expect to see?

step 3 give the same output as step 2.

What did you see instead?

step 2 gives:

type _panic struct {
    argp      unsafe.Pointer // pointer to arguments of deferred call run during panic; cannot move - known to liblink
    arg       interface{}    // argument to panic
    link      *_panic        // link to earlier panic
    pc        uintptr        // where to return to in runtime if this panic is bypassed
    sp        unsafe.Pointer // where to return to in runtime if this panic is bypassed
    recovered bool           // whether this panic is over
    aborted   bool           // the panic was aborted
    goexit    bool
}
A _panic holds information about an active panic.

A _panic value must only ever live on the stack.

The argp and link fields are stack pointers, but don't need special handling during stack growth: because they are pointer-typed and _panic values only live on the stack, regular stack pointer adjustment takes care of them.

step 3 gives:

type _panic struct{argp unsafe.Pointer; arg interface{}; link *_panic; pc uintptr; sp unsafe.Pointer; recovered bool; aborted bool; goexit bool}

Appendix

  1. Here is the relevant rpc trace log.

// step 2

[Trace - 02:51:14.391 AM] Sending request 'textDocument/hover - (27)'.
Params: {"textDocument":{"uri":"file:///usr/local/go/src/runtime/panic.go"},"position":{"line":870,"character":27}}

[Trace - 02:51:14.399 AM] Received response 'textDocument/hover - (27)' in 7ms.
Result: {"contents":{"kind":"markdown","value":"```go\ntype _panic struct {\n\targp      unsafe.Pointer // pointer to arguments of deferred call run during panic; cannot move - known to liblink\n\targ       interface{}    // argument to panic\n\tlink      *_panic        // link to earlier panic\n\tpc        uintptr        // where to return to in runtime if this panic is bypassed\n\tsp        unsafe.Pointer // where to return to in runtime if this panic is bypassed\n\trecovered bool           // whether this panic is over\n\taborted   bool           // the panic was aborted\n\tgoexit    bool\n}\n```\n\nA \\_panic holds information about an active panic\\.\n\nA \\_panic value must only ever live on the stack\\.\n\nThe argp and link fields are stack pointers, but don\\'t need special\nhandling during stack growth\\: because they are pointer\\-typed and\n\\_panic values only live on the stack, regular stack pointer\nadjustment takes care of them\\.\n"},"range":{"start":{"line":870,"character":24},"end":{"line":870,"character":30}}}

// step 3

[Trace - 02:52:02.504 AM] Sending request 'textDocument/hover - (29)'.
Params: {"textDocument":{"uri":"file:///usr/local/go/src/runtime/panic.go"},"position":{"line":915,"character":10}}

[Trace - 02:52:02.511 AM] Received response 'textDocument/hover - (29)' in 7ms.
Result: {"contents":{"kind":"markdown","value":"```go\ntype _panic struct{argp unsafe.Pointer; arg interface{}; link *_panic; pc uintptr; sp unsafe.Pointer; recovered bool; aborted bool; goexit bool}\n```"},"range":{"start":{"line":915,"character":7},"end":{"line":915,"character":13}}}
  1. I tried to dig into the difference and found function HoverIdentifier returned different HoverInformation. But the source hasn't been changed for a long time. Is it feature or I need some extra options for gopls?

https://github.com/golang/tools/blob/55cd4804dfa0984a27cd61589b7f56f937e05545/internal/lsp/source/hover.go#L66-L71

func Hover(ctx context.Context, snapshot Snapshot, fh FileHandle, position protocol.Position) (*protocol.Hover, error) {
    ident, err := Identifier(ctx, snapshot, fh, position)Rohan Challa, 1 year ago: • internal/lsp: support textDocument/hover for …
    if err != nil {
        return nil, nil
    }
    h, err := HoverIdentifier(ctx, ident)

// step 2 

>>> p *h
$1 = {
  Signature = "type _panic struct {\n\targp      unsafe.Pointer // pointer to arguments of deferred call run during panic; cannot move - known to liblink\n\targ       interface{}    // argument to panic\n\tlink      *_pan"...,
  SingleLine = "type _panic struct{argp unsafe.Pointer; arg interface{}; link *_panic; pc uintptr; sp unsafe.Pointer; recovered bool; aborted bool; goexit bool}",
  Synopsis = "A _panic holds information about an active panic.",
  FullDocumentation = "A _panic holds information about an active panic.\n\nA _panic value must only ever live on the stack.\n\nThe argp and link fields are stack pointers, but don't need special\nhandling during stack growth: b"...,
  LinkPath = "",
  LinkAnchor = "",
  importPath = "",
  symbolName = "",
  source = {
    _type = 0xd6cc40,
    data = 0xc011e614a0
  },
  comment = 0xc00534ed20,
  typeName = "_panic",
  isTypeAlias = false
}

// step3

>>> p *h
$2 = {
  Signature = "type _panic struct{argp unsafe.Pointer; arg interface{}; link *_panic; pc uintptr; sp unsafe.Pointer; recovered bool; aborted bool; goexit bool}",
  SingleLine = "type _panic struct{argp unsafe.Pointer; arg interface{}; link *_panic; pc uintptr; sp unsafe.Pointer; recovered bool; aborted bool; goexit bool}",
  Synopsis = "",
  FullDocumentation = "",
  LinkPath = "",
  LinkAnchor = "",
  importPath = "",
  symbolName = "",
  source = {
    _type = 0xe23c00,
    data = 0xc0034e2f50
  },
  comment = 0x0,
  typeName = "",
  isTypeAlias = false
}
findleyr commented 3 years ago

Thank you for the repro.

I believe this is the same as #46158, but I am investigating.

stamblerre commented 3 years ago

Duplicate of #46158

katsusan commented 3 years ago

Hi @findleyr, thank you for the fix, it seems there is still an edge case not covered.

Another reproduce:

version:

D:\Projects\leetcode>gopls version
golang.org/x/tools/gopls v0.7.2
    golang.org/x/tools/gopls@v0.7.2 h1:kRKKdvA8GOzra8rhSFDClOR7hV/x8v0J0Vm4C/gWq8s=

D:\Projects\leetcode>go version
go version go1.17 windows/amd64

steps:

  1. open $GOROOT/src/runtime/proc.go.
  2. hover for the global variable debug in following code. https://github.com/golang/go/blob/6097ebe627b7bc58d63d6765abc005f548b9644c/src/runtime/proc.go#L199-L202

hover result: image

findleyr commented 3 years ago

Thanks very much for the repro.

findleyr commented 2 years ago

Just looked into this. The debug struct is a real edge case: it is a var with a large struct literal type. For vars we use types.ObjectString to format their signature, which usually looks reasonable but in this case produces a very large type expression. It's hard to imagine doing something else: if we naively used the declaration syntax we'd have to handle all sorts of edge cases, like x := 2 // no type or x := []byte{/* lots of data */}, etc.

So I think the original issue is fixed, and we are unlikely to spend engineering time addressing this particular edge case. Closing so that this issue doesn't stagnate.

katsusan commented 2 years ago

Thank you Robert for the investigation, the edge case doesn't miss type information and gopls needs improvement in other more important aspects, so it doesn't deserve if needs too much time.