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] Add variable symbol gathering to symbol filler #1949

Merged
merged 3 commits into from
Jul 10, 2023

Conversation

jbylicki
Copy link
Collaborator

This PR adds variables to symbol gathering for outline-like features in editors. Tested to work as expected in coc.nvim, as did the original issue and in builtin helix lsp handler

Fixes #1882

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (c719ed3) 92.83% compared to head (b4e9fbf) 92.85%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1949      +/-   ##
==========================================
+ Coverage   92.83%   92.85%   +0.01%     
==========================================
  Files         355      355              
  Lines       26073    26129      +56     
==========================================
+ Hits        24206    24261      +55     
- Misses       1867     1868       +1     
Impacted Files Coverage Δ
verilog/tools/ls/document-symbol-filler.cc 98.96% <100.00%> (+0.20%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jbylicki jbylicki requested a review from hzeller June 19, 2023 08:19
@hzeller
Copy link
Collaborator

hzeller commented Jun 20, 2023

There are typically a lot of variables so I fear the output of such a symbol outline will be very 'noisy'. That is the reason why I didn't include these in the original implementation as it would make the tree less useful and visually overwhelming.
Navigation to these might be easier with go-to-definition ?

However, I could be convinced if the more noisy tree would actually be useful in practice. Are there ways on the client (e.g. the tree view in vscode) to keep out certain elements ? Maybe we can provide a flag to configure if it should be included ?

@jbylicki
Copy link
Collaborator Author

I will look into what filtering options the most popular editors provide for outlines (VSCode, neovim) and I will probably add a flag either way. The default will be probably determined by the quality of those options if they exist

@jbylicki
Copy link
Collaborator Author

Editor filtering options for outlines / symbol lists

Consistent finding for me is that most of the "big" files are not that huge in terms of outline size maybe besides generated .sv, as they tend to have quite a few regs/variables.

How popular editors handle outlines

VSCode

Allows for sorting by type of symbol (struct, var, module/function) inside the outline. Goto symbol allows for searching within the outline for the desired symbol and is decently responsive. Usability will probably not decrease by a lot with variables included into the outline.

Neovim (default LSP client)

Using "raw" <cmd> lua vim.lsp.buf.document_symbol()<CR> bound to a shortcut opens a split buffer that is noticably dense to navigate, therefore the addition of variables reduces the usability of that method.
On the other hand, most people seem to use plug-ins and providers for lists, searchables, and fuzzy finders instead of builtin quickfix buffer filled with symbols. There was little documentation on setting it up that way; most guides were focusing on coc.nvim, vista and symbols-outline.nvim.

Neovim (coc.nvim)

coc.nvim provides an outline that sorts by "relevance" (modules, blocks, funcitons, variables) and within each category there is alphabetical sorting with line indicators.
Each structure that has embedded symbols inside (eg. variables in modules) are collapsable at each node that contains those children that helps a lot with browsing.
The variables clutter the outline a bit, however there is a fuzzy search option via :CocList outline. I feel like the positives of having a full outline outweigh the minor clutter
that the addition of variables creates.

Neovim (symbols-outline.nvim)

This plugin adds a structured view that can collapse nodes simmilar to coc.nvim There is a bit more clutter, as the UI isn't as easy to navigate and there are no simple search options.
Still, on a decently large file (taken from opentitan) it is very usable with those variables added. There are also filtering options within the configuration that will allow to limit the scope of what is outlined on a per-user basis.

Neovim (vista)

In vista, there are multiple providers for structure. In my case, I have tested coc and nvim_lsp as providers and both had the means to efficiently navigate the outline. nvim_lsp backend filters all symbols into categories (thus allowing the variables to easily get collapsed), and coc has a structured view up top and defaults to a bottom section that just lists the modules and allows to jump to a tree of the desired one. It is surprisingly nice to use and the addition of variables does not hinder the usability.

Helix

The symbol outline is piped into the fuzzy finder, like almost everything searchable in helix. It does not currently distinguish by type of symbol, but the search is effortless. The default sorting is by position in the file. There is little to no difference in usability in my opinion to how difficult it is to find a symbol with variables added to the outline.

Conclusion

Therefore, I'd say that a flag that would enable/disable variables in outlines should have a default such that variables are included. It is the behavior of popular LSP servers (clangd, jedi) and most of the outline/symbol picking solutions provide decent or good ways of navigating around them. @hzeller do you agree?

@hzeller
Copy link
Collaborator

hzeller commented Jul 10, 2023

Yes, sounds good!

@hzeller hzeller merged commit 4e68249 into chipsalliance:master Jul 10, 2023
34 checks passed
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.

symbol only output module name in nvim coc.nvim
3 participants