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: attach documentation to completion items #29151

Open
stamblerre opened this issue Dec 7, 2018 · 28 comments

Comments

Projects
None yet
9 participants
@stamblerre
Copy link
Contributor

commented Dec 7, 2018

LSP has a field for documentation in completion items, as well as space for markdown in the "hover" request. We should attach useful documentation here.

@gopherbot gopherbot added this to the Unreleased milestone Dec 7, 2018

@Tolsee

This comment has been minimized.

Copy link

commented Dec 9, 2018

I would love to help If possible, can you please guide me @stamblerre

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

@Tolsee: absolutely! The main feature here involves generating documentation from a types.Object. You can see that in both completion and hover, we have access to a types.Object, a file's AST, a types.Package, and a types.Info. The package https://godoc.org/go/doc will likely be needed here as well. The tool https://github.com/zmb3/gogetdoc also does this task, so it might be helpful to look at.

Feel free to send me a change for review if you'd like to work on this.

@ramya-rao-a

This comment has been minimized.

Copy link

commented Dec 14, 2018

Note: If in case getting the docs takes a little more time, then it need not be sent for each of the completion item in the result. There is a resolve completion item request for just the completion item in focus. You can send the docs as part of this request.

@zmb3

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Also happy to assist @Tolsee, let me know if I can be of any help.

@stamblerre stamblerre added gopls and removed gopls labels Mar 12, 2019

@nicpottier

This comment has been minimized.

Copy link

commented Mar 27, 2019

Saw in the tools update that this would be a good issue to get started. Is anybody actively working on this? If not I'm game to give it a go, probably starting in trying to get any docs included in Hover as that seems like a fairly clear start.

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Don't believe so - just assign yourself when you start working so others will be aware. Feel free to reach out to me here or on Slack with any questions! We should be able to add a documentation function that can be used by both Hover and Completion.

@nicpottier

This comment has been minimized.

Copy link

commented Mar 27, 2019

Great, let me see if I can get a dev environment rolling against VSCode and if that works then I'll volunteer for this. I've done some documentation extraction for our own go stuff so have some experience on this so in theory should be able to knock this out.

@nicpottier

This comment has been minimized.

Copy link

commented Mar 27, 2019

Ok, I'm going to give this a go, got this working with a vscode-ception of gopls which is fun. I can't assign myself this but I'll look at this over the next few days and hopefully have something in a day or two to show.

@stamblerre stamblerre assigned stamblerre and unassigned stamblerre Mar 27, 2019

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Looks like I can't assign it to you either - I guess you have to have some ownership in the repository or something. Self-assigned to make it obvious that this is taken.

@nicpottier

This comment has been minimized.

Copy link

commented Mar 28, 2019

Realize this is a slightly different issue but on the call you mentioned not liking the tag treatment when looking at a struct. Started looking at that since that seemed an even simpler entry point and that looks like explicit behavior in writeType. How do you want those tags represented? Do you want them gone entirely? (I feel that is likely most natural, I don't expect tags to be included in a hover, but checking what desired is)

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

good idea! yeah i think eliminating them makes the most sense.

@nicpottier

This comment has been minimized.

Copy link

commented Mar 28, 2019

Ah, I see now, the format is part of the core library in ObjectString... So next question is do we want to mangle what we get back from ObjectString or reimplement/copy and tweak that implementation into lsp? Seems the latter is the only reasonable option.

@ianthehat

This comment has been minimized.

Copy link

commented Mar 28, 2019

I looked at this when writing the guru compatibility, I think a complete implementation inside lsp/source is the only viable approach.
I was thinking of writing something that builds a cleaned ast and then using the normal printer to build the string, but I did not look hard enough to be sure that is the right approach.

@zmb3

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

I'm not convinced it's the tags that makes the tooltip hard to look at, but rather the lack of formatting.

We've been displaying the tags in Atom's hover implementation for a while and haven't received any feedback that the tags are a problem. Here's a screenshot of a tooltip that includes tags but properly formats the struct type definition:

Screen Shot 2019-03-28 at 2 17 06 PM

@nicpottier

This comment has been minimized.

Copy link

commented Mar 28, 2019

Ya, I kinda agree. Maybe we should start with formatting first and see how people feel about it before reimplementing all the ObjectString stuff? Is that a vscode responsibility or the responsibility of the markdown we produce?

@muirrn

This comment has been minimized.

Copy link

commented Apr 18, 2019

I'd like to jump on this one if it is back to available.

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

I actually have a CL in progress to fix this, sorry I didn't write here earlier. It's actually a fairly big change, so I may be able to file several more issues once my first one is in.

@stamblerre stamblerre removed the Suggested label Apr 18, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Apr 18, 2019

Change https://golang.org/cl/172661 mentions this issue: internal/lsp: use ast.Nodes for hover information

gopherbot pushed a commit to golang/tools that referenced this issue Apr 18, 2019

internal/lsp: use ast.Nodes for hover information
This change associates an ast.Node for some object declarations.
In this case, we only handle type declarations, but future changes will
support other objects as well. This is the first step in adding
documentation on hover.

Updates golang/go#29151

Change-Id: I39ddccf4130ee3b106725286375cd74bc51bcd38
Reviewed-on: https://go-review.googlesource.com/c/tools/+/172661
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Apr 19, 2019

Change https://golang.org/cl/172958 mentions this issue: internal/lsp: support comments on hover for all typenames, funcs

gopherbot pushed a commit to golang/tools that referenced this issue Apr 22, 2019

internal/lsp: support comments on hover for typenames, funcs, fields
This change adds support for showing documentation when hovering over any
named type or function. For now, we show the entire comment associated
with the type; in future CLs, we should refine our approach and perhaps
only show the first line or sentence.

Updates golang/go#29151

Change-Id: Ib33284747b19acba67d79fb55c916574c3dd8073
Reviewed-on: https://go-review.googlesource.com/c/tools/+/172958
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

@stamblerre stamblerre changed the title x/tools/internal/lsp: attach documentation to hover, completion items x/tools/internal/lsp: attach documentation to completion and signature help Apr 25, 2019

@segevfiner

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

As I'm sure you are aware, note that Go doc strings are not Markdown but rather have their own minimalistic formatting. For them to display with formatting correctly they will need to be converted to Markdown before returning them if we want them to show up formatted in the editor correctly. See microsoft/vscode-go#2245 where I implemented this transformation albeit in JS and here it is needed in Go.

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Thanks for bringing this to my attention, @segevfiner! Right now in gopls we don't do anything to make sure the comments are formatted well, so I will definitely look into this.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 5, 2019

Change https://golang.org/cl/180657 mentions this issue: internal/lsp: attach documentation to signature help

gopherbot pushed a commit to golang/tools that referenced this issue Jun 6, 2019

internal/lsp: attach documentation to signature help
This change refactors hover to generate documentation for just the
declaration portion of an identifier.

Updates golang/go#29151

Change-Id: I16d48a99b56c36132e49cc87e2736f85c88ed14a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/180657
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

@stamblerre stamblerre changed the title x/tools/internal/lsp: attach documentation to completion and signature help x/tools/internal/lsp: attach documentation to completion items Jun 28, 2019

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

The last case that needs documentation is completion items. This should probably be done on completionItem/resolve to avoid extra work.

@segevfiner

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2019

To do that on completionItem/resolve we first need to add some info to the returned CompletionItem-s data field that can be used to quickly get back the types.Object for the given item. Once we have that resolve can use that to get the item again and extract the relevant documentation for it. It also seems that the existing code for documentation is based on ast instead of types, so we either need code to grab documentation from types or get the ast node in resolve instead.

So what should that key for looking up the object in resolve be?

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

I do think we should cache identifier information to avoid recomputing hover information, but I think to start, we may be able to avoid caching. If we know the position of the completion item, we can get the identifier at the position and get the documentation that way. It might not be technically correct, as the code could change before the item is resolved, but in practice, I'd be surprised if that were a common occurrence.

@segevfiner

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

I think gopls has type info cached so it sounds reasonable to me it will already have a mechanism for this. Maybe Info.ObjectOf but I didn't try this yet.

@stamblerre stamblerre changed the title x/tools/internal/lsp: attach documentation to completion items x/tools/gopls: attach documentation to completion items Jul 2, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Jul 3, 2019

Change https://golang.org/cl/184721 mentions this issue: internal/lsp: add documentation to completion items

gopherbot pushed a commit to golang/tools that referenced this issue Jul 9, 2019

internal/lsp: add documentation to completion items
This change adds documentation to the completion items. This normally
should be done in completionItem/resolve, since it takes more time to
compute documentation. However, I am not sure if that latency incurred
by pre-computing documentation is actually significantly more than the
latency incurred by an extra call to 'completionItem/resolve'. This
needs to be investigated, so we begin by just precomputing all of the
documentation for each item.

Updates golang/go#29151

Change-Id: I148664d271cf3f1d089c1a871901e3ee404ffbe8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184721
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

@stamblerre stamblerre removed the Suggested label Jul 10, 2019

movie-travel-code added a commit to movie-travel-code/tools that referenced this issue Jul 11, 2019

internal/lsp: add documentation to completion items
This change adds documentation to the completion items. This normally
should be done in completionItem/resolve, since it takes more time to
compute documentation. However, I am not sure if that latency incurred
by pre-computing documentation is actually significantly more than the
latency incurred by an extra call to 'completionItem/resolve'. This
needs to be investigated, so we begin by just precomputing all of the
documentation for each item.

Updates golang/go#29151

Change-Id: I148664d271cf3f1d089c1a871901e3ee404ffbe8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184721
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

The above change experimentally added the documentation to the completion items, but we still have a few things to determine before turning this feature on by default. In particular - should we move to the approach of using completionItem/resolve or stick to population documentation for all items in textDocument/completion? This will require benchmarks to compare the approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.