Subject: bug#72705: 31.0.50; eglot--dumb-tryc Filters out too much

From: Dmitry Gutov <[email-masked]>
Date: Sun, 25 Aug 2024 05:49:59 +0300

On 23/08/2024 13:23, João Távora wrote:

>> Great! Bump it to 1.17.60?
> 
> No, keep it at 1.17.30.  That's the 1.17-ish version of Eglot that will
> ship with Emacs 30, just as 1.12.29 is the 1.12-ish version of Eglot
> that shipped with Emacs 29.
> 
> The change you're proposing will eventually also be merged to master
> and be part of the 1.18 GNU Elpa release (earlier I wrote 1.17 GNU Elpa
> by mistake).

Perfect, thanks.

> Foreign as it might seem to Emacs devs and users, LSP doesn't want any
> concept of "prefix" (or suffix or commonality).  In general, two very
> closely related completions in the final insertion result can have two
> wildly different labels displayed to the user.  In this sense, the
> "completions" popup in LSP is less like our all-too-familiar listing of
> strings that can help complete the thing at point and more akin to a
> specialized context menu for "things to _do_ at point".  There's not
> even an LSP mandate that this thing to _do_ must include something to
> insert at point.  Currently it could be just tidying up imports
> elsewhere in the LSP document.  In future LSP versions, it could be just
> running an arbitrary server action, or sending an email.

I see what you're saying, but insofar that current completion is mostly 
working out well, adding the special logic for parens would improve a 
lot of cases, and keep others the same degree of broken (the 
email-sending ones, for sure). So it might be worth a try.

Anyway, stopping any partial completion (at first I didn't understand 
that you meant a more general notion that the partial-completion 
c-style) should be similarly easy to what the current patch does. You 
would just drop the second clause in eglot--dumb-tryc, I think. Or maybe 
both the first and the second.

> The other "fix" is to lobby with the LSP spec people, but they're very
> VSCode-inclined.  From what I've seen, even other editors which are more
> popular than Emacs have very little sway there.

It seems we only have our hacks to help. They got pretty advanced, though.

>> Yeah, it'd be great to have C-M-i stop before the paren.
> 
> No, it wouldn't.  It'd fix this particular case, but it could break in
> some other future version of LSP where the LSP label isn't a prefix of
> the thing that selecting that label would insert.

Quite possibly, yes. Though reverting to the simpler behavior at that 
point would just require a 3-4 lines diff, as discussed.

>>>> An alternative might be to use a completion-all-completions call, to
>>>> assert the full list of completions. Or some completions that are in
>>>> it anyway.
>>> Yes, I generally prefer "end-to-end" tests using interactive
>>> interfaces.
>>> So unless you're duplicating the test I think this part should be
>>> skipped.  But your call.
>>
>> It's a different test (albeit with the same result as an existing
>> one). I don't mind rewriting it in a different style, if you
>> prefer. The result is kind of messy either way.
> 
> My suggestion is just test that *Completions* pops up.

Alas, this check does not succeed:

   (should (get-buffer-window "*Completions"))

This works:

   (when (buffer-live-p "*Completions*")
     (kill-buffer "*Completions*"))
   (completion-at-point)
   (should (looking-back "foo"))
   (should (looking-at "123"))
   (should (get-buffer "*Completions*"))

Is that better?

>>>> It's the same case as '("foo123" . 3) in the test above, isn't it?
>>> I don't think so.  In a rust-analyzer hello world (which you can
>>> make
>>> with cargo init, if I'm not mistaken), both
>>>      v.c<cursor>1234.12345676890
>>>      v.count_on<cursor>1234.12345676890
>>> should expand to
>>>      v.count_ones()<cursor>.1234567890
>>> In the first case, you'll have to manually select "count_ones" from
>>> the
>>> completions buffer.  The expansion and removal of the 1234 happens via
>>> LSP text edits, which are enacte by the :exit-function.
>>
>> Okay, I am seeing a difference too: C-M-i does eat the suffix in the
>> Rust example (the "1234"). But completion with Company does not :-/
> 
> I can't even get Company to appear in those situations.

You might have an older version installed?

> But I wouldn't bother about it.  The bug report was about C-M-i
> originally.  The important thing is that the LSP textEdit is run (via
> exit-function) with the correct LSP document state that the server
> expects.  As far as I can tell, this is happening right now, so I'd be
> nice to have a test to shore it up.

Okay, see the attached patch with the added test.

It took way longer than expected. Seems pretty brittle (note how 
eglot--wait-for-rust-analyzer is defined), but should be better than 
nothing.

I also checked how VS Code behaves in such completions, since we were 
talking about what LSP's authors designed for or not. Works differently:

1) Completions are not filtered by suffix, ever.
2) Completing count_ones() does not delete the suffix ("1234" remains).

It might mean that either behavior is okay, though.

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 353877f60c2..59d9c346424 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3142,8 +3142,18 @@ eglot--dumb-allc
 (defun eglot--dumb-tryc (pat table pred point)
   (let ((probe (funcall table pat pred nil)))
     (cond ((eq probe t) t)
-          (probe (cons probe (length probe)))
-          (t (cons pat point)))))
+          (probe
+           (if (and (not (equal probe pat))
+                    (cl-every
+                     (lambda (s) (string-prefix-p probe s completion-ignore-case))
+                     (funcall table pat pred t)))
+               (cons probe (length probe))
+             (cons pat point)))
+          (t
+           ;; Match ignoring suffix: if there are any completions for
+           ;; the current prefix at least, keep the current input.
+           (and (funcall table (substring pat 0 point) pred t)
+                (cons pat point))))))
 
 (add-to-list 'completion-category-defaults '(eglot-capf (styles eglot--dumb-flex)))
 (add-to-list 'completion-styles-alist '(eglot--dumb-flex eglot--dumb-tryc eglot--dumb-allc))
diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
index 534c47b2646..41824f477b8 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -587,6 +587,18 @@ eglot--wait-for-clangd
     (eglot--wait-for (s-notifs 20) (&key method &allow-other-keys)
       (string= method "textDocument/publishDiagnostics"))))
 
+(defun eglot--wait-for-rust-analyzer ()
+  (eglot--sniffing (:server-notifications s-notifs)
+    (should (eglot--tests-connect))
+    (eglot--wait-for (s-notifs 20) (&key method params &allow-other-keys)
+      (and
+       (string= method "$/progress")
+       "rustAnalyzer/Indexing"
+       (equal params
+              '(:token "rustAnalyzer/Indexing" :value
+                       ;; Could wait for :kind "end" instead, but it's 2 more seconds.
+                       (:kind "begin" :title "Indexing" :cancellable :json-false :percentage 0)))))))
+
 (ert-deftest eglot-test-basic-completions ()
   "Test basic autocompletion in a clangd LSP."
   (skip-unless (executable-find "clangd"))
@@ -600,6 +612,20 @@ eglot-test-basic-completions
       (message (buffer-string))
       (should (looking-back "fprintf.?")))))
 
+(ert-deftest eglot-test-common-prefix-completion ()
+  "Test completion appending the common prefix."
+  (skip-unless (executable-find "clangd"))
+  (eglot--with-fixture
+      `(("project" . (("coiso.c" .
+                       ,(concat "int foo_bar; int foo_bar_baz;"
+                                "int main() {foo")))))
+    (with-current-buffer
+        (eglot--find-file-noselect "project/coiso.c")
+      (eglot--wait-for-clangd)
+      (goto-char (point-max))
+      (completion-at-point)
+      (should (looking-back "{foo_bar")))))
+
 (ert-deftest eglot-test-non-unique-completions ()
   "Test completion resulting in 'Complete, but not unique'."
   (skip-unless (executable-find "clangd"))
@@ -619,19 +645,99 @@ eglot-test-non-unique-completions
         (forward-line -1)
         (should (looking-at "Complete, but not unique")))))))
 
-(ert-deftest eglot-test-basic-xref ()
-  "Test basic xref functionality in a clangd LSP."
+(ert-deftest eglot-test-stop-completion-on-nonprefix ()
+  "Test completion also resulting in 'Complete, but not unique'."
+  (skip-unless (executable-find "clangd"))
+  (eglot--with-fixture
+      `(("project" . (("coiso.c" .
+                       ,(concat "int foot; int footer; int fo_obar;"
+                                "int main() {foo")))))
+    (with-current-buffer
+        (eglot--find-file-noselect "project/coiso.c")
+      (eglot--wait-for-clangd)
+      (goto-char (point-max))
+      (completion-at-point)
+      (should (looking-back "foo")))))
+
+(ert-deftest eglot-test-try-completion-nomatch ()
+  "Test completion table with non-matching input, returning nil."
+  (skip-unless (executable-find "clangd"))
+  (eglot--with-fixture
+      `(("project" . (("coiso.c" .
+                       ,(concat "int main() {abc")))))
+    (with-current-buffer
+        (eglot--find-file-noselect "project/coiso.c")
+      (eglot--wait-for-clangd)
+      (goto-char (point-max))
+      (should
+       (null
+        (completion-try-completion
+         "abc"
+         (nth 2 (eglot-completion-at-point)) nil 3))))))
+
+(ert-deftest eglot-test-try-completion-inside-symbol ()
+  "Test completion table inside symbol, with only prefix matching."
   (skip-unless (executable-find "clangd"))
   (eglot--with-fixture
       `(("project" . (("coiso.c" .
-                       ,(concat "int foo=42; int fooey;"
-                                "int main() {foo=82;}")))))
+                       ,(concat
+                         "int foobar;"
+                         "int main() {foo123")))))
     (with-current-buffer
         (eglot--find-file-noselect "project/coiso.c")
-      (should (eglot--tests-connect))
-      (search-forward "{foo")
-      (call-interactively 'xref-find-definitions)
-      (should (looking-at "foo=42")))))
+      (eglot--wait-for-clangd)
+      (goto-char (- (point-max) 3))
+      (when (buffer-live-p "*Completions*")
+        (kill-buffer "*Completions*"))
+      (completion-at-point)
+      (should (looking-back "foo"))
+      (should (looking-at "123"))
+      (should (get-buffer "*Completions*"))
+      )))
+
+(ert-deftest eglot-test-rust-completion-exit-function ()
+  "Hover and highlightChanges."
+  (skip-unless (executable-find "rust-analyzer"))
+  (skip-unless (executable-find "cargo"))
+  (eglot--with-fixture
+      '(("cmpl-project" .
+         (("main.rs" .
+           "fn test() -> i32 { let v: usize = 1; v.count_on1234.1234567890;"))))
+    (with-current-buffer
+        (eglot--find-file-noselect "cmpl-project/main.rs")
+      (should (zerop (shell-command "cargo init")))
+      (eglot--tests-connect)
+      (goto-char (point-min))
+      (search-forward "v.count_on")
+      (let ((minibuffer-message-timeout 0)
+            ;; Fail at (ding) if completion fails.
+            (executing-kbd-macro t))
+        (when (buffer-live-p "*Completions*")
+          (kill-buffer "*Completions*"))
+        (eglot--wait-for-rust-analyzer)
+        (completion-at-point)
+        (should (looking-back "\\.count_on"))
+        (should (get-buffer "*Completions*"))
+        (minibuffer-next-completion 1)
+        (minibuffer-choose-completion t))
+      (should
+       (equal
+        "fn test() -> i32 { let v: usize = 1; v.count_ones().1234567890;"
+        (buffer-string))))))
+
+(ert-deftest eglot-test-basic-xref ()
+  "Test basic xref functionality in a clangd LSP."
+  (skip-unless (executable-find "clangd"))
+  (eglot--with-fixture
+   `(("project" . (("coiso.c" .
+                    ,(concat "int foo=42; int fooey;"
+                             "int main() {foo=82;}")))))
+   (with-current-buffer
+       (eglot--find-file-noselect "project/coiso.c")
+     (should (eglot--tests-connect))
+     (search-forward "{foo")
+     (call-interactively 'xref-find-definitions)
+     (should (looking-at "foo=42")))))
 
 (defvar eglot--test-c-buffer
   "\