Support the which-func package, i.e. which-function-mode

[sfllaw]

which-func assumes that imenu is enabled and determines which function your current point is in using imenu--index-alist.

However, it doesn’t understand special elements in imenu--index-alist so those entries must be handled by a hook installed in
which-func-functions.

• eglot.el (eglot-which-func): Hook to determine the function which your current point is in, based on Eglot’s special Imenu element.

• eglot.el (eglot--managed-mode): Add and remove the eglot-which-func hook in which-func-functions.

• eglot-tests.el (basic-which-func): Test which-function-mode.

[joaotavora]

However, it doesn’t understand special elements in imenu--index-alist so those entries must be handled by a hook installed in which-func-functions.

Ah, but why isn't that hook installed by default in which-func-function? IOW, is it really impossible to have LSP-agnostic imenu clients?

The special elements in imenu--index-alist have been giving me trouble for a while now. It seems a lot of clients simply don't understand them. I don't understand why, but it's the reality. Maybe a better solution for your use case and all those other clients would be to simply stop using the special elements. I think some functionality would be lost, but perhaps much more "free" functionality would be gained.

[sfllaw]

However, it doesn’t understand special elements in imenu--index-alist so those entries must be handled by a hook installed in which-func-functions.

Ah, but why isn't that hook installed by default in which-func-function? IOW, is it really impossible to have LSP-agnostic imenu clients?

But which-func-functions is provided by the which-func package specifically to support imenu--index-alist special elements?

Are you asking why this patch doesn’t install the hook? It does here, inside eglot--managed-mode.

Or are you asking why which-func doesn’t rely on imenu’s interface to do the reverse lookup? I think this would be too slow. Imenu doesn’t expose this kind of API, and because the special function is arbitrary, you’d basically have to walk imenu--index-alist with save-excursion to locate the right entry, which would probably be super slow.

The special elements in imenu--index-alist have been giving me trouble for a while now. It seems a lot of clients simply don't understand them. I don't understand why, but it's the reality. Maybe a better solution for your use case and all those other clients would be to simply stop using the special elements. I think some functionality would be lost, but perhaps much more "free" functionality would be gained.

Yeah, it seems like overloading imenu--index-alist to present rich location information was a good attempt but imenu’s API isn’t flexible enough for other clients to deal with? What kind of existing functionality would be lost?

As an alternative, we could store the rich information returned by textDocument/documentSymbol in eglot--document-symbol-alist, which gets reduces to the simple elements in imenu--index-alist. But perhaps that would be better done in another PR?

If eglot--document-symbol-alist existed, I can also imagine a world where overlay properties are used to directly annotate the buffer. The documentation in which-func-mode actually points this out:

;; TODO LIST
;; ---------
;;     1. Dependence on imenu package should be removed.  Separate
;; function determination mechanism should be used to determine the end
;; of a function as well as the beginning of a function.
;;     2. This package should be realized with the help of overlay
;; properties instead of imenu--index-alist variable.

[stephe-ada-guru]

Simon Law ***@***.***> writes:

As an alternative, we could store the rich information returned by textDocument/documentSymbol in eglot--document-symbol-alist, which gets reduces to the simple elements in imenu--index-alist. But perhaps that would be better done in another PR? If eglot--document-symbol-alist existed, I can also imagine a world where [overlay properties](https://www.gnu.org/software/emacs/manual/html_node/elisp/Overlay-Properties.html) are used to directly annotate the buffer. The documentation in [which-func-mode](https://github.com/emacs-mirror/emacs/blob/3af9e84ff59811734dcbb5d55e04e1fdb7051e77/lisp/progmodes/which-func.el#L41-L47) actually points this out: > ``` elisp > ;; TODO LIST > ;; --------- > ;; 1. Dependence on imenu package should be removed. Separate > ;; function determination mechanism should be used to determine the end > ;; of a function as well as the beginning of a function. > ;; 2. This package should be realized with the help of overlay > ;; properties instead of imenu--index-alist variable. > ```

For what it's worth, ada-mode does not use imenu to provide which-function. Instead, it adds ada-which-function to which-func-functions. ada-which-function queries the syntax tree to find the declaration that contains point. eglot could do something similar; the textDocument/documentSymbol query essentially returns the syntax tree. You can descend the tree to find the symbol containing point, then ascend the tree to find a symbol that is a "function" in the which-function-mode sense (ada-mode includes type declarations here). That's assuming the server supports textDocument/documentSymbol. It would be more efficient if LSP defined a "containingDeclaration" query; then you would not have to process the entire syntax tree in lisp - I'm guessing that will be too slow in large files. Since ada-mode has complete control over the parser interface, the ada-mode parser provides that query. -- -- Stephe

[joaotavora]

@sfllaw

Are you asking why this patch doesn’t install the hook? It does here, inside eglot--managed-mode.

No sorry, I wasn't making myself clear. I'm going to try to say more or less the same thing in three different ways :-)

1. I was asking:

• if currently which-func depends on imenu structures to work correctly, AND

• if Eglot is already providing imenu structures

  then why should any new code be added to Eglot to support which-func?

2. Another way to see point 1

   a. if it's possible to avoid adding a dependency of which-func in Eglot, I'd like to do so, even if that means changing some of which-func.el (in a backward compatible way, of course)

   b. if it's not possible to avoid this dependency, or somehow not desirable, then OK, let's add it this PR, but I'd like to be as minimal as possible.

3. My other comment about imenu, which probably also confused you, is asking about Eglot's usage of imenu's "special" interfaces.
   I know from other issues (not very hard to find in the tracker, though I haven't done so now) that this usage is problematic and breaks some imenu clients, who don't understand it. If which-func were to be yet another such client, then it could make sense to revisit Eglot's usage of imenu and perhaps kill many birds with just one stone.

[joaotavora]

@sfllaw

Yeah, it seems like overloading imenu--index-alist to present rich location information was a good attempt but imenu’s API isn’t flexible enough for other clients to deal with? What kind of existing functionality would be lost?

You are asking the right questions, I think. But unfortunately, I have lost the answer to this (maybe the Commit log remembers?)

As an alternative, we could store the rich information returned by textDocument/documentSymbol in eglot--document-symbol-alist, which gets reduces to the simple elements in imenu--index-alist. But perhaps that would be better done in another PR?

That seems interesting. Especially so if it could somehow magically solve all the other imenu-related issues in addition to this which-func special-hook need. But yes, another PR.

[sfllaw]

That seems interesting. Especially so if it could somehow magically solve all the other imenu-related issues in addition to this which-func special-hook need. But yes, another PR.

@joaotavora I started drafting this implementation in #760. I would appreciate your thoughts about my approach.

[joaotavora]

Hi @sfllaw, I briefly tested the latest code with which-func-mode in a cpp file with clangd as the supporting server and it seems to work OK.

Can double check if anything is missing?

[sfllaw]

@joaotavora: It seems to work OK for me as well, so thank you for doing this! Also, I’m glad that you removed the magic closures!