Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

langserver: add godef-based hover backend #196

Merged
merged 3 commits into from
Jun 23, 2017
Merged

langserver: add godef-based hover backend #196

merged 3 commits into from
Jun 23, 2017

Conversation

emidoots
Copy link
Member

This change adds a godef-based hover backend, similar to the godef-based
textDocument/definition backend added prior. The motivation for this
change is to avoid typechecking the entire program just to serve a single
textDocument/hover request. After this change, hover and definition are
both very quick and use little resources. Larger requests like workspace/symbol,
or textDocument/references, will continue to use more resources as they must
perform typechecking.

The output style of this hover implementation does vary from our prior
typechecking implementation in slight ways, but overall the implementation
always produces results that are on par or slightly better than our typechecking
implementation.

As with the textDocument/definition implementation, we should attempt consolidation
of this hover implementation with our typechecking variant further in the future. At
this point, of course, they are too different to share much code or implementation.

Helps #178
Helps microsoft/vscode-go#853

emidoots added 2 commits June 23, 2017 00:51
This change adds a godef-based hover backend, similar to the godef-based
`textDocument/definition` backend added prior. The motivation for this
change is to avoid typechecking the entire program just to serve a single
`textDocument/hover` request. After this change, hover and definition are
both very quick and use little resources. Larger requests like `workspace/symbol`,
or `textDocument/references`, will continue to use more resources as they must
perform typechecking.

The output style of this hover implementation does vary from our prior
typechecking implementation in slight ways, but overall the implementation
always produces results that are on par or slightly better than our typechecking
implementation.

As with the `textDocument/definition` implementation, we should attempt consolidation
of this hover implementation with our typechecking variant further in the future. At
this point, of course, they are too different to share much code or implementation.

Helps #178
Helps microsoft/vscode-go#853
filename := uriToFilePath(loc.URI)

// Parse the entire dir into its respective AST packages.
pkgs, err := parser.ParseDir(fset, filepath.Dir(filename), nil, parser.ParseComments)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have ways to parse that use the virtual filesystem. If you use that you will correctly parse unsaved changes. But it is likely a very minor issue, and I think we can fix all that when we consolidate the implementations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment. This reminded me that I needed to fix hover & jump to definition for unsaved files.

I didn't address your comment directly, though, because as you can see in 8d908eb the fix is not quite as simple. We can't use parseDir directly here because we need it to be aware of the test-suite somewhat. I agree, though, that this is something we can resolve down the line / is very minor.

This change makes it such that hovering or jumping to definition while
inside a file that has been modified always works. There is more to be
done in the future, in this regard, for e.g. hover on a modified
dependency [see here](#196 (comment))
but this change fixes the most common case (hovering/jumping to def on
code that is actively being written / modified).
@emidoots emidoots merged commit 938da20 into master Jun 23, 2017
@emidoots emidoots deleted the sg/perf2 branch June 23, 2017 22:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants