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

[LSP] Initial support for textDocument/hover request #1922

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

glatosinski
Copy link
Collaborator

@glatosinski glatosinski commented May 16, 2023

Resolves #1187

This PR adds initial support for textDocument/hover request.

It adds a HoverBuilder that constructs hover messages for the current position.

Tasks:

  • Adds hover feature to LS capabilities
  • Prints metatype for symbol
  • Prints type for symbol
  • Prints documentation for symbol
  • Provides label for end
  • Provides label for endmodule, endfunction of block (currently WIP)
  • Tests for the textDocument/hover method (currently WIP)

Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

Nice! A few comments so far.

verilog/tools/ls/hover.h Outdated Show resolved Hide resolved
verilog/tools/ls/hover.cc Show resolved Hide resolved
verilog/tools/ls/hover.cc Outdated Show resolved Hide resolved
@@ -593,6 +593,13 @@ std::string ReferencesRequest(absl::string_view file, int id, int line,
line, character);
}

// Creates a textDocument/hover request
std::string HoverRequest(absl::string_view file, int id, int line,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the id actually a relevant value we need to pass for the tests ? If not maybe we just replace it with some static constexpr int kDummyId = 1; to not distract from the other values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be unique per request - we can go with the static value that is incremented per request

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, but is any place even checking if it is unique per request ?

verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved
verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved
verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved
verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved
verilog/tools/ls/verilog-language-server_test.cc Outdated Show resolved Hide resolved
@glatosinski glatosinski force-pushed the lsp-hover-support branch 2 times, most recently from 3f53a4b to fbeeed8 Compare May 18, 2023 15:34
…ol_table header

Method SymbolMetaTypeAsString is now provided by header to allow
printing symbol meta types, e.g. for hover purposes

Signed-off-by: Grzegorz Latosinski <[email protected]>
…ding definitions and tokens

To be able to use FindDefinitionLocation in other use cases than goto-definition,
the parameters are now of type TextDocumentPositionParams.
Also, GetTokenAtTextDocumentPosition now returns optional TokenInfo instead
of absl::string_view, to provide more useful information regarding the symbol.

Signed-off-by: Grzegorz Latosinski <[email protected]>
…g SymbolTableNode with definition

Signed-off-by: Grzegorz Latosinski <[email protected]>
…s to Language Server

Signed-off-by: Grzegorz Latosinski <[email protected]>
… for hovering over symbol

Signed-off-by: Grzegorz Latosinski <[email protected]>
…publicly available

Signed-off-by: Grzegorz Latosinski <[email protected]>
…e hover is applied, added using dedicated class

Signed-off-by: Grzegorz Latosinski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language Server: implement textDocument/hover
2 participants