joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.27k stars 200 forks source link

Support for call hierarchies #614

Open leungbk opened 3 years ago

leungbk commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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

joaotavora commented 2 years ago

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 bug-gnu-emacs@gnus.org, and CC me in the report. You can also quote the paragraphs above as my suggestion.

gsingh93 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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

gsingh93 commented 1 year ago

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 https://github.com/joaotavora/eglot/issues/614#issuecomment-1288106083, so if anyone is interested in working on this that's a good starting point.

dolmens commented 1 year ago

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

joaotavora commented 1 year ago

@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 commented 1 year ago

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

joaotavora commented 1 year ago

@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.

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

joaotavora commented 1 year ago

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 commented 1 year ago

Sure.

kvaneesh commented 11 months ago

Did this change get merged back to eglot?

joaotavora commented 11 months ago

No, it is heavily clangd specific. And clangd needs work anyways. I have a clangd branch with the fixes, but no time to integrate. Sorry.

On Sun, Dec 10, 2023, 13:25 Aneesh Kumar K.V @.***> wrote:

Did this change get merged back to eglot?

— Reply to this email directly, view it on GitHub https://github.com/joaotavora/eglot/issues/614#issuecomment-1848965157, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6PQ7DEPMHDJO2K4GMV4LYIWZ33AVCNFSM4W3AU2FKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBUHA4TMNJRGU3Q . You are receiving this because you were mentioned.Message ID: @.***>