bug#62687: 30.0.50; Eglot (eglot--sig-info): SignatureInformation's Documentation is never shown when it's of type MarkupContent

bug#62687: 30.0.50; Eglot (eglot--sig-info): SignatureInformation's Documentation is never shown when it's of type MarkupContent

[Mekeor Melire]

[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]

What's the problem?

When you enable eglot-mode from Eglot 1.14; when it then 
successfully
connects to a language server; when you then move the cursor to
"something callable", Eglot is supposed to show the Documentation 
from
the SignatureInformation, as specified in the language server 
protocol.
This SignatureInformation's Documentation can either be of type 
string,
or of type MarkupContent [1]. Eglot will never show it, if it's of
latter type.

Why does this happen?

You enable eglot-mode. Eglot successfully connects to a language 
server.
Eglot enables eldoc-mode. When you move the cursor, the function
`eglot-signature-eldoc-function' is called because it's a member 
of
`eldoc-documentation-functions'. This function,
`eglot-signature-eldoc-function`, calls `eglot--sig-info'. That 
function
will only show the documentation if it's of type string [2].

--8<---------------cut here---------------start------------->8---
        ;; Decide whether to add one-line-summary to signature 
        line
        (when (and (stringp documentation)
                   (string-match "[[:space:]]*\\([^.\r\n]+[.]?\\)"
                                 documentation))
          (setq documentation (match-string 1 documentation))
          (unless (string-prefix-p (string-trim documentation) 
          label)
            (goto-char (point-max))
            (insert ": " (eglot--format-markup documentation))))
--8<---------------cut here---------------end--------------->8---


How to see this happen?

    Install GNU Emacs from master-branch; use any commit after 
    "Eglot:
    Bump to 1.14" 8125d4cfc5605ead9102b7d823c4241029eb76cc. You 
    might
    need to build with tree-sitter and install a tree-sitter 
    grammar for
    typescript, so that you can use `typescript-ts-mode' because 
    Emacs
    has no other mode for typescript.

    Install Git and Node.js, including NPM.

    Install typescript-language-server:
        npm install --global typescript-language-server

    Clone the minimal-reproducing-example:
        git clone 
        https://github.com/mekeor/emacs-eglot-minimal-reproducing-example
        cd emacs-eglot-minimal-reproducing-example

    Install the project dependencies:
        npm install

    Open source file with Emacs:
        emacs ./main.ts

    In Emacs, type:
        M-x typescript-ts-mode RET
        M-x eglot RET
        C-3 C-5 C-2 C-f

    In the echo-area, you will see the type signature of the 
    function
    "fastify.addHook", but there will be no documentation:
        

[-- Attachment #2: 2023-04-06-000337_496x610_scrot.png --]
[-- Type: image/png, Size: 59585 bytes --]

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]



[1] 
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#signatureInformation

[2] 
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/eglot.el?id=fa669c4b17c04eff852eb23a6179ccb8fab864db#n3132

bug#62687: 30.0.50; Eglot (eglot--sig-info): SignatureInformation's Documentation is never shown when it's of type MarkupContent

[Mekeor Melire]

Sorry for the bad formatting of my bug report. Let me try to send 
the same content a second time, hopefully with better line 
wrapping.


What's the problem?

When you enable eglot-mode from Eglot 1.14; when it then 
successfully connects to a language server; when you then move the 
cursor to "something callable", Eglot is supposed to show the 
Documentation from the SignatureInformation, as specified in the 
language server protocol. This SignatureInformation's 
Documentation can either be of type string, or of type 
MarkupContent [1]. Eglot will never show it, if it's of latter 
type.

Why does this happen?

You enable eglot-mode. Eglot successfully connects to a language 
server. Eglot enables eldoc-mode. When you move the cursor, the 
function `eglot-signature-eldoc-function' is called because it's a 
member of `eldoc-documentation-functions'. This function, 
`eglot-signature-eldoc-function`, calls `eglot--sig-info'. That 
function will only show the documentation if it's of type string 
[2].

--8<---------------cut here---------------start------------->8---
;; Decide whether to add one-line-summary to signature line
(when (and (stringp documentation)
           (string-match "[[:space:]]*\\([^.\r\n]+[.]?\\)"
                         documentation))
  (setq documentation (match-string 1 documentation))
  (unless (string-prefix-p (string-trim documentation) label)
    (goto-char (point-max))
    (insert ": " (eglot--format-markup documentation))))
--8<---------------cut here---------------end--------------->8---

How to see this happen?

1. Install GNU Emacs from master-branch; use any commit after 
"Eglot: Bump to 1.14" 8125d4cfc5605ead9102b7d823c4241029eb76cc. 
You might need to build with tree-sitter and install a tree-sitter 
grammar for typescript, so that you can use `typescript-ts-mode' 
because Emacs has no other mode for typescript.

2. Install Git and Node.js, including NPM.

3. Install typescript-language-server:

    npm install --global typescript-language-server

4. Clone the minimal-reproducing-example:

    git clone 
    https://github.com/mekeor/emacs-eglot-minimal-reproducing-example; 
    cd emacs-eglot-minimal-reproducing-example

5. Install the project dependencies:

    npm install

6. Open source file with Emacs:

    emacs ./main.ts

7. In Emacs, type:

    M-x typescript-ts-mode RET
    M-x eglot RET
    C-3 C-5 C-2 C-f

8. In the echo-area, you will see the type signature of the 
function "fastify.addHook", but there will be no documentation; 
see the attached screenshot.

[1] 
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#signatureInformation

[2] 
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/eglot.el?id=fa669c4b17c04eff852eb23a6179ccb8fab864db#n3132

bug#62687: 30.0.50; Eglot (eglot--sig-info): SignatureInformation's Documentation is never shown when it's of type MarkupContent

[Mekeor Melire]

Let's take a closer look at the relevant code snippet; understand 
its bug; understand its features; and try to come up with a 
solution.

2023-04-06 19:21 mekeor@posteo.de:

> (when (and (stringp documentation)

This line is the reason why the documentation is only echoed if 
it's of type string, and why it will never be shown in case it's 
markup content.

>            (string-match "[[:space:]]*\\([^.\r\n]+[.]?\\)"
>                          documentation))
>   (setq documentation (match-string 1 documentation))

Here, we trim the beginning of the documentation-string and 
extract the first sentence; or rather: We extract the text up to 
the first dot.

(I guess here is another bug because this regex will cut 
"a().chain().like().this()" (which might occur inside a 
documentation-string) off right after the first dot.)

I guess, the motivation for these lines is to reduce the size of 
the echoed documentation message; i.e. prevent flooding of the 
echo-area. Large echo-areas take up much of the screen and are 
annoying.

Personally, I don't think that we should extract only one sentence 
or so. Instead, I think, `max-mini-window-height' (and 
`eldoc-echo-area-use-multiline-p') suffices to configure the 
maximum size of the echo-area.

But, if we decide to stick to the idea of only echoing the first 
sentence, then I wonder: How do we implement this feature for 
documentation-markups? How to extract the first sentence of a 
markup? `string-match' would not work, right?

>   (unless (string-prefix-p (string-trim documentation) label)

This line makes sure that we do not echo the "first sentence" of 
the documentation-string if it's a prefix of the label. How often 
does this happen? Has this been reported as a bug before? It seems 
rather unlikely to me.

Also, if we decide to stick to this feature, then I wonder: How to 
recreate the logic of `string-prefix-p' for documentations of type 
markup?

>     (goto-char (point-max))
>     (insert ": " (eglot--format-markup documentation))))

Here, we finally format the "first sentence" of the 
documentation-string and insert it into the temporal buffer which 
will be echoed later.

Also, does it make sense to pass a documentation of type string 
into `eglot--format-markup' which will format it as 
GitHub-Flavored-Markdown (GFM) although the documentation is not 
of type markup?

bug#62687: 30.0.50; Eglot (eglot--sig-info): SignatureInformation's Documentation is never shown when it's of type MarkupContent

[Mekeor Melire]

[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]

2023-04-06 19:49 mekeor@posteo.de:

> 2023-04-06 19:21 mekeor@posteo.de:

> >            (string-match "[[:space:]]*\\([^.\r\n]+[.]?\\)"
> >                          documentation))
> >   (setq documentation (match-string 1 documentation))
>
> Here, we trim the beginning of the documentation-string and 
> extract the first sentence; or rather: We extract the text up to 
> the first dot.
>
> (I guess here is another bug because this regex will cut 
> "a().chain().like().this()" (which might occur inside a 
> documentation-string) off right after the first dot.)
>
> I guess, the motivation for these lines is to reduce the size of 
> the echoed documentation message; i.e. prevent flooding of the 
> echo-area. Large echo-areas take up much of the screen and are 
> annoying.
>
> Personally, I don't think that we should extract only one 
> sentence or so. Instead, I think, `max-mini-window-height' (and 
> `eldoc-echo-area-use-multiline-p') suffices to configure the 
> maximum size of the echo-area.
>
> But, if we decide to stick to the idea of only echoing the first 
> sentence, then I wonder: How do we implement this feature for 
> documentation-markups? How to extract the first sentence of a 
> markup? `string-match' would not work, right?

Another idea would be to echo the documentation up to the first 
newline. That would work especially nice if we stick to use ": " 
as separator between label and documentation. See below.

> >   (unless (string-prefix-p (string-trim documentation) label)
>
> This line makes sure that we do not echo the "first sentence" of 
> the documentation-string if it's a prefix of the label. How 
> often does this happen? Has this been reported as a bug before? 
> It seems rather unlikely to me.
>
> Also, if we decide to stick to this feature, then I wonder: How 
> to recreate the logic of `string-prefix-p' for documentations of 
> type markup?
>
> >     (goto-char (point-max))
> >     (insert ": " (eglot--format-markup documentation))))
>
> Here, we finally format the "first sentence" of the 
> documentation-string and insert it into the temporal buffer 
> which will be echoed later.
>
> Also, does it make sense to pass a documentation of type string 
> into `eglot--format-markup' which will format it as 
> GitHub-Flavored-Markdown (GFM) although the documentation is not 
> of type markup?

Additionally, I don't think that ": " is a good separator between 
the previously inserted label and the documentation. I think we 
should use a newline.

Alternatively, we could make that separator customizable.

But if we decide to stick to ": " as separator, here's a minimal 
patch that makes Eglot echo the SignatureInformation's 
Documentation -- even if it's of type MarkupContent:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 62687.patch --]
[-- Type: text/x-patch, Size: 1385 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 10b6c0cc2ca..a4f859745e7 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3128,14 +3128,12 @@ for which LSP on-type-formatting should be requested."
           (setq params-start (match-beginning 2) params-end (match-end 2))
           (add-face-text-property (match-beginning 1) (match-end 1)
                                   'font-lock-function-name-face))
-        ;; Decide whether to add one-line-summary to signature line
-        (when (and (stringp documentation)
-                   (string-match "[[:space:]]*\\([^.\r\n]+[.]?\\)"
-                                 documentation))
-          (setq documentation (match-string 1 documentation))
-          (unless (string-prefix-p (string-trim documentation) label)
-            (goto-char (point-max))
-            (insert ": " (eglot--format-markup documentation))))
+        ;; Insert documentation
+        (goto-char (point-max))
+        (unless (null documentation)
+          (insert ": " (if (stringp documentation)
+                         documentation
+                         (eglot--format-markup documentation))))
         ;; Decide what to do with the active parameter...
         (when (and active-param (< -1 active-param (length parameters)))
           (eglot--dbind ((ParameterInformation) label documentation)

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

[-- Attachment #1: Type: text/plain, Size: 2706 bytes --]

Tags: patch

1. I already reported this issue with Eglot's eglot--sig-info:

    (when (and (stringp documentation)

It does not show the SignatureInformation's documentation if it's 
not a string but rather of type markup.

This issue is addressed by the attached patch.


2. I found another bug while reading the code:

    (concat "\\<" (regexp-quote label) "\\>")

Here, we try to find the ParameterInformation's label within the 
parameters. And that's the regex used for searching. 
Unfortunately, this regex won't match if the label has a non-word 
character at its beginning or end. Take a look at these examples:

    ELISP> (string-match "\\<bar: \"BAR\"" "foo(bar: \"BAR\", 
    baz)")
    4 (#o4, #x4, ?\C-d)

    ELISP> (string-match "\\<bar: \"BAR\"\\>" "foo(bar: \"BAR\", 
    baz)")
    nil

I tried to use "\\(\\`\\|\\W\\|\\=\\)" instead of "\\<" and 
"\\(\\'\\|\\W\\)" instead of "\\>", but that fixes the problem 
only for the first parameter, somehow. Maybe you have an idea 
which regex would fit here?

Until then, the attached patch just removes the "\\<" and "\\>" 
parts.


3. eglot--sig-info does not highlight the active parameter if it 
does not match the <name>(<params>) pattern. Here's the 
responsible code:

    ;; Ad-hoc attempt to parse label as <name>(<params>)
    (when (looking-at "\\([^(]*\\)(\\([^)]+\\))")
      (setq params-start (match-beginning 2) params-end (match-end 
      2))
      ;; ...
      )
    (when params-start
      ;; ...
      (add-face-text-property
        beg end
        'eldoc-highlight-function-argument))

But we are in fact able to highlight the active parameter, if 
ParameterInformation's label is a pair of numbers, denoting the 
active parameter's position. We just need to nest our conditions 
the other way around.

This issue is addressed by the attached patch.


4. The "documentation" field of both SignatureInformation and 
ParameterInformation can be either of type string or of type 
MarkupContent, according to LSP 3.17. I think we should not format 
the documentation as GitHub-Flavored-Markdown (GFM) because it 
might lead to wrong interpretations/displayings of the doc-string.

This is addressed by the attached patch.


5. There is a little overhead on pattern matching on a pair. 
Instead of

    ((`(,beg ,end)
      (if COND
        (list FOO BAR)
        (append CONS-CELL nil))))

we could simply:

    ((`(,beg . ,end)
      (if COND
        (cons FOO BAR)
        CONS-CELL)))

This is also addressed in the attached patch.


6. Using ": " and "\n" to separate information is not enough of 
separation. This is fixed by the attached patch by using "\n\n" 
instead. Should we make it a customizable variable?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 62687.patch --]
[-- Type: text/patch, Size: 4593 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 10b6c0cc2ca..42989b9e73e 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3120,6 +3120,7 @@ for which LSP on-type-formatting should be requested."
   (eglot--dbind ((SignatureInformation) label documentation parameters activeParameter)
       sig
     (with-temp-buffer
+      ;; Insert SignatureInformation label
       (save-excursion (insert label))
       (let ((active-param (or activeParameter sig-help-active-param))
             params-start params-end)
@@ -3128,44 +3129,40 @@ for which LSP on-type-formatting should be requested."
           (setq params-start (match-beginning 2) params-end (match-end 2))
           (add-face-text-property (match-beginning 1) (match-end 1)
                                   'font-lock-function-name-face))
-        ;; Decide whether to add one-line-summary to signature line
-        (when (and (stringp documentation)
-                   (string-match "[[:space:]]*\\([^.\r\n]+[.]?\\)"
-                                 documentation))
-          (setq documentation (match-string 1 documentation))
-          (unless (string-prefix-p (string-trim documentation) label)
-            (goto-char (point-max))
-            (insert ": " (eglot--format-markup documentation))))
+        ;; Insert SignatureInformation documentation
+        (goto-char (point-max))
+        (unless (null documentation)
+          (insert "\n\n"
+            (if (stringp documentation)
+              (string-trim documentation)
+              (eglot--format-markup documentation))))
         ;; Decide what to do with the active parameter...
         (when (and active-param (< -1 active-param (length parameters)))
           (eglot--dbind ((ParameterInformation) label documentation)
               (aref parameters active-param)
             ;; ...perhaps highlight it in the formals list
-            (when params-start
-              (goto-char params-start)
-              (pcase-let
-                  ((`(,beg ,end)
-                    (if (stringp label)
-                        (let ((case-fold-search nil))
-                          (and (re-search-forward
-                                (concat "\\<" (regexp-quote label) "\\>")
-                                params-end t)
-                               (list (match-beginning 0) (match-end 0))))
-                      (mapcar #'1+ (append label nil)))))
-                (if (and beg end)
-                    (add-face-text-property
-                     beg end
-                     'eldoc-highlight-function-argument))))
-            ;; ...and/or maybe add its doc on a line by its own.
+            (pcase-let
+              ((`(,beg . ,end)
+                 (if (stringp label)
+                   (if params-start
+                     (let ((case-fold-search nil))
+                       (goto-char params-start)
+                       (and (re-search-forward
+                              (regexp-quote label)
+                              params-end t)
+                         (cons (match-beginning 0) (match-end 0)))))
+                   (mapcar #'1+ label))))
+              (if (and beg end)
+                (add-face-text-property
+                  beg end
+                  'eldoc-highlight-function-argument)))
+            ;; ...and maybe add its doc on a line by its own.
             (when documentation
               (goto-char (point-max))
-              (insert "\n"
-                      (propertize
-                       (if (stringp label)
-                           label
-                         (apply #'buffer-substring (mapcar #'1+ label)))
-                       'face 'eldoc-highlight-function-argument)
-                      ": " (eglot--format-markup documentation))))))
+              (insert "\n\n"
+                (if (stringp documentation)
+                  (string-trim documentation)
+                  (eglot--format-markup documentation)))))))
       (buffer-string))))
 
 (defun eglot-signature-eldoc-function (cb)
@@ -3183,7 +3180,7 @@ for which LSP on-type-formatting should be requested."
                                   (aref signatures (or activeSignature 0)))))
              (if (not active-sig) (funcall cb nil)
                (funcall cb
-                        (mapconcat #'eglot--sig-info signatures "\n")
+                        (mapconcat #'eglot--sig-info signatures "\n\n")
                         :echo (eglot--sig-info active-sig t activeParameter))))))
        :deferred :textDocument/signatureHelp))
     t))

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

Hello Mekeor,

You wrote 5 long messages back to back in this bug, reporting
multiple issues and bugs and wishes. I'm not one to request
TL;DR or executive summaries usually, but here I must, as I
am a single threaded person.

Can you try to summarize the main issue in less than 20 lines?
Else, I'm going to find it very hard to be able to help you.


João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

Also, in this message:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62687#8

You give a very nice bug report (that nevertheless requires me
to install software to reproduce, but that's life).  But you
forgot to attach the Eglot events buffer printout for that 8-step
recipe.  Please do so.

João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

On April 8, 2023 1:49:20 AM GMT+02:00, "João Távora" <joaotavora@gmail.com> wrote:
>Hello Mekeor,

Hello João!

>You wrote 5 long messages back to back in this bug, reporting
>multiple issues and bugs and wishes. I'm not one to request
>TL;DR or executive summaries usually, but here I must, as I
>am a single threaded person.
>
>Can you try to summarize the main issue in less than 20 lines?
>Else, I'm going to find it very hard to be able to help you.

The main issue is that Eglot won't show the "SignatureInformation"'s "documentation" field if it is of type MarkupContent.

If you want, I can create separate bug reports for the other bugs I found in the eglot--sig-info function. But I think my last e-mail was quite concise.

>João

Mekeor

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

Mekeor Melire <mekeor@posteo.de> writes:

>>You wrote 5 long messages back to back in this bug, reporting
>>multiple issues and bugs and wishes. I'm not one to request
>>TL;DR or executive summaries usually, but here I must, as I
>>am a single threaded person.
>>
>>Can you try to summarize the main issue in less than 20 lines?
>>Else, I'm going to find it very hard to be able to help you.
>
> The main issue is that Eglot won't show the "SignatureInformation"'s
> "documentation" field if it is of type MarkupContent.

OK I see it, I understand it, and I've done some changes to master.
Please have a look at commit 685435cb52eaa6f61b7088398f1f53e69d76e63e.
Note that the general principle here is that the echo area gets as few
lines as possible, and the *eldoc* buffer gets much more.  So it's a
good idea to have the *eldoc* buffer showing somewhere (which you can
summon with 'C-h .').

> If you want, I can create separate bug reports for the other bugs I
> found in the eglot--sig-info function. 

Let's focus on this "main issue" before we move on to others.

> But I think my last e-mail was quite concise.

The problem is you wrote 5 of them :-) Should I just focus on your last
email and ignore the others, then?  If so what exactly is the debbugs
link for this last "concise" email?

João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

2023-04-08 20:47 joaotavora@gmail.com:

> Mekeor Melire <mekeor@posteo.de> writes:
>
> > The main issue is that Eglot won't show the 
> > "SignatureInformation"'s "documentation" field if it is of 
> > type MarkupContent.
>
> OK I see it, I understand it, and I've done some changes to 
> master. Please have a look at commit 
> 685435cb52eaa6f61b7088398f1f53e69d76e63e.

Thank you! That commit indeed makes Eglot show 
"SignatureInformation"'s "documentation" field even if it's of 
type MarkupContent. So it solves this main issue.

Unfortunately, that commit causes Eglot to not show the 
"ParameterInformation"'s "documenation" field. I propose to show 
both the SigInfo- and the ParamInfo-documentation, whenever 
possible. (To be more precise: First, the SigInfo-doc should be 
shown, if non-nil. Then, the ParamInfo-doc should be shown, if 
non-nil.) What do you think?

Would you like to have an audio-call to talk about the problems 
related to `eglot--sig-info'? I think it would be much more 
efficient than email.

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]

On Sat, Apr 8, 2023, 22:04 Mekeor Melire <mekeor@posteo.de> wrote:

> 2023-04-08 20:47 joaotavora@gmail.com:
>
> > Mekeor Melire <mekeor@posteo.de> writes:
> >
> > > The main issue is that Eglot won't show the
> > > "SignatureInformation"'s "documentation" field if it is of
> > > type MarkupContent.
> >
> > OK I see it, I understand it, and I've done some changes to
> > master. Please have a look at commit
> > 685435cb52eaa6f61b7088398f1f53e69d76e63e.
>
> Thank you! That commit indeed makes Eglot show
> "SignatureInformation"'s "documentation" field even if it's of
> type MarkupContent. So it solves this main issue.
>
> Unfortunately, that commit causes Eglot to not show the
> "ParameterInformation"'s "documenation" field. I propose to show
> both the SigInfo- and the ParamInfo-documentation, whenever
> possible. (To be more precise: First, the SigInfo-doc should be
> shown, if non-nil. Then, the ParamInfo-doc should be shown, if
> non-nil.) What do you think?
>

Ok, i took it out, because I didn't find any server that used it. Pylsp
uses it, but the doc is always the empty string. Does your TS server use
it? Did you get around to providing the event log i asked for?

Would you like to have an audio-call to talk about the problems
> related to `eglot--sig-info'? I think it would be much more
> efficient than email.
>

I'm on GMT time, but I don't know when I'll be available. Mail me off-list.

João

[-- Attachment #2: Type: text/html, Size: 2455 bytes --]

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

João Távora <joaotavora@gmail.com> writes:

>  Thank you! That commit indeed makes Eglot show 
>  "SignatureInformation"'s "documentation" field even if it's of 
>  type MarkupContent. So it solves this main issue.
>
>  Unfortunately, that commit causes Eglot to not show the 
>  "ParameterInformation"'s "documenation" field. I propose to show 
>  both the SigInfo- and the ParamInfo-documentation, whenever 
>  possible. (To be more precise: First, the SigInfo-doc should be 
>  shown, if non-nil. Then, the ParamInfo-doc should be shown, if 
>  non-nil.) What do you think?

I did more changes to master taking that into account.  See
e33c0a549153fa3894f3b5e9c5e42ce07a1a68c7 and tell me if there's any more
stuff missing.

> Ok, i took it out, because I didn't find any server that used
> it. Pylsp uses it, but the doc is always the empty string.  Does your
> TS server use it? Did you get around to providing the event log i
> asked for?

Never mind, I downloaded typescript-language-server and checked it out
myself.

João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

2023-04-08 23:31 joaotavora@gmail.com:

> João Távora <joaotavora@gmail.com> writes:
>
> > Unfortunately, that commit causes Eglot to not show the 
> > "ParameterInformation"'s "documenation" field. I propose to 
> > show both the SigInfo- and the ParamInfo-documentation, 
> > whenever possible. (To be more precise: First, the SigInfo-doc 
> > should be shown, if non-nil. Then, the ParamInfo-doc should be 
> > shown, if non-nil.) What do you think?
>
> I did more changes to master taking that into account. See 
> e33c0a549153fa3894f3b5e9c5e42ce07a1a68c7 and tell me if there's 
> any more stuff missing.

Thank you. That commit is very useful. Let's move on to the next 
thing: Variable-names.

If you don't want me tamper with variable-names, then that's fine, 
just let me know and I will further move to the next thing.

Otherwise, I'd suggest a coherent naming of the variables. 
Specifically, I think we should derive the variable-names from the 
LSP-types (e.g. "SignatureHelp" and "ParameterInformation"). A 
patch is attached. Feel free to apply it or do something similar 
on your own.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 62687-2023-04-09-rename-variables.patch --]
[-- Type: text/x-patch, Size: 3931 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 3f00281e155..3cfdba6b550 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3118,28 +3118,30 @@ for which LSP on-type-formatting should be requested."
   (mapconcat #'eglot--format-markup
              (if (vectorp contents) contents (list contents)) "\n"))
 
-(defun eglot--sig-info (sig &optional sig-active briefp)
+(defun eglot--sig-info (sig &optional sig-help-active-par briefp)
   (eglot--dbind ((SignatureInformation)
-                 ((:label siglabel))
-                 ((:documentation sigdoc)) parameters activeParameter)
+                  ((:label sig-info-label))
+                  ((:documentation sig-info-doc))
+                  parameters
+                  ((:activeParameter sig-info-active-par)))
       sig
     (with-temp-buffer
-      (save-excursion (insert siglabel))
+      (save-excursion (insert sig-info-label))
       ;; Ad-hoc attempt to parse label as <name>(<params>)
         (when (looking-at "\\([^(]*\\)(\\([^)]+\\))")
           (add-face-text-property (match-beginning 1) (match-end 1)
                                   'font-lock-function-name-face))
         ;; Add documentation, indented so we can distinguish multiple signatures
-        (when-let (doc (and (not briefp) sigdoc (eglot--format-markup sigdoc)))
+        (when-let (doc (and (not briefp) sig-info-doc (eglot--format-markup sig-info-doc)))
           (goto-char (point-max))
           (insert "\n" (replace-regexp-in-string "^" "  " doc)))
         ;; Now to the parameters
         (cl-loop
-         with active-param = (or sig-active activeParameter)
+         with active-param = (or sig-help-active-par sig-info-active-par)
          for i from 0 for parameter across parameters do
          (eglot--dbind ((ParameterInformation)
-                        ((:label parlabel))
-                        ((:documentation pardoc)))
+                        ((:label par-label))
+                        ((:documentation par-doc)))
              parameter
            ;; ...perhaps highlight it in the formals list
            (when (and (eq i active-param))
@@ -3147,24 +3149,24 @@ for which LSP on-type-formatting should be requested."
                (goto-char (point-min))
                (pcase-let
                    ((`(,beg ,end)
-                     (if (stringp parlabel)
+                     (if (stringp par-label)
                          (let ((case-fold-search nil))
-                           (and (search-forward parlabel (line-end-position) t)
+                           (and (search-forward par-label (line-end-position) t)
                                 (list (match-beginning 0) (match-end 0))))
-                       (mapcar #'1+ (append parlabel nil)))))
+                       (mapcar #'1+ (append par-label nil)))))
                  (if (and beg end)
                      (add-face-text-property
                       beg end
                       'eldoc-highlight-function-argument)))))
            ;; ...and/or maybe add its doc on a line by its own.
            (let (fpardoc)
-             (when (and pardoc (not briefp)
+             (when (and par-doc (not briefp)
                         (not (string-empty-p
-                              (setq fpardoc (eglot--format-markup pardoc)))))
+                              (setq fpardoc (eglot--format-markup par-doc)))))
                (insert "\n  "
                        (propertize
-                        (if (stringp parlabel) parlabel
-                          (apply #'substring siglabel (mapcar #'1+ parlabel)))
+                        (if (stringp par-label) par-label
+                          (apply #'substring sig-info-label (mapcar #'1+ par-label)))
                         'face (and (eq i active-param) 'eldoc-highlight-function-argument))
                        ": " fpardoc)))))
       (buffer-string))))

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

Mekeor Melire <mekeor@posteo.de> writes:

> 2023-04-08 23:31 joaotavora@gmail.com:
>
>> João Távora <joaotavora@gmail.com> writes:
>>
>> > Unfortunately, that commit causes Eglot to not show the  >
>> "ParameterInformation"'s "documenation" field. I propose to  > show
>> both the SigInfo- and the ParamInfo-documentation,  > whenever
>> possible. (To be more precise: First, the SigInfo-doc  > should be
>> shown, if non-nil. Then, the ParamInfo-doc should be  > shown, if
>> non-nil.) What do you think?
>>
>> I did more changes to master taking that into account. See
>> e33c0a549153fa3894f3b5e9c5e42ce07a1a68c7 and tell me if there's any
>> more stuff missing.
>
> Thank you. That commit is very useful. Let's move on to the next
> thing: Variable-names.

If you mean the names of the formal parameters of a given function and
the ability to display them in the docstring, I think the latest version
handles that already.  I've tested with jdtls,
typescript-language-server, pylsp, and others.

If you mean the local variables names in the Elisp code, then please
let's not touch them.  They might not be up to your standards or ideal
naming, but they help me remember this code, and I have no intention of
changing them.

> If you don't want me tamper with variable-names, then that's fine,
> just let me know and I will further move to the next thing.

You told me my last commit introduced another problem, so I just want to
understand what that new problem is.

> Otherwise, I'd suggest a coherent naming of the
> variables. Specifically, I think we should derive the variable-names
> from the LSP-types (e.g. "SignatureHelp" and
> "ParameterInformation"). A patch is attached. Feel free to apply it or
> do something similar on your own.

Reading your patch, all I see is changed variable names and aliases,
which doesn't help much in reading what behaviour you actually want to
change.  It seems to do exactly the same.  Can you do one of the
following?

1. Re-do your patch without changing names like "siglabel" to
   "sig-info-label".

2. Explain what the problem is in terms of user-visible behaviour.  Like
   "I see 'fooey: foo factor' and I would like to see 'barey: foo
   factor'"

Thanks,
João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

[-- Attachment #1: Type: text/plain, Size: 4886 bytes --]

2023-04-10 08:14 joaotavora@gmail.com:

> If you mean the names of the formal parameters of a given 
> function

No, I did not mean those. Sorry if I wasn't clear enough about 
this.

> If you mean the local variables names in the Elisp code, then 
> please let's not touch them.

Alright.

> You told me my last commit introduced another problem, so I just 
> want to understand what that new problem is.

> 2. Explain what the problem is in terms of user-visible 
> behaviour. Like "I see 'fooey: foo factor' and I would like to 
> see 'barey: foo factor'"

In my opinion, it makes most sense to discuss the problems of this 
function by the order of its lines of code. Since the first lines 
introduce variables, I had decided to discuss their naming first.

Also, I never claimed that the problem, that your last commit 
introduced, was related to user-visible behavior.

Decide yourself how you want to proceed. I hope this still counts 
as "single threaded" to you, since I don't request you to do 
something about all of these things at the same time, but I just 
want you to decide how we go on. Please, just choose one single 
path to take and continue it together. Don't reply to all of these 
things. Otherwise, everything will be chaotic (because there are 
interdependencies) and our conversation won't be single-threaded 
at all.

1. If you are interested in non-semantical code changes and 
specifically about indentation of eglot.el: The indentation of all 
lines of eglot--sig-info, beginning at the following line, needs 
to be adjusted by two spaces to the left: 
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/eglot.el?h=e33c0a549153fa3894f3b5e9c5e42ce07a1a68c7#n3129

2. If you are interested in semantical code changes that are not 
related to user-visible behavior but rather efficiency, one such 
problem is: For highlighting the active parameter in the formals 
list, we don't need to compare the index of each parameter to the 
known index of the active parameter. Instead, we can just access 
it, as it was done before the last commit on eglot.el. A 
minimalist patch for this issue is attached.

3.: (That's the rest of this email.)

If you are only interested in user-visible behavior: I'd like to 
discuss the visibility of documentation inside the echo-area.

Generally, I agree that we should keep the echoed message short in 
order to avoid flooding and distraction from the actual code that 
the user is editing. But at the same time, the documentation that 
LSP provides is one of its key features. So, there is a trade-off 
between keeping the echoed message short and quickly accessible 
useful documentation.

I guess, there are many different approaches to this trade-off, 
including the following:

A: We keep it the way it is. That is, we keep Eglot echoing only 
the SignatureInformation label, and there within, highlighting the 
active parameter, if possible.

B: We make Eglot echo the SignatureInformation label (where 
ParameterInformation label is highlighted, if possible) and 
SignatureInformation documentation and ParameterInformation 
documentation.

C: We make this configurable. In the simplest case, the 
configuration-variable would be a boolean to turn echoed-docs on 
or off. Alternatively, a more complex configuration-variable would 
be a list of symbols out of 'signature-information-label 
'signature-information-documentation and 
'parameter-information-documentation, which would also allow to 
specify the order of these.

D: We introduce a new function, similar to 
eglot-signature-eldoc-function, which will (leverage Eldoc in 
order to) not only echo the SigInfo-label, but also SigInfo- and 
ParamInfo-doc. This function could be bound to a key.

Personally, I'd vote for B (echo docs by default) together with C 
(add a configuration-variable). And I strongly vote against A 
(keep as-is).

Maybe you are interested in how other LSP-clients solve this 
problem. Personally, I only use Emacs+Eglot, but I did some 
research:

α: lsp-mode (for Emacs) seems to use Eldoc by default and echoes 
both the SigInfo-label and some docs. (I'm not sure if SigInfo- 
and/or ParamInfo-docs.) lsp-mode also offers a variable to turn 
docs off: lsp-signature-render-documentation. 
https://emacs-lsp.github.io/lsp-mode/tutorials/how-to-turn-off/ 
(13.)

β: lsp-ui, an addon for lsp-mode, show the label and both docs for 
the thing at point in a child-frame. 
https://emacs-lsp.github.io/lsp-ui/#lsp-ui-doc

γ: Neovim offers the function vim.lsp.buf.signature_help which is 
often bound to C-k but can also be configured to be called on the 
thing when/where the cursor holds. It will show SigInfo-label, a 
horizontal line, SigInfo- and finally ParamInfo-doc. A screenshot 
of this is attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 62687-2023-04-10-no-loop-index-comparison-for-active-param.patch --]
[-- Type: text/x-patch, Size: 3149 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 3f00281e155..247ebaf42da 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3133,30 +3133,34 @@ for which LSP on-type-formatting should be requested."
         (when-let (doc (and (not briefp) sigdoc (eglot--format-markup sigdoc)))
           (goto-char (point-max))
           (insert "\n" (replace-regexp-in-string "^" "  " doc)))
+      ;; Perhaps highlight active parameter in the formals list
+      (let
+        ((active-param (or sig-active activeParameter)))
+        (when (and active-param (< -1 active-param (length parameters)))
+          (save-excursion
+            (goto-char (point-min))
+            (pcase-let
+              ((`(,beg ,end)
+                 (eglot--dbind ((ParameterInformation)
+                                 ((:label parlabel)))
+                   (aref parameters active-param)
+                   (if (stringp parlabel)
+                     (let ((case-fold-search nil))
+                       (and (search-forward parlabel (line-end-position) t)
+                         (list (match-beginning 0) (match-end 0))))
+                     (mapcar #'1+ (append parlabel nil))))))
+              (if (and beg end)
+                (add-face-text-property
+                  beg end
+                  'eldoc-highlight-function-argument)))))
         ;; Now to the parameters
         (cl-loop
-         with active-param = (or sig-active activeParameter)
          for i from 0 for parameter across parameters do
          (eglot--dbind ((ParameterInformation)
                         ((:label parlabel))
                         ((:documentation pardoc)))
              parameter
-           ;; ...perhaps highlight it in the formals list
-           (when (and (eq i active-param))
-             (save-excursion
-               (goto-char (point-min))
-               (pcase-let
-                   ((`(,beg ,end)
-                     (if (stringp parlabel)
-                         (let ((case-fold-search nil))
-                           (and (search-forward parlabel (line-end-position) t)
-                                (list (match-beginning 0) (match-end 0))))
-                       (mapcar #'1+ (append parlabel nil)))))
-                 (if (and beg end)
-                     (add-face-text-property
-                      beg end
-                      'eldoc-highlight-function-argument)))))
-           ;; ...and/or maybe add its doc on a line by its own.
+           ;; Maybe add its doc on a line by its own.
            (let (fpardoc)
              (when (and pardoc (not briefp)
                         (not (string-empty-p
@@ -3166,7 +3170,7 @@ for which LSP on-type-formatting should be requested."
                         (if (stringp parlabel) parlabel
                           (apply #'substring siglabel (mapcar #'1+ parlabel)))
                         'face (and (eq i active-param) 'eldoc-highlight-function-argument))
-                       ": " fpardoc)))))
+                       ": " fpardoc))))))
       (buffer-string))))
 
 (defun eglot-signature-eldoc-function (cb)

[-- Attachment #3: neovim-lsp-signature-help.png --]
[-- Type: image/png, Size: 54666 bytes --]

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

Mekeor Melire <mekeor@posteo.de> writes:

> Also, I never claimed that the problem, that your last commit
> introduced, was related to user-visible behavior.

OK, that's really what I and Eglot users are mostly interested in.

> Decide yourself how you want to proceed. 

I'll close this bug.  The problem reported is fixed and I can't discern
any others.  Your remarks are noted here for posterity.

I saw your patch, but I don't think it has any effect on efficiency.  If
you think otherwise, report it in another bug, and I'd prefer if it is
accompanied with some kind of reproducible performance measurement in
addition to the list of things requested in the Troubleshooting page I
linked to earlier.

My best and friendly regards,
João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On April 11, 2023 1:16:49 PM GMT+02:00, "João Távora" <joaotavora@gmail.com> wrote:
>Mekeor Melire <mekeor@posteo.de> writes:
>
>> Also, I never claimed that the problem, that your last commit
>> introduced, was related to user-visible behavior.
>
>OK, that's really what I and Eglot users are mostly interested in.
>
>> Decide yourself how you want to proceed. 
>
>I'll close this bug.  The problem reported is fixed and I can't discern
>any others.  Your remarks are noted here for posterity.
>
>I saw your patch, but I don't think it has any effect on efficiency.  If
>you think otherwise, report it in another bug, and I'd prefer if it is
>accompanied with some kind of reproducible performance measurement in
>addition to the list of things requested in the Troubleshooting page I
>linked to earlier.
>
>My best and friendly regards,
>João

Would you be fine with me creating another bug report / feature request, aiming to make it configurable, whether docs should be shown in the echo-area?

[-- Attachment #2: Type: text/html, Size: 1573 bytes --]

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

Mekeor Melire <mekeor@posteo.de> writes:

> Would you be fine with me creating another bug report / feature
> request, aiming to make it configurable, whether docs should be shown
> in the echo-area?

You can create any bug reports you think are necessary, I don't have a
say in that :-) That said, IMO there's already plenty of configuration
knobs for what gets shown where, so don't expect much enthusiasm from me
personally.

João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[João Távora]

On Tue, Apr 11, 2023 at 1:54 PM João Távora <joaotavora@gmail.com> wrote:
>
> Mekeor Melire <mekeor@posteo.de> writes:
>
> > Would you be fine with me creating another bug report / feature
> > request, aiming to make it configurable, whether docs should be shown
> > in the echo-area?
>
> You can create any bug reports you think are necessary, I don't have a
> say in that :-) That said, IMO there's already plenty of configuration
> knobs for what gets shown where, so don't expect much enthusiasm from me
> personally.

I will say, though, that some of these ElDoc mechanisms are somewhat
poorly documented in the Emacs manual.  So if you have time and energy
on your hands, writing an ElDoc manual (separate section, same
section, or even separate manual -- Eli is the person to contact
about that), is a worthy endeavor IMO.

João

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Eli Zaretskii]

> Cc: 62687@debbugs.gnu.org
> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 11 Apr 2023 14:53:11 +0100
> 
> I will say, though, that some of these ElDoc mechanisms are somewhat
> poorly documented in the Emacs manual.  So if you have time and energy
> on your hands, writing an ElDoc manual (separate section, same
> section, or even separate manual -- Eli is the person to contact
> about that), is a worthy endeavor IMO.

Yes, ElDoc's documentation "needs work".  It originally documented
only its support for Lisp, so lots of later-added functionality is not
yet documented well enough.

bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire]

2023-04-08 22:12 joaotavora@gmail.com:

> Ok, i took it out, because I didn't find any server that used 
> it. [...] Does your TS server use it?

For the record: Yes, typescript-language-server uses 
ParameterInformation documentation. You can use this this source 
code to check it out. Just move the cursor to the first 
"undefined".

#+begin_src typescript
/**
 ,* Returns the average of two numbers.
 ,*
 ,* @param x - The first input number
 ,* @param y - The second input number
 ,* @returns The arithmetic mean of `x` and `y`
 ,*
 ,* @beta
 ,*/
function getAverage(x: number, y: number): number {
  return (x + y) / 2.0;
}

getAverage(undefined as number, undefined as number);
#+end_src


The value of this parameter's documentation will be:

#+begin_src emacs-lisp
(:kind "markdown" :value "- The first input number")
#+end_src