* bug#69657: Missing imenu entries with eglot @ 2024-03-08 20:01 Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-08 21:44 ` Felician Nemeth 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-08 20:01 UTC (permalink / raw) To: 69657 [-- Attachment #1: Type: text/plain, Size: 883 bytes --] For language servers that reply to the textDocument/documentSymbol request with instances of DocumentSymbol rather than SymbolInformation, eglot's imenu builder omits symbols containing other symbols. This applies to eglot 1.17 from ELPA as well as the version in Emacs master as of today. You can use clangd to reproduce the problem: 1. Put this code in a C++ source file, e.g., test.cpp: class Foo { void bar() {} }; 2. Start eglot with clangd. 3. Invoke imenu; there will be a single entry "bar", nested under "Foo". In particular, there is no entry that lets you jump to Foo. This is inconsistent with how eglot handles language servers that return SymbolInformation (e.g., pylsp), and I would argue that it's not what users expect. The attached patch fixes the behavior by inserting a dedicated entry for each symbol in addition to entries for its children. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Eglot-include-containers-in-imenu.patch --] [-- Type: text/x-diff, Size: 1731 bytes --] From 953372f4cb1eea435a21a66e72226c7b2a6be51b Mon Sep 17 00:00:00 2001 From: Sebastian Poeplau <sebastian.poeplau@mailbox.org> Date: Fri, 8 Mar 2024 16:29:25 +0100 Subject: [PATCH] Eglot: include containers in imenu When the language server provides symbols as instances of DocumentSymbol, eglot now includes containers (i.e., symbols with children) in the imenu listing. This is consistent with eglot's behavior when symbols are given as SymbolInformation objects. * lisp/progmodes/eglot.el (eglot--imenu-DocumentSymbol): Include an entry for symbols with children. --- lisp/progmodes/eglot.el | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el index f341428cac3..87366811efd 100644 --- a/lisp/progmodes/eglot.el +++ b/lisp/progmodes/eglot.el @@ -3454,10 +3454,13 @@ for which LSP on-type-formatting should be requested." 'breadcrumb-region reg 'breadcrumb-kind kind))) (if (seq-empty-p children) - (cons name (car reg)) - (cons name - (mapcar (lambda (c) (apply #'dfs c)) children)))))) - (mapcar (lambda (s) (apply #'dfs s)) res))) + (list (cons name (car reg))) + (list (cons name (car reg)) + (cons name + (mapcan (lambda (c) + (apply #'dfs c)) + children))))))) + (mapcan (lambda (s) (apply #'dfs s)) res))) (cl-defun eglot-imenu () "Eglot's `imenu-create-index-function'. -- 2.43.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-08 20:01 bug#69657: Missing imenu entries with eglot Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-08 21:44 ` Felician Nemeth 2024-03-08 22:28 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 9+ messages in thread From: Felician Nemeth @ 2024-03-08 21:44 UTC (permalink / raw) To: Sebastian Poeplau; +Cc: 69657 > 1. Put this code in a C++ source file, e.g., test.cpp: > > class Foo { > void bar() {} > }; > > 2. Start eglot with clangd. > 3. Invoke imenu; there will be a single entry "bar", nested under "Foo". > In particular, there is no entry that lets you jump to Foo. How do you invoke imenu? If I type M-x imenu-add-menubar-index RET, then I cannot select "Foo" as you wrote above. However, if I apply the patch, the menu looks exactly the same. On the other hand, the patch seems to ruin breadcrumb-mode: putting the point after "{}", the breadcrumb header line shows: "dir/test.cpp: Class > Foo" instead of the original "dir/test.cpp: Class > Foo > bar". $ clangd --version Debian clangd version 14.0.6 Features: linux+grpc ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-08 21:44 ` Felician Nemeth @ 2024-03-08 22:28 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-09 16:20 ` Felician Nemeth 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-08 22:28 UTC (permalink / raw) To: Felician Nemeth; +Cc: 69657 > How do you invoke imenu? I did M-x imenu, although I should add that I normally use consult-imenu, which presents a flattened menu. That's also what I wrote the patch for. > If I type M-x imenu-add-menubar-index RET, then I cannot select "Foo" > as you wrote above. However, if I apply the patch, the menu looks > exactly the same. Indeed, it seems that for regular imenu the patch doesn't make a difference (presumably because both the category and the symbol have the same name). I had tested that the entry for "Foo" is absent, but I guess i should have verified that it shows up with the patch. In any case, for users of consult-imenu (and possibly others) there's a benefit, also in terms of consistency with language servers that use SymbolInformation. > On the other hand, the patch seems to ruin breadcrumb-mode: putting > the point after "{}", the breadcrumb header line shows: "dir/test.cpp: > Class > Foo" instead of the original "dir/test.cpp: Class > Foo > > bar". If you move point _after_ "{}", you're outside of method "bar", aren't you? Doesn't that mean the breadcrumb header is correct now and was wrong before? > $ clangd --version > Debian clangd version 14.0.6 > Features: linux+grpc I tested with clangd 15 from the Ubuntu repositories and clangd 17 from the repositories maintained by the LLVM project (for Ubuntu and Debian). But I don't think this is a matter of clangd versions; for the record, this is the textDocument/documentSymbol response I'm getting for the example in my original message: { "id": 5, "jsonrpc": "2.0", "result": [ { "children": [ { "detail": "void ()", "kind": 6, "name": "bar", "range": { "end": { "character": 15, "line": 1 }, "start": { "character": 2, "line": 1 } }, "selectionRange": { "end": { "character": 10, "line": 1 }, "start": { "character": 7, "line": 1 } } } ], "detail": "class", "kind": 5, "name": "Foo", "range": { "end": { "character": 1, "line": 2 }, "start": { "character": 0, "line": 0 } }, "selectionRange": { "end": { "character": 9, "line": 0 }, "start": { "character": 6, "line": 0 } } } ] } ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-08 22:28 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-09 16:20 ` Felician Nemeth 2024-03-10 1:05 ` João Távora 0 siblings, 1 reply; 9+ messages in thread From: Felician Nemeth @ 2024-03-09 16:20 UTC (permalink / raw) To: Sebastian Poeplau; +Cc: 69657, João Távora CC-ing João, the maintainer of breadcrumb-mode. > I did M-x imenu, although I should add that I normally use > consult-imenu, which presents a flattened menu. That's also what I wrote > the patch for. Without the patch, I can jump to "bar" (M-x imenu RET Foo RET bar RET) and cannot jump to "Foo". With the patch, I can jump to "Foo" (M-x imenu RET Foo RET) and cannot jump to "bar". So the default / basic behavior has changed, but I don't think it is a definit improvement. >> On the other hand, the patch seems to ruin breadcrumb-mode: putting >> the point after "{}", the breadcrumb header line shows: "dir/test.cpp: >> Class > Foo" instead of the original "dir/test.cpp: Class > Foo > >> bar". > > If you move point _after_ "{}", you're outside of method "bar", aren't > you? Doesn't that mean the breadcrumb header is correct now and was > wrong before? This is the first time I use breadcrumb, so I don't really know what is the exact correct behaviour. But without the patch there are locations for with "bar" is present it the breadcrumb header, and with the patch there isn't any. Maybe this is a bug in breadcrumb, but applying the current version of the patch is going to ruin an existing feature. Additionally, with the patch, the *Completions* buffer shows: "3 possible completions: *Rescan* Foo". So maybe there is a bug in imenu as well, since it seems it cannot handle items having the same name. > for the record, this is the textDocument/documentSymbol response I'm > getting for the example in my original message: [...] This is what I get as well, so luckily the different clangd versions don't cause problems. ---- By the way, I haven't used consult before, but after a quick look at it, I think consult-imenu could use the breadcrumb-kind text properties to show additional information in its live preview mode. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-09 16:20 ` Felician Nemeth @ 2024-03-10 1:05 ` João Távora 2024-03-10 16:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 9+ messages in thread From: João Távora @ 2024-03-10 1:05 UTC (permalink / raw) To: Felician Nemeth; +Cc: 69657, Sebastian Poeplau On Sat, Mar 9, 2024 at 4:20 PM Felician Nemeth <felician.nemeth@gmail.com> wrote: > > CC-ing João, the maintainer of breadcrumb-mode. I've made a file with class Foo { void bar() {} }; And the imenu--index-alist calculated by Eglot has what I think is the most sensible way to represent this within the ancient rigid limitations of that variable's format. ((#("Foo" 0 3 (breadcrumb-region (1 . 31) breadcrumb-kind "Class")) (#("bar" 0 3 (breadcrumb-region (15 . 28) breadcrumb-kind "Method")) . 15))) I understand Sebastian wants to see an extra top-evel ("Foo" . 1) there or something, but that leads to two entries with duplicate names and imenu just doesn't like that (if I apply the patch I just can't go to 'bar' with M -x imenu), neither do some of its clients. This is way Eglot emits "breadcrumb"-friendly cookies to overcome imenu's limitations. Using breadcrumb i can jump to Foo and Foo > bar just fine (using `M-x breadcrumb-jump`). These cookies are innocuous to anyone else. But I guess if someone took the time to consecrate them as imenu things instead of breadcrumb things. Then others UIs could use them. Or someone could invent something much better than imenu and have Eglot write to that idk. Imenu is pretty cursed, but it's what was already there so I picked it. Anyway, if users don't like Eglot's imenu function, they can use something else. Eglot's imenu is different from c++-ts-mode, and afaik c++-ts-mode's is already different from c++-mode's. I wouldn't install this patch though, teach other UIs about those optional cookies seems like a better deal that doesn't break stuff. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-10 1:05 ` João Távora @ 2024-03-10 16:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 19:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-10 16:15 UTC (permalink / raw) To: João Távora; +Cc: 69657, Felician Nemeth > I understand Sebastian wants to see an extra top-evel ("Foo" . 1) there > or something, but that leads to two entries with duplicate names and imenu > just doesn't like that (if I apply the patch I just can't go to 'bar' with M > -x imenu), neither do some of its clients. What about naming the separate entry differently, e.g., "Foo (top)"? I'll certainly look into using the breadcrumb metadata instead, but that doesn't help users of plain imenu. > This is way Eglot emits "breadcrumb"-friendly cookies to overcome imenu's > limitations. Using breadcrumb i can jump to Foo and Foo > bar just > fine (using > `M-x breadcrumb-jump`). Thanks for the hint, I didn't know about `breadcrumb-jump'; I'd only used the headline feature before. > Anyway, if users don't like Eglot's imenu function, they can use something > else. Eglot's imenu is different from c++-ts-mode, and afaik c++-ts-mode's is > already different from c++-mode's. I wouldn't install this patch though, > teach other UIs about those optional cookies seems like a better deal that > doesn't break stuff. I wonder why I did get separate entries in consult-imenu with pylsp though. I'll have to give it another look, but it seems to me that in this case (e.g., with SymbolInformation objects) eglot's imenu function returns data that (a) works with plain imenu and (b) gives me what I expect with consult-imenu. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-10 16:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-10 19:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 19:33 ` João Távora 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-10 19:15 UTC (permalink / raw) To: João Távora; +Cc: 69657, Felician Nemeth > I wonder why I did get separate entries in consult-imenu with pylsp > though. I'll have to give it another look, but it seems to me that in > this case (e.g., with SymbolInformation objects) eglot's imenu function > returns data that (a) works with plain imenu and (b) gives me what I > expect with consult-imenu. Here's the return value of eglot-imenu with pylsp: ((#1="Class" (#("Foo" 0 3 (breadcrumb-kind #1# breadcrumb-region (1 . 44))) . 1)) (#2="Method" ("Foo" (#("bar" 0 3 (breadcrumb-kind #2# breadcrumb-region (16 . 44))) . 16)))) The Python code I used is the following: class Foo: def bar(self): pass Like I said before, in this case I would consider the behavior of plain imenu, consult-imenu, and which-function-mode as correct, even though they don't use the breadcrumb annotations. I can jump to both the class and the method, and outside the method which-function-mode tells me that I'm in Foo. Maybe we could make eglot produce something similar for the data from clangd... For completeness, this is pylsp's response to textDocument/documentSymbol: { "jsonrpc": "2.0", "id": 21, "result": [ { "name": "Foo", "containerName": null, "location": { "uri": "file:///tmp/test.py", "range": { "start": { "line": 0, "character": 0 }, "end": { "line": 3, "character": 0 } } }, "kind": 5 }, { "name": "bar", "containerName": "Foo", "location": { "uri": "file:///tmp/test.py", "range": { "start": { "line": 1, "character": 4 }, "end": { "line": 3, "character": 0 } } }, "kind": 6 } ] } ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-10 19:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-10 19:33 ` João Távora 2024-03-17 11:04 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 9+ messages in thread From: João Távora @ 2024-03-10 19:33 UTC (permalink / raw) To: Sebastian Poeplau; +Cc: 69657, Felician Nemeth On Sun, Mar 10, 2024 at 7:22 PM Sebastian Poeplau <sebastian.poeplau@mailbox.org> wrote: > Here's the return value of eglot-imenu with pylsp: > > ((#1="Class" > (#("Foo" 0 3 (breadcrumb-kind #1# breadcrumb-region (1 . 44))) . 1)) > (#2="Method" > ("Foo" (#("bar" 0 3 (breadcrumb-kind #2# breadcrumb-region (16 . 44))) . 16)))) > For completeness, this is pylsp's response to > textDocument/documentSymbol: See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_documentSymbol pylsp does this because it is old, it uses SymbolInformation. clangd and most other servers use DocumentSymbol[] which is not only much more powerful and logical for represting a file's AST but also doesn't lend itself well to that structure of categorization, which is rather useless except for the fact that it makes leaves out of AST non-leaves. The problems are in Imenu. You should rather lobby for a better imenu structure (which will be hard to do fully backward compatibly will all existing UIs -- except if you use the string properties trick that Eglot/breadcrumb use as I explained) You could also lobby for a brand new imenu replacement in Emacs. João ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#69657: Missing imenu entries with eglot 2024-03-10 19:33 ` João Távora @ 2024-03-17 11:04 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-17 11:04 UTC (permalink / raw) To: João Távora; +Cc: 69657, Felician Nemeth > The problems are in Imenu. > > You should rather lobby for a better imenu structure (which will be > hard to do fully backward compatibly will all existing UIs -- except if > you use the string properties trick that Eglot/breadcrumb use as > I explained) You could also lobby for a brand new imenu replacement > in Emacs. I've been thinking about this. A "brand new imenu replacement" sounds like something that would be hard to get adoption for. So, assuming that we wanted to change the imenu structure to something that works better for representing ASTs, I think there are two somewhat related problems: (1) how to represent the AST internally, and (2) how to show it to the user. 1. The current representation in `imenu--index-alist', as you said, has the problem that it doesn't allow position information on inner nodes; also, additional metadata like the category has to be tagged on with text properties or other tricks. One could extend the existing structure. For example, leaves could become lists (name beg end category) with everything but NAME and BEG being optional, or something like that; inner nodes could optionally have the same format instead of being just strings. I don't see how to much such a change in a way that wouldn't break existing UIs, but maybe one could find a way that at least makes it easy for them to support the new structure (e.g., by providing helper functions in imenu.el, which could also facilitate future changes in `imenu--index-alist'). I guess while changing the structure one could try to optimize it for the use cases of existing UIs. Breadcrumb, for example, probably needs an efficient means of computing the most specific node corresponding to a buffer position, as well as the path down from the root to that node, and the list of its sibling nodes. 2. A menu structure like the one offered by M-x imenu or `imenu-add-menubar-index' makes it difficult to jump to inner nodes. The best I can think of is an entry labeled "(top)" or something like that underneath each leave node with a position; in my earlier example, the "Foo" menu entry would then expand to a list of two sub-entries, "(top)" and "bar". Alternative UIs probably have much better ways to display the information (such as the entry in consult-imenu that I was originally looking for). Even if this sounds like a nice improvement, I wonder about the compatibility issue. Do you think such a patch would even be considered? ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-17 11:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-08 20:01 bug#69657: Missing imenu entries with eglot Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-08 21:44 ` Felician Nemeth 2024-03-08 22:28 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-09 16:20 ` Felician Nemeth 2024-03-10 1:05 ` João Távora 2024-03-10 16:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 19:15 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors 2024-03-10 19:33 ` João Távora 2024-03-17 11:04 ` Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.