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

Support for call hierarchies #614

Open
leungbk opened this issue Jan 31, 2021 · 19 comments
Open

Support for call hierarchies #614

leungbk opened this issue Jan 31, 2021 · 19 comments

Comments

@leungbk
Copy link
Contributor

leungbk commented Jan 31, 2021

https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#textDocument_prepareCallHierarchy

It would be nice if Eglot supported incoming- and outgoing-call hierarchies, introduced in the 3.16 spec.

lsp-mode uses treemacs, which unfortunately is not part of ELPA, to display the hierarchies. Is the new hierarchy.el in core Emacs suitable for displaying these hierarchies?

It seems that we'd have to redraw the entire hierarchy every time we expand a node when making successive call-hierarchy requests; for example, when an incoming-call hierarchy has a branch like child <- parent <- grandparent and we expand the grandparent node, the entire tree, including siblings and cousins of child, would be redrawn after having fetched the great-grandparent from the server. I don't understand Emacs' internals enough to know if having to redraw everything is an issue or not.

@joaotavora
Copy link
Owner

I'd think that redrawing time << server request time.

I'm happy to take a look at a prototype implementation based on hierarchy.el.

@gsingh93
Copy link

I took a quick look at this. I have results back from clangd from the callHierarchy/incomingCalls request, but I'm a bit stuck on how to store the hierarchy. I'm reading https://emacs.cafe/emacs/guest-post/2017/06/26/hierarchy.html, and it seems hierarchy.el requires a function which given an element in the hierarchy, returns the parent. I'm not sure how this function would look in this case.

Alternatively, we could just store a regular elisp list in a tree-like structure and pass that to hierarchy.el, but I'm not good enough with elisp to know exactly how to update this tree each time we get the next level of incoming call information.

Once that part is figured out, the actual displaying of the tree seems straightforward, hierarchy-tree-display seems to do what we want.

Here's my current code which just makes the LSP requests (not production code that I would submit in a PR, just wrote it for some quick testing):

(defun eglot-prepare-call-hierarchy (server)
  (jsonrpc-request (eglot--current-server-or-lose)
                   :textDocument/prepareCallHierarchy
                   (eglot--TextDocumentPositionParams)))

(defun eglot-incoming-calls ()
  (interactive)
  (let* ((server (eglot--current-server-or-lose))
         (call-hierarchy-item (seq-elt (eglot-prepare-call-hierarchy server) 0))
         (results (mapcar (eglot--lambda (CallHierarchyIncomingCall from)
                            (eglot--dbind (CallHierarchyItem name) from
                              (message "%s" name)))
                          (jsonrpc-request server
                                           :callHierarchy/incomingCalls
                                           `(:item ,call-hierarchy-item)))))

    (message "%s" results)))
    
(eval-and-compile
  (defvar eglot--lsp-interface-alist
    `(
      (CallHierarchyItem (:name :kind :uri :range :selectionRange) (:tags :detail ::data))
      (CallHierarchyIncomingCall (:from :fromRanges))
      ;; ...    

@gsingh93
Copy link

Any advice on how to implement this? This feature is the only reason I still need to use vscode.

@joaotavora
Copy link
Owner

Any advice on how to implement this? This feature is the only reason I still need to use vscode.

For top-level architecture, the approach I recommend is to propose a new library call-hierarchy.el to Emacs (that can also be distributed as :core GNU ELPA package) and then make eglot.el depend on it. This call-hierarchy.el library has an entry point M-x call-hierarchy (which is a better name for your "incoming-calls" command). That command that displays, in a new buffer, the call hierarchy to the "thing at point" by default. The library uses hierarchy.el to do the display heavy-lifting, and because of that hierarchy.el also needs to become a :core GNU ELPA package.

Then, call-hierarchy.el defines a data format and a protocol for passing info to it. This can be a "special hook" of functions that are tried until one of them returns something in the format. A good name for this special hook can be call-hierarchy-functions. During testing, you don't need to use Eglot and/or LSP for testing this. You can just put a function call-hierarchy-dummy-data in the special hook that returns some hardcoded test data so that you can fine-tune the display mechanisms and the interaction with hierarchy.el.

Then, after that step is more or less done, you start experimenting with having Eglot add an LSP-powered function to the new special hook, buffer locally in the eglot--managed-mode.

Instead of call-hierarchy.el, another expressive name for this new library could be who-calls.el and M-x who-calls.

This is just the top-level architecture. I recommend we start a discussion in Emacs's bug tracker about this. Use M-x report-emacs-bug or send email to [email protected], and CC me in the report. You can also quote the paragraphs above as my suggestion.

@gsingh93
Copy link

gsingh93 commented Oct 26, 2022

Thanks @joaotavora! Before I create an Emacs bug for this, I want to make sure I understand one thing: what's the reason for adding this library to Emacs instead of creating it as a third-party library first, and then potentially adding it to Emacs in the future or leaving it as a third-party package?

@joaotavora
Copy link
Owner

If it's easier, you can still follow most of my above advice and make it an optional or "soft" dependency, like company-mode, markdown-mode and yasnippet. Read the commentary of lisp/progmodes/eglot.el for more.

The disadvantage is that many users using only Emacs won't get the benefits of using call hierarchies. But maybe it's good to start like this and then move into a :core package, yes.

@mrunhap
Copy link

mrunhap commented Nov 14, 2022

@gsingh93 Hi, may I ask is there any update to make eglot support call hierarchies?

@gsingh93
Copy link

I haven't had time to look into this, and honestly my elisp skills are probably not good enough to start this project. On the bright side, @joaotavora described what needs to be done in #614 (comment), so if anyone is interested in working on this that's a good starting point.

@dolmens
Copy link

dolmens commented Aug 25, 2023

I have just written one and it works for me.
https://github.com/dolmens/eglot-hierarchy

@joaotavora
Copy link
Owner

@dolmens I'd like to add a version of your code to eglot.el proper (and do a number of improvements on top). Do you have an FSF copyright assignment?

@dolmens
Copy link

dolmens commented Aug 25, 2023

@joaotavora No I haven't, I am currently working on it.

@joaotavora
Copy link
Owner

@dolmens After reading through your code, I realized that it's heavily clangd-specific. Moreover clangd doesn't fully respect the protocol. Specifically there are two problems with it.

  • It doesn't support callHierarchy/outgoingCalls
  • The data field of TypeHierarchyItem instances isn't stable.

So, your extension doesn't work with other server, such as rust-analyzer and very likely others. So this is a bit of a showstopper, as Eglot the LSP client is supposed to be server-agnostic.

Your code also has other UI problems, but those are due to the inherent limitations of hierarchy.el and "not your fault". For example, a good UI would be able to show or at least summarize the locus of each incoming call.

Nevertheless your extension is a very good start and has very good ideas, such as the use of the UTF up and down arrows to represent a directed (possibly cyclic) graph as a tree.

I'm rewriting a version of this from scratch, which works with rust-analyzer (that server is compliant as far as I can tell). But not with clang. So I'll be having a good look at clangd code to see if those shortcomings may be fixed. For example there's a (stalled) patch for callHierarchy/outgoingCalls support here https://reviews.llvm.org/D93829. I might take it up soon-ish. As to the unstable data, I'm still trying to understand if it's indeed a non-compliance.

@dolmens
Copy link

dolmens commented Aug 26, 2023

@joaotavora Go ahead, I assume you are free to do what you think is appropriate.

If a certain feature has a standardized approach, we adopt it, if not, it’s preferable to have an adaptation layer, allowing specific language server tasks to be delegated to plugins or users. If there’s functionality exclusive to a particular language server, then it can only be handled by plugins or users, in such cases, Eglot’s only role is just ‘allowing it’. I guess.

@joaotavora
Copy link
Owner

If there’s functionality exclusive to a particular language server, then it can only be handled by plugins or users, in such cases,

Yes, that's true. For example, I've just shown in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65418#23 how to implement the clangd-specific textDocument/inactiveRegions specification using the Eglot API.

But here, there is no such need. This feature has a standardized approach: it is described in the LSP standard, which surely you are familar with (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/).

So a server-specific adaptation layer isn't necessary, all that's necessary is, as usual, for Eglot and the servers to follow the standard. In practice, what happens is that features start out as experimental server-specific behaviour (like the aforementioned textDocument/inactiveRegions) and then become standardized.

Who is "we", by the way? Are you a member of some relevant project or were you talking about "we Emacs devs"?

@dolmens
Copy link

dolmens commented Aug 26, 2023

@joaotavora We the community, not just the Emacs devs. Cause in standard situations, there is generally no argument.

@joaotavora
Copy link
Owner

OK, I see. By the way I hope you are still interested in getting a FSF copyright assignment for future contribution contributions to Eglot and Emacs itself. Let me know if you need help getting the process started.

@dolmens
Copy link

dolmens commented Aug 26, 2023

Sure.

@kvaneesh
Copy link

Did this change get merged back to eglot?

@joaotavora
Copy link
Owner

joaotavora commented Dec 10, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants