Refactor Imenu support to store textDocument/documentSymbol information in overlays

[sfllaw]

eglot-imenu used to build imenu--index-alist with special elements, of the form (INDEX-NAME POSITION FUNCTION ARGUMENTS...) which is not a great API. Most packages that integrate with Imenu cannot work with these elements, and expect the simple element of (INDEX-NAME . POSITION). See related issues: #535, #536, #547, #621, #758.

This pull request is designed to tackle this problem by:

1. Storing the result of textDocument/documentSymbol JSON-RPC calls in overlays, both in raw and parsed forms.
2. Building imenu--index-alist from these overlays with entirely simple elements.
3. Registring a which-func-functions hook to read the current function/class/named-scope from the best overlay.

[sfllaw]

@joaotavora Here is my work-in-progress on refactoring Imenu support. Do you mind taking a look at the code and providing some feedback before I get too far?

[sfllaw]

❓ pyls supports SymbolInformation but not DocumentSymbol. Can I add another dependency, maybe on gopls, that would exercise this other code path?

[skangas]

I think an additional dependency is fine: use skip-unless to skip it if the gopls binary is not available as you do here with pyls.

[sfllaw]

I’m not sure how much you like using cl-labels to hide helper functions. I found that the backtraces were difficult to read when using cl-labels, but I can also appreciate keeping a small API. Thoughts?

[skangas]

It's largely a matter of taste. Personally, I'd use cl-labels for cleaner code and live with the suboptimal backtrace.

[sfllaw]

🎗️ I want to manage the recursion locally within eglot--make-symbol-overlay to avoid stack overflows with deeply nested children.

[sfllaw]

🔧 If we actually give variables and fields their proper priority because they are the most deeply nested, actually using (get-char-property (point) 'eglot-symbol-name) becomes unusable. This is my hack to work around this. Would you do something differently?

[sfllaw]

🙁 I’m not particularly happy about this implementation, since it relies on the side-effect of a missing 'eglot-symbol-containerName property to fallthrough. Also, this parses the overlays twice.

[sfllaw]

🐛 I think I will have to build this tree by hand, but I wasn’t smart enough when drafting this.

Do you mind adding a require for map.el? It is built-in to Emacs 26.1, which I think is the minimum version you support?

[skangas]

I see no problem with requiring map.el if we need it.

[joaotavora]

Yes, well, this is trickier. On the one hand, this solves a real problem IIRC which is that the advanced usage that Eglot makes of the imenu data structures, which are supported by the simple imenu command, are not supported by a number of imenu-like frontends that only understand the simple usage of those structures. The problem has been mentioned a number of times in the issues that Simon Links.

So solving this problem is very good.

On the other hand, it puts a lot of imenu code into eglot.el for things that should really be solved in imenu and/or which-func. This the typical reason why it would be better if Eglot lived in the core: it could more easily cooperate with changes to other packages, and all (or a subset of) those packages could still be published to (GNU|M|NonGNU)ELPA

But even without that integration into Emacs's core, I think that we should still consider changes to imenu and/or which-func.el in Emacs. It's just a question of making those into :core ELPA packages, doing any fixes/features in Emacs's trunk, and then adding them as dependencies to eglot.el. Much like flymake, project, jsonrpc, and others.

In summary: I think we should idealize any feature/bugfix of Eglot holystically within Emacs, even if Eglot is not integrated there yet.

But that's just one opinion, @skangas should chime in.

[skangas]

I fully agree with what @joaotavora says above.

@sfllaw Could you start looking into integrating the relevant changes into imenu.el and which-func.el in Emacs itself? If we need more recent versions of those files for eglot to work, no problem, we make them into core packages and release them on GNU ELPA as such, as João says.

[sfllaw]

Could you start looking into integrating the relevant changes into imenu.el and which-func.el in Emacs itself? If we need more recent versions of those files for eglot to work, no problem, we make them into core packages and release them on GNU ELPA as such, as João says.

@skangas, @joaotavora: I'm not sure which bits of this patch should be native to imenu and which-func?

This patch stores the result of LSP's textDocument/documentSymbol protocol in eglot-specific overlays. Then it renders those overlays into something imenu--index-alist, which is part of its documented API. The reason I chose overlays for eglot is that you can then ask for documentSymbol information at point.

However, I am unsure whether imenu or which-func should be using overlays by default. Are you proposing something else that I'm not understanding?

[skangas]

Could you start looking into integrating the relevant changes into imenu.el and which-func.el in Emacs itself? If we need more recent versions of those files for eglot to work, no problem, we make them into core packages and release them on GNU ELPA as such, as João says.

@skangas, @joaotavora: I'm not sure which bits of this patch should be native to imenu and which-func?

This patch stores the result of LSP's textDocument/documentSymbol protocol in eglot-specific overlays. Then it renders those overlays into something imenu--index-alist, which is part of its documented API. The reason I chose overlays for eglot is that you can then ask for documentSymbol information at point.

IIUC, I guess the question is: why doesn't imenu already support all the extended capabilities that we want? If it did, then all imenu frontends would support them "for free" (or at least they would do that eventually, as they add such support). So rather than implementing this in eglot, we should add the capabilities we need to imenu, and then extend eglot to use them.

In other words, we should perhaps not just move any specific bits from this patch at this point, but rather take a step back and think about how we could move the more general imenu stuff we currently have into imenu itself (or to which-func.el). Does that make sense?

(I have no idea if extending the imenu datastructures or using overlays is better.)

[sfllaw]

@skangas

IIUC, I guess the question is: why doesn't imenu already support all the extended capabilities that we want? If it did, then all imenu frontends would support them "for free" (or at least they would do that eventually, as they add such support). So rather than implementing this in eglot, we should add the capabilities we need to imenu, and then extend eglot to use them.

Oooh! I think we have a misunderstanding! The point of this pull request is to remove the use of special elements in imenu--index-alist, which is part of a vestigal imenu API that is effectively unused and unsupported:

;; The latest buffer index.
(defvar-local imenu--index-alist nil
  "The buffer index alist computed for this buffer in Imenu.

Simple elements in the alist look like (INDEX-NAME . POSITION).
POSITION is the buffer position of the item; to go to the item
is simply to move point to that position.

POSITION is passed to `imenu-default-goto-function', so it can be
a non-number if that variable has been changed (e.g. Semantic
uses overlays for POSITIONs).

Special elements look like
\(INDEX-NAME POSITION FUNCTION ARGUMENTS...).
To \"go to\" a special element means applying FUNCTION to
INDEX-NAME, POSITION, and the ARGUMENTS.

A nested sub-alist element looks like (INDEX-NAME . SUB-ALIST).
The function `imenu--subalist-p' tests an element and returns t
if it is a sub-alist.

There is one simple element with negative POSITION; selecting that
element recalculates the buffer's index alist.")

#758 (comment) suggested that I try replacing special elements with simple elements, which resulted in this patch. In that pull request, I suggested storing this information in a new eglot--document-symbol-alist but later realized that overlays might be more efficient and elegant.

If we don’t think it’s necessary to store the results of textDocument/documentSymbol anywhere, we could just throw it away after calculating imenu--index-alist. That would be the simplest implementation.

Again, I don't believe there is any need to change imenu or which-func, because we can use imenu’s simple elements like almost every other package does.

[skangas]

I hope that @joaotavora could explain what he had in mind, because I'm not familiar enough with this to have an opinion at this point. Thanks.

[sfllaw]

@joaotavora It seems like you’ve chosen to punt on explicit imenu support, based on fdd87b7. If not, and you want me to work on this, please feel free to reopen this issue and we can discuss how you want to approach this.

[joaotavora]

Hi @sfllaw ,

@joaotavora It seems like you’ve chosen to punt on explicit imenu support, based on fdd87b7. If not, and you want me to work on this, please feel free to reopen this issue and we can discuss how you want to approach this.

Yes and no. OT1H fdd87b7 is just fixing these bugs you mention in the description of this PR, and doing so in the simplest fashion I could think of. It actually removes code instead of adding.

OTOH it is a bit of a punt, because I've lost sight of the broader problem (if any) that you were solving with this PR. This is exclusively my fault: my attention is divided among so many things that I simply lose track of things.

So, if you could kindly remind me of what limitations the current version of the code fdd87b7 still has, I would be indebted.

[sfllaw]

Hi @joaotavora,

So, if you could kindly remind me of what limitations the current version of the code fdd87b7 still has, I would be indebted.

The which-func.el package suggests using overlays, since those are significantly more efficient than searching through Imenu’s alist for large files with many symbols.

https://github.com/emacs-mirror/emacs/blob/2f9f5e4850a65ce5ead0fd9ee934dfe29d2d01f3/lisp/progmodes/which-func.el#L45-L46:

;;     2. This package should be realized with the help of overlay
;; properties instead of the `imenu--index-alist' variable.

Above, I think you might have suggested that someone tackle this generally from inside Emacs itself. The reason I suggested this implementation is because eglot knows when new documentSymbol information has been loaded from LSP, whereas adding a variable watcher to imenu--index-alist seems kinda wrong.

Anyway, your implementation is Good Enough for small files, so maybe we shouldn’t do anything until someone complains?

[joaotavora]

Alright, i get it. Seems like this is now an optimization issue. Thanks for the update.