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: sometimes documentation on hover is not shown #46158

Open
inliquid opened this issue May 13, 2021 · 13 comments
Open

x/tools/gopls: sometimes documentation on hover is not shown #46158

inliquid opened this issue May 13, 2021 · 13 comments
Assignees
Milestone

Comments

@inliquid
Copy link

@inliquid inliquid commented May 13, 2021

  1. VS Code
  2. go1.16.4
  3. gopls@master

For example in case of (*url.URL).Query():

изображение

изображение

@findleyr
Copy link
Contributor

@findleyr findleyr commented May 14, 2021

Thanks for the report. Which gopls commit did you build at? I thought this might be related to recent work on AST trimming, but can't reproduce at tip.

Are you able to repro? If so, can you please capture RPC logs, as described here?
https://github.com/golang/vscode-go/blob/master/docs/troubleshooting.md#collect-gopls-information

@inliquid
Copy link
Author

@inliquid inliquid commented May 14, 2021

It was built with go install run from the gopls repo.

Unfortunately I can't reproduce it now, it had been in that state for a while, but after restart of VS Code, I can see documentation for this particular function. I'll watch over it and let you know if this repeats.

@ShoshinNikita
Copy link

@ShoshinNikita ShoshinNikita commented May 14, 2021

I faced a similar issue. But in my case fields of a struct have disappeared. The first commit with this issue is golang/tools@cd1d088. Note that this behavior is inconsistent (see screenshots)

Steps to reproduce

package main

import (
	"github.com/go-pg/pg/v10/orm"
)

func main() {
	orm.NewQuery(nil, nil)
	(&orm.Query{}).New()
}
  1. Go to Definition of orm.NewQuery
  2. Hover on the return value
  3. Query has ho fields

And

  1. Go to Definition of method (*orm.Query).New
  2. Hover on Query in the line clone := &Query{
  3. Query has fields, but they are poorly formatted

gopls@09ab05b (master)

1 2
image image

gopls@v0.6.11

1 2
image image

@findleyr findleyr self-assigned this May 14, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented May 17, 2021

FWIW, I think these are separate issues.

@ShoshinNikita you issue seems to clearly be a result of AST trimming. I am not so sure about the original issue. Keeping this as a single issue until I understand it better.

@gopherbot
Copy link

@gopherbot gopherbot commented May 17, 2021

Change https://golang.org/cl/320550 mentions this issue: internal/lsp/source: re-parse if needed when collecting identifier info

gopherbot pushed a commit to golang/tools that referenced this issue May 18, 2021
With the new ParseExported logic, we can lose some unexported fields on
exported structs. This can lead to misleading or malformatted hover
information.

Fix this by ensuring we always extract the Spec from a full parse. Since
this path is only hit via user-initiated requests (and should only be
hit ~once per request), it is preferable to do the parse on-demand
rather than parse via the cache and risk pinning the full AST for the
remaining duration of the session.

For golang/go#46158

Change-Id: Ib3eb61c3f75e16199eb492e3e129ba875bd8553e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/320550
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@findleyr
Copy link
Contributor

@findleyr findleyr commented May 18, 2021

The CL above should fix the issue reported by @ShoshinNikita, but I'm still not sure what's going on in the original issue: none of the AST trimming should have dropped comments.

So I think we should leave this issue open for a little while in case the problem reoccurs and we can capture logs. Unassigning in the meantime.

@findleyr findleyr removed their assignment May 18, 2021
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 21, 2021

@inliquid: Have you encountered this again, after the CL above? If not, let's close this issue as I think it has been resolved.

@inliquid
Copy link
Author

@inliquid inliquid commented May 21, 2021

@stamblerre no, haven't seen this so far.

@inliquid inliquid closed this May 21, 2021
@stamblerre stamblerre removed this from the gopls/unplanned milestone May 24, 2021
@stamblerre stamblerre added this to the gopls/v0.7.0 milestone May 24, 2021
@ShoshinNikita
Copy link

@ShoshinNikita ShoshinNikita commented Jun 8, 2021

@findleyr At first, I decided the issue I reported is not completely fixed. But then I understood it is the intended result of AST trimming. Is that correct?

Exported function ast.FilterFile:

image

Unexported function ast.filterFile:

image

@stamblerre stamblerre reopened this Jun 9, 2021
@stamblerre stamblerre removed this from the gopls/v0.7.0 milestone Jun 9, 2021
@stamblerre stamblerre added this to the gopls/v0.7.1 milestone Jun 9, 2021
@stamblerre stamblerre removed this from the gopls/v0.7.1 milestone Jun 9, 2021
@stamblerre stamblerre added this to the Unreleased milestone Jun 9, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Jun 9, 2021

Thanks for catching that. This must be an edge case that doesn't trigger re-parsing, for some reason.

I'll look into it.

@suzmue suzmue removed this from the Unreleased milestone Jun 21, 2021
@suzmue suzmue added this to the gopls/unplanned milestone Jun 21, 2021
@hyangah hyangah removed this from the gopls/unplanned milestone Jun 21, 2021
@hyangah hyangah added this to the gopls/v0.7.1 milestone Jun 21, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Jul 9, 2021

The issue is that in the case of filterFile, we're not finding the hovered declaration at all. The patch of CL 320550 was really too shallow: we need a mechanism for specifying exactly what information is needed to fulfill the hover request, and this mechanism should go through the gopls cache.

I'm experimenting with this now, but it will likely turn out not to be a small change.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 9, 2021

Change https://golang.org/cl/333689 mentions this issue: internal/lsp: improve package search in a couple places

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 9, 2021

Change https://golang.org/cl/333690 mentions this issue: internal/lsp/source: improve logic for finding full syntax in hover

gopherbot pushed a commit to golang/tools that referenced this issue Jul 13, 2021
When we open a file in a package, independent of whether it is in the
workspace, we type check in ParseFull mode. However, several other
code paths don't find this better parse mode.

We need a better abstraction, but for now improve a couple code paths
specifically for the purpose of fixing Hover content.

Updates golang/go#46158
Updates golang/go#46902

Change-Id: I34c0432fdba406d569ea963ab4366336068767a2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/333689
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre removed this from the gopls/v0.7.1 milestone Jul 26, 2021
@stamblerre stamblerre added this to the gopls/v0.7.2 milestone Jul 26, 2021
gopherbot pushed a commit to golang/tools that referenced this issue Jul 26, 2021
When enriching identifier info with full syntax, it's cleaner to find
the enclosing decl. Use the full decl in hover if we were unable to find
a node in the original type-checked package.

Update the regtest to exercise hovering in a non-workspace package.

Updates golang/go#46158

Change-Id: Ic1772a38fb1758fb57a09da9483a8853cc5498f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/333690
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre removed this from the gopls/v0.7.2 milestone Sep 9, 2021
@stamblerre stamblerre added this to the gopls/on-deck milestone Sep 9, 2021
@stamblerre stamblerre removed this from the gopls/on-deck milestone Sep 10, 2021
@stamblerre stamblerre added this to the gopls/v0.7.3 milestone Sep 10, 2021
@stamblerre stamblerre removed this from the gopls/v0.7.3 milestone Oct 19, 2021
@stamblerre stamblerre added this to the gopls/on-deck milestone Oct 19, 2021
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
7 participants