bug#69657: Missing imenu entries with eglot

bug#69657: Missing imenu entries with eglot

[Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- 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

bug#69657: Missing imenu entries with eglot

[Felician Nemeth]

> 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

bug#69657: Missing imenu entries with eglot

[Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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
        }
      }
    }
  ]
}

bug#69657: Missing imenu entries with eglot

[Felician Nemeth]

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.

bug#69657: Missing imenu entries with eglot

[João Távora]

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.

bug#69657: Missing imenu entries with eglot

[Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

bug#69657: Missing imenu entries with eglot

[Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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
    }
  ]
}

bug#69657: Missing imenu entries with eglot

[João Távora]

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

bug#69657: Missing imenu entries with eglot

[Sebastian Poeplau via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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?