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

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

[Dmitry Gutov]

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

X-Debbugs-Cc: joaotavora@gmail.com

1. Have a project with just one file, test.go (attached).
2. Visit it, enable go-ts-mode, call 'M-x eglot'.
3. Move point right after "math.As", press C-M-i.
4. Code will get completed to "math.Asin"

This is a problem because "math.As" has more completions (one can look
at them by pressing TAB after "math.A") - such as "Acos", "Acosh" and
"Abs" - in other words, "fuzzy" matches.

Expected behavior:

1. When input is "math.As" - keep the string as-is.
2. When input is "math.Asi" - complete to "math.Asin".

This problem seems older than 65ea742ed5ec (the change that introduced
eglot--dumb-tryc itself, bug#68699), but it doesn't reproduce with Eglot
from Emacs 29. Branches emacs-30 and master are affected.

There is also another issue there: when there are no completions at all,
this style still has completion-try-completion return a non-nil value
(the last line of the implementation).

Both of these issues is something I came across when working on closer
'try-completion' integration for company-mode
(https://github.com/company-mode/company-mode/pull/1488) and testing out
the new code with Eglot.

Not sure what's the best fix, but the patch below seems to address both
problems in my limited testing. WDYT?

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 353877f60c2..e8823a9d2f0 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3142,8 +3142,14 @@ 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)))))
+          ((and probe
+                (cl-every
+                 (lambda (s) (string-prefix-p probe s 
completion-ignore-case))
+                 (funcall table pat pred t)))
+           (cons probe (length probe)))
+          (t
+           (and (funcall table pat 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))

[-- Attachment #2: test.go --]
[-- Type: text/x-go, Size: 54 bytes --]

package test

import "math"

func main() {
	math.As
}

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

[João Távora]

On Mon, Aug 19, 2024 at 2:52 AM Dmitry Gutov <dmitry@gutov.dev> wrote:

> has more completions
> (one can look at them by pressing TAB after "math.A")

That's not relevant.  LSP servers can make completely different
decisions in each position.  What's relevant is to look at the completion
list returned in the "math.As" case (via the events buffer, for example).

> Not sure what's the best fix, but the patch below seems to address both
> problems in my limited testing. WDYT?

I would be (pleasantly) surprised if your change doesn't break something
else.  I tried many many variations in the fairly recent past and each
of them seemed to break something. Be wary of :exit-function
which is where things like snippet conversion and edits happen.
So I'd say this need more testing, especially with more servers.

I also don't understand why the "string-prefix" is being used.
The "dumb" in eglot--dumb-try should be taken to heart.

See logs of commits:
d376462c7183752bf44b9bd20bf5020fe7eaf75a
4dcbf61c1518dc53061707aeff8887517e050003
a6ef458e3831001b0acad57cf8fa75b77a4aff3f

You should also encode whatever decisions you make in
tests in eglot-tests.el.

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

[Dmitry Gutov]

On 19/08/2024 12:22, João Távora wrote:
> On Mon, Aug 19, 2024 at 2:52 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
>> has more completions
>> (one can look at them by pressing TAB after "math.A")
> 
> That's not relevant.  LSP servers can make completely different
> decisions in each position.  What's relevant is to look at the completion
> list returned in the "math.As" case (via the events buffer, for example).

True, but it's hard to look at completions for "math.As" using 
completion-at-point because try-completion behaves this way.

You can also check out Company's completions popup for "math.As" - it 
shows the extra ones.

>> Not sure what's the best fix, but the patch below seems to address both
>> problems in my limited testing. WDYT?
> 
> I would be (pleasantly) surprised if your change doesn't break something
> else.  I tried many many variations in the fairly recent past and each
> of them seemed to break something. Be wary of :exit-function
> which is where things like snippet conversion and edits happen.
> So I'd say this need more testing, especially with more servers.

Here's hoping.

> I also don't understand why the "string-prefix" is being used.
> The "dumb" in eglot--dumb-try should be taken to heart.

string-prefix-p is being used because 'try-completion' will ignore 
non-prefix completions as if they weren't there. We need to make sure 
that the proposed "probe" is indeed a prefix for all completions.

> See logs of commits:
> d376462c7183752bf44b9bd20bf5020fe7eaf75a
> 4dcbf61c1518dc53061707aeff8887517e050003
> a6ef458e3831001b0acad57cf8fa75b77a4aff3f
> 
> You should also encode whatever decisions you make in
> tests in eglot-tests.el.

Thanks for the references, I'll dig in more.

Surprised to hear that exit-function can be affected.

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

[João Távora]

On Mon, Aug 19, 2024 at 12:39 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> > So I'd say this need more testing, especially with more servers.
> Here's hoping.

If you want this change, you'll have to do the testing.

> Thanks for the references, I'll dig in more.
>
> Surprised to hear that exit-function can be affected.

exit-function needs a fully formed completion.  try-completion's
and Emacs partial complete semantics aren't compatible.  ISTR
that even a full completion obtained via try-completion would
sometimes not run the function.  Maybe I'm misremembering.

This whole system is held  up by very thin wires unfortunately,
and a lot of people are relying on those wires.  The best choice
to use LSP's completions, which were obviously modelled after
other IDEs where completions are much simpler and similar to
the Company tooltip...   is  naturally to use something like
company (or corfu with some patch applied).

I hope you don't change any Company behaviour backward-incompatibly
(though I have my fork anyway, so no problem).

João

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

[Dmitry Gutov]

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

On 19/08/2024 15:59, João Távora wrote:
> On Mon, Aug 19, 2024 at 12:39 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
>>> So I'd say this need more testing, especially with more servers.
>> Here's hoping.
> 
> If you want this change, you'll have to do the testing.

So far I've tested with gopls and clangd (when writing the new tests),
typescript-language-server and rust-analyzer.

>> Thanks for the references, I'll dig in more.
>>
>> Surprised to hear that exit-function can be affected.
> 
> exit-function needs a fully formed completion.  try-completion's
> and Emacs partial complete semantics aren't compatible.  ISTR
> that even a full completion obtained via try-completion would
> sometimes not run the function.  Maybe I'm misremembering.

All right.

The proposed change doesn't alter the kinds of strings that are 
inserted, only the cases when that happens. When the added predicate 
returns nil, we fall back to the next clause; and the check at the end 
also allows us to return nil, which is useful in certain rare contexts.

> This whole system is held  up by very thin wires unfortunately,
> and a lot of people are relying on those wires.  The best choice
> to use LSP's completions, which were obviously modelled after
> other IDEs where completions are much simpler and similar to
> the Company tooltip...   is  naturally to use something like
> company (or corfu with some patch applied).
> 
> I hope you don't change any Company behaviour backward-incompatibly
> (though I have my fork anyway, so no problem).

The change is in a pending PR, it's designed to be opt-in on the backend 
level, but company-capf is a separate backend, so... you know.

It alters what happens when you explicitly press TAB, which previously 
only did prefix-matching inside Company code, but now can delegate to 
backends for some extra smarts (longer expansions being the goal), which 
unfortunately don't work too well with LSP.

The report that's referenced in the 3 commits your mentioned does seem 
to have regressed is https://github.com/joaotavora/eglot/issues/1339 - 
not to the original behavior (exit-function still works), but C-M-i 
changes buffer text to

   v.call1234.1234567890

Not sure how important that case is, but it's a consequence of having 
the style return nil in try-completion and unavoidable failover to 
'basic' because of that (in completion--styles, because a category can't 
actually override, only prepend styles).

I suppose that could be fixed by moving some matching logic from the 
style into the completion table. Not sure how important/realistic the 
example is, although somebody did report it...

In the meantime, here's an updated patch with 3 new tests (and existing 
ones all passing). The implementation has been retouched too for better 
performance (the typescript server in particular made things slower with 
the previous proposal).

[-- Attachment #2: eglot--dumb-tryc-more-checks.diff --]
[-- Type: text/x-patch, Size: 3445 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 353877f60c2..744f2b26c3b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3142,8 +3142,16 @@ 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
+           (and (funcall table pat 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..625be2ea30f 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -600,6 +600,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,6 +633,36 @@ eglot-test-non-unique-completions
         (forward-line -1)
         (should (looking-at "Complete, but not unique")))))))
 
+(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-basic-xref ()
   "Test basic xref functionality in a clangd LSP."
   (skip-unless (executable-find "clangd"))

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

[João Távora]

On Tue, Aug 20, 2024 at 3:08 AM Dmitry Gutov <dmitry@gutov.dev> wrote:

> So far I've tested with gopls and clangd (when writing the new tests),
> typescript-language-server and rust-analyzer.

That's pretty good.  If you could add pylsp or pyright (basedpyright),
I'd feel even more confident.

> All right.
>
> The proposed change doesn't alter the kinds of strings that are
> inserted, only the cases when that happens. When the added predicate
> returns nil, we fall back to the next clause; and the check at the end
> also allows us to return nil, which is useful in certain rare contexts.

I don't have time to spend examining these details, just the final
outcome is relevant: partial completions cannot be inserted, but
fragments of a completion can be completed to a fully formed
completion (eventually running exit-function then).

> The report that's referenced in the 3 commits your mentioned does seem
> to have regressed is https://github.com/joaotavora/eglot/issues/1339
> not to the original behavior (exit-function still works), but C-M-i
> changes buffer text to
>
>    v.call1234.1234567890

I don't have time to investigate right now, but indeed that issue there were
two consecutive fixes: the first one (which you say "still works") and a second
one which I eventually reverted because it fixes some things and broke
others.

So is the regression you mention in relation to the current Eglot state or
to that intermediate state?

Regardless, if possible, make an automated test and mark it "expected
failing" to record this fact.

> I suppose that could be fixed by moving some matching logic from the
> style into the completion table. Not sure how important/realistic the
> example is, although somebody did report it...

I'd have to have a clearer view on what exactly has regressed and what
has advanced.  Then a better decision can be made.  Thank you for
this work.

João

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

[Dmitry Gutov]

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

On 20/08/2024 12:40, João Távora wrote:
> On Tue, Aug 20, 2024 at 3:08 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
>> So far I've tested with gopls and clangd (when writing the new tests),
>> typescript-language-server and rust-analyzer.
> 
> That's pretty good.  If you could add pylsp or pyright (basedpyright),
> I'd feel even more confident.

Installed pylsp with apt - the basic things I tested seem to work too.

> I don't have time to spend examining these details, just the final
> outcome is relevant: partial completions cannot be inserted, but
> fragments of a completion can be completed to a fully formed
> completion (eventually running exit-function then).

And only fragments that prefix-match all completions as prefix. 
exit-function seems to function.

>> The report that's referenced in the 3 commits your mentioned does seem
>> to have regressed is https://github.com/joaotavora/eglot/issues/1339
>> not to the original behavior (exit-function still works), but C-M-i
>> changes buffer text to
>>
>>     v.call1234.1234567890
> 
> I don't have time to investigate right now, but indeed that issue there were
> two consecutive fixes: the first one (which you say "still works") and a second
> one which I eventually reverted because it fixes some things and broke
> others.

Now that I've looked at them a few more times - all 3 changes were in 
the completion table, whereas I'm altering the completion style here. 
And indeed what seems to be the crux of the previous improvement 
(working exit-function) stays around.

> So is the regression you mention in relation to the current Eglot state or
> to that intermediate state?

In relation to the current state, but I think I've figured it out - see 
the new revision attached.

So what I think what was happening, is since the style was returning nil 
when the input has a non-matching suffix, the 'emacs22' style came into 
play (style which only looks at the prefix) and expanded the input from 
"c" to "call".

Anyway, the attached avoids the failover by returning non-nil in the 
last clause, if at least the prefix matches the table.

That makes "C-M-i" keep the input at "v.c|1234" (with two completions: 
"match" and "call") - but selecting either of the completions using 
Company or the mouse, or M-RET, expands each into the corresponding 
snippet, which seems most optimal all-around.

See the new test 'eglot-test-try-completion-inside-symbol' inside the 
patch. Wasn't sure whether to make it use 'completion-try-completion' or 
'completion-at-point', but the former seems more explicit.

[-- Attachment #2: eglot--dumb-tryc-more-checks-v2.diff --]
[-- Type: text/x-patch, Size: 4279 bytes --]

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..4f745b26b42 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -600,6 +600,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,6 +633,55 @@ eglot-test-non-unique-completions
         (forward-line -1)
         (should (looking-at "Complete, but not unique")))))))
 
+(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 foobar;"
+                         "int main() {foo123")))))
+    (with-current-buffer
+        (eglot--find-file-noselect "project/coiso.c")
+      (eglot--wait-for-clangd)
+      (goto-char (- (point-max) 3))
+      (should
+       (equal
+        '("foo123" . 3)
+        (completion-try-completion
+         "foo123"
+         (nth 2 (eglot-completion-at-point)) nil 3))))))
+
 (ert-deftest eglot-test-basic-xref ()
   "Test basic xref functionality in a clangd LSP."
   (skip-unless (executable-find "clangd"))

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

[João Távora]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 20/08/2024 12:40, João Távora wrote:
>> On Tue, Aug 20, 2024 at 3:08 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>> 
>>> So far I've tested with gopls and clangd (when writing the new tests),
>>> typescript-language-server and rust-analyzer.
>> That's pretty good.  If you could add pylsp or pyright
>> (basedpyright),
>> I'd feel even more confident.
>
> Installed pylsp with apt - the basic things I tested seem to work too.

Great.

>> I don't have time to spend examining these details, just the final
>> outcome is relevant: partial completions cannot be inserted, but
>> fragments of a completion can be completed to a fully formed
>> completion (eventually running exit-function then).
>
> And only fragments that prefix-match all completions as
> prefix. exit-function seems to function.

Also good.

> In relation to the current state, but I think I've figured it out -
> see the new revision attached.

Great.

> See the new test 'eglot-test-try-completion-inside-symbol' inside the
> patch. Wasn't sure whether to make it use 'completion-try-completion'
> or 'completion-at-point', but the former seems more explicit.

This all looks very good, especially the abundant tests.  You may push
right now if you want.

I'll just comment on those:

+(ert-deftest eglot-test-common-prefix-completion ()
+  "Test completion appending the common prefix."
+  (skip-unless (executable-find "clangd"))
+  (eglot--with-fixture
+      `(("project" . (("coiso.c" .

Eh.  You may replace "coiso.c" for the Russian equivalent of "thingy.c"
if you want :-).  Or something more imaginative.

+                       ,(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")))))

Here, we completed to foo_bar and it's a fully formed completion.  I
understand you've checked that exit-function does run.  This is good.

Unfortunately, if the the two ints were named "foo_bar_1" and
"foo_bar_2" it would also complete to foo_bar_, which is wrong in the
general case, since that text is not a fully formed completion.
exit-function cannot run here.

It happens to "work" here because clangd supplies the "insertText" as
well as the "textEdit" property and that "insertText" isn't meaningless.
The LSP spec recommendation is for servers to prefer supplying textEdit.

    * The `insertText` is subject to interpretation by the client side.
    * Some tools might not take the string literally. For example
    * VS Code when code complete is requested in this example
    * `con<cursor position>` and a completion item with an `insertText` of
    * `console` is provided it will only insert `sole`. Therefore it is
    * recommended to use `textEdit` instead since it avoids additional client
    * side interpretation.

Anyway this is _not_ a regression in your patch: the current state also
does this mistake.  Though, I wonder if you could fix it as well (in a
follow-up commit, maybe).

+(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")))))

Here, we're looking-back to "foo" here because _no_ completion took
place.  This means a *Completions* buffer has appeared and is now
offering foot and footer.

The current state does not do this, and chooses one of the two
completions indiscrimately.  This is, I think, the original bug you
reported here.  So this is clearly a win.  To underline it and defend
against regresison, I suggest to also assert that the *Completions*
buffer popped up.  Though, maybe not going so far as asserting the there
are exactly those two completions in that order -- doing it
"graphically" might prove too brittle.

+(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 foobar;"
+                         "int main() {foo123")))))
+    (with-current-buffer
+        (eglot--find-file-noselect "project/coiso.c")
+      (eglot--wait-for-clangd)
+      (goto-char (- (point-max) 3))
+      (should
+       (equal
+        '("foo123" . 3)
+        (completion-try-completion
+         "foo123"
+         (nth 2 (eglot-completion-at-point)) nil 3))))))

I don't understand the point of this last test.  Are you checking that
nothing changes?  What are you guarding against?

Finally, an interesting test could be the rust-analyzer one from the
issue 1339 issue, which starts with.

fn main() {
    let v: usize = 1;
    1234.1234567890
}

I've manually checked that neither the v.c1234.12345676890 nor the
v.count_on1234.1234567890 case has regressed.

The tests for rust-analyzer require a bit more boilerplate (they require
a "cargo" call to setup a temporary project), but other than that it's
mostly the same.  They won't run on Hydra (no rust-analyzer there)), but
I do run them on occasion.

João

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

[Dmitry Gutov]

On 21/08/2024 19:52, João Távora wrote:

>> See the new test 'eglot-test-try-completion-inside-symbol' inside the
>> patch. Wasn't sure whether to make it use 'completion-try-completion'
>> or 'completion-at-point', but the former seems more explicit.
> 
> This all looks very good, especially the abundant tests.  You may push
> right now if you want.

Thanks! The next question I should ask, though, is can we get it into 
Emacs 30?

Like with the previous discussions about Eglot in Emacs 29, I think 
there will be a lot of users who just use the version of it that comes 
with the release, without upgrading.

> I'll just comment on those:
> 
> +(ert-deftest eglot-test-common-prefix-completion ()
> +  "Test completion appending the common prefix."
> +  (skip-unless (executable-find "clangd"))
> +  (eglot--with-fixture
> +      `(("project" . (("coiso.c" .
> 
> Eh.  You may replace "coiso.c" for the Russian equivalent of "thingy.c"
> if you want :-).  Or something more imaginative.

schtuka? The Portuguese version sounds pretty good to my ear. I'd try 
the Greek variant, but "pragma" already has a different meaning in C langs.

> +                       ,(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")))))
> 
> Here, we completed to foo_bar and it's a fully formed completion.  I
> understand you've checked that exit-function does run.  This is good.

I did check, though not in any of the included tests.

> Unfortunately, if the the two ints were named "foo_bar_1" and
> "foo_bar_2" it would also complete to foo_bar_, which is wrong in the
> general case, since that text is not a fully formed completion.
> exit-function cannot run here.

Is it really a problem? In my testing completing to foo_bar_ usually is 
a good thing (you have fewer chars left to type), even though nothing 
else happens.

To try a different example, if I have C functions foo_bar_1 and 
foo_bar_2, with Clang input 'foo_' does get completed to 'foo_bar_', 
without exit-function running. But to get it called, I only need to 
additionally type '1' and press C-M-i again. Or, if Company is used, 
completing to 'foo_bar_' keeps the popup active, so I just press RET to 
pick the first completion.

> It happens to "work" here because clangd supplies the "insertText" as
> well as the "textEdit" property and that "insertText" isn't meaningless.
> The LSP spec recommendation is for servers to prefer supplying textEdit.
> 
>      * The `insertText` is subject to interpretation by the client side.
>      * Some tools might not take the string literally. For example
>      * VS Code when code complete is requested in this example
>      * `con<cursor position>` and a completion item with an `insertText` of
>      * `console` is provided it will only insert `sole`. Therefore it is
>      * recommended to use `textEdit` instead since it avoids additional client
>      * side interpretation.
> 
> Anyway this is _not_ a regression in your patch: the current state also
> does this mistake.  Though, I wonder if you could fix it as well (in a
> follow-up commit, maybe).

It seems to me that performing *some* completion is better than always 
returning the original input, isn't it? Do you have an example of a 
language server and a scenario where this becomes a problem?

> +(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")))))
> 
> Here, we're looking-back to "foo" here because _no_ completion took
> place.  This means a *Completions* buffer has appeared and is now
> offering foot and footer.

Right.

> The current state does not do this, and chooses one of the two
> completions indiscrimately.  This is, I think, the original bug you
> reported here.  So this is clearly a win.  To underline it and defend
> against regresison, I suggest to also assert that the *Completions*
> buffer popped up.

Sure.

> Though, maybe not going so far as asserting the there
> are exactly those two completions in that order -- doing it
> "graphically" might prove too brittle.

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.

> +(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 foobar;"
> +                         "int main() {foo123")))))
> +    (with-current-buffer
> +        (eglot--find-file-noselect "project/coiso.c")
> +      (eglot--wait-for-clangd)
> +      (goto-char (- (point-max) 3))
> +      (should
> +       (equal
> +        '("foo123" . 3)
> +        (completion-try-completion
> +         "foo123"
> +         (nth 2 (eglot-completion-at-point)) nil 3))))))
> 
> I don't understand the point of this last test.  Are you checking that
> nothing changes?  What are you guarding against?

With the input, I'm describing the actual prefix and suffix that get 
used. It might help someone who comes later to understand this separate 
case. Indeed "nothing happens", it's just that something did happen with 
the previous version of my patch, and we're testing against that.

Might be more interesting to have a similar example where the prefix 
changes while the (non-emtpy) suffix stays the same, but I think we 
aren't going to support this in this c-style.

> Finally, an interesting test could be the rust-analyzer one from the
> issue 1339 issue, which starts with.
> 
> fn main() {
>      let v: usize = 1;
>      1234.1234567890
> }
> 
> I've manually checked that neither the v.c1234.12345676890 nor the
> v.count_on1234.1234567890 case has regressed.

Yep, that's the one I fixed in the last revision.

It's the same case as '("foo123" . 3) in the test above, isn't it?

> The tests for rust-analyzer require a bit more boilerplate (they require
> a "cargo" call to setup a temporary project), but other than that it's
> mostly the same.  They won't run on Hydra (no rust-analyzer there)), but
> I do run them on occasion.

Was a bit non-trivial to get it running indeed.

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

[João Távora]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Like with the previous discussions about Eglot in Emacs 29, I think
> there will be a lot of users who just use the version of it that comes
> with the release, without upgrading.

Hopefully upgrading should be a matter of M-x eglot-upgrade-eglot.

But no problem on my side.  In a separate "don't merge" commit, look
into editing the etc/EGLOT-NEWS there which I've already bumped to
1.17.30 (the version of Eglot to be bundled with Emacs 30).  I'll then
copy that text to master eventually and release 1.17 to GNU Elpa.

>> Unfortunately, if the the two ints were named "foo_bar_1" and
>> "foo_bar_2" it would also complete to foo_bar_, which is wrong in the
>> general case, since that text is not a fully formed completion.
>> exit-function cannot run here.
>
> Is it really a problem?

Yes.  Start 'clangd --completion-style=detailed' in this thingy.cpp C++
file:

   int foobar(int x, char c) {
     return x + c;
   }
    
   int foobar(int x, float f) {
     return x + f;
   }
    
   int main() {
     foob<cursor here>
   }

if you press C-M-i here this will expand to the partial 'foobar(int x, '
which doesn't make C/C++ syntactic sense in that context.  Emacs is just
inserting the LSP "label", which as the name implies, is just for
displaying, not for inserting.  The goal of these two completions is to
expand a snippet and the snippet can only be expanded through the exit
function.  Even if we complexified Eglot's capf to insert the
'insertText' instead of the 'label' (assuming it existed), we'd be
inserting a partial snippet skeleton, even more nonsensical in C/C++.

> completing to 'foo_bar_' keeps the popup active, so I just press RET
> to pick the first completion.

With the above 'foobar(int x, ' example and with bare C-M-i it's even
worse: it's hard (if not impossible) to complete to the full completion
and thus get the desired snippet expansion because the common prefix of
both completion labels ends with a space.

> It seems to me that performing *some* completion is better than always
> returning the original input, isn't it?

No, not always.  LSP just wasn't thought with the "partial completion"
scenario in mind.

> Do you have an example of a language server and a scenario where this
> becomes a problem?

The above.

>> The current state does not do this, and chooses one of the two
>> completions indiscrimately.  This is, I think, the original bug you
>> reported here.  So this is clearly a win.  To underline it and defend
>> against regresison, I suggest to also assert that the *Completions*
>> buffer popped up.
>
> Sure.

Great.

>> Though, maybe not going so far as asserting the there
>> are exactly those two completions in that order -- doing it
>> "graphically" might prove too brittle.
>
> 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.

>> +(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 foobar;"
>> +                         "int main() {foo123")))))
>> +    (with-current-buffer
>> +        (eglot--find-file-noselect "project/coiso.c")
>> +      (eglot--wait-for-clangd)
>> +      (goto-char (- (point-max) 3))
>> +      (should
>> +       (equal
>> +        '("foo123" . 3)
>> +        (completion-try-completion
>> +         "foo123"
>> +         (nth 2 (eglot-completion-at-point)) nil 3))))))
>> I don't understand the point of this last test.  Are you checking
>> that
>> nothing changes?  What are you guarding against?
>
> With the input, I'm describing the actual prefix and suffix that get
> used. It might help someone who comes later to understand this
> separate case. Indeed "nothing happens", it's just that something did
> happen with the previous version of my patch, and we're testing
> against that.

Fine.  So maybe be a little bit more explicit in the comment or
docstring about what you _don't_ want to happen.  AFAIU, you don't want
'foobar123' or 'foobar' to be the result of that completion.

> Might be more interesting to have a similar example where the prefix
> changes while the (non-emtpy) suffix stays the same, but I think we
> aren't going to support this in this c-style.
>
>> Finally, an interesting test could be the rust-analyzer one from the
>> issue 1339 issue, which starts with.
>> fn main() {
>>      let v: usize = 1;
>>      1234.1234567890
>> }
>> I've manually checked that neither the v.c1234.12345676890 nor the
>> v.count_on1234.1234567890 case has regressed.
>
> Yep, that's the one I fixed in the last revision.
>
> 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.  This scenario
wasn't working a while ago and I'd like to protect it against
regression.

João

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

[Dmitry Gutov]

On 22/08/2024 19:59, João Távora wrote:
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>> Like with the previous discussions about Eglot in Emacs 29, I think
>> there will be a lot of users who just use the version of it that comes
>> with the release, without upgrading.
> 
> Hopefully upgrading should be a matter of M-x eglot-upgrade-eglot.
> 
> But no problem on my side.  In a separate "don't merge" commit, look
> into editing the etc/EGLOT-NEWS there which I've already bumped to
> 1.17.30 (the version of Eglot to be bundled with Emacs 30).  I'll then
> copy that text to master eventually and release 1.17 to GNU Elpa.

Great! Bump it to 1.17.60?

>>> Unfortunately, if the the two ints were named "foo_bar_1" and
>>> "foo_bar_2" it would also complete to foo_bar_, which is wrong in the
>>> general case, since that text is not a fully formed completion.
>>> exit-function cannot run here.
>>
>> Is it really a problem?
> 
> Yes.  Start 'clangd --completion-style=detailed' in this thingy.cpp C++
> file:
> 
>     int foobar(int x, char c) {
>       return x + c;
>     }
>      
>     int foobar(int x, float f) {
>       return x + f;
>     }
>      
>     int main() {
>       foob<cursor here>
>     }
> 
> if you press C-M-i here this will expand to the partial 'foobar(int x, '
> which doesn't make C/C++ syntactic sense in that context.  Emacs is just
> inserting the LSP "label", which as the name implies, is just for
> displaying, not for inserting.  The goal of these two completions is to
> expand a snippet and the snippet can only be expanded through the exit
> function.  Even if we complexified Eglot's capf to insert the
> 'insertText' instead of the 'label' (assuming it existed), we'd be
> inserting a partial snippet skeleton, even more nonsensical in C/C++.

Thanks, now I remember: it's 
https://github.com/company-mode/company-mode/issues/1412.

The current proposal doesn't fix it, though it probably makes things a 
bit better (simply because the "common" string is on average computed to 
be shorter). A somewhat complete solution would probably include 
per-language "stop character" map - where we could check the `probe' for 
chars such as "(" (or whatever the language uses) and shorten the return 
value accordingly if found.

It would replace the currently added "string-prefix-p" check, I guess.

>> completing to 'foo_bar_' keeps the popup active, so I just press RET
>> to pick the first completion.
> 
> With the above 'foobar(int x, ' example and with bare C-M-i it's even
> worse: it's hard (if not impossible) to complete to the full completion
> and thus get the desired snippet expansion because the common prefix of
> both completion labels ends with a space.

Yeah, it'd be great to have C-M-i stop before the paren.

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

>>> +(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 foobar;"
>>> +                         "int main() {foo123")))))
>>> +    (with-current-buffer
>>> +        (eglot--find-file-noselect "project/coiso.c")
>>> +      (eglot--wait-for-clangd)
>>> +      (goto-char (- (point-max) 3))
>>> +      (should
>>> +       (equal
>>> +        '("foo123" . 3)
>>> +        (completion-try-completion
>>> +         "foo123"
>>> +         (nth 2 (eglot-completion-at-point)) nil 3))))))
>>> I don't understand the point of this last test.  Are you checking
>>> that
>>> nothing changes?  What are you guarding against?
>>
>> With the input, I'm describing the actual prefix and suffix that get
>> used. It might help someone who comes later to understand this
>> separate case. Indeed "nothing happens", it's just that something did
>> happen with the previous version of my patch, and we're testing
>> against that.
> 
> Fine.  So maybe be a little bit more explicit in the comment or
> docstring about what you _don't_ want to happen.  AFAIU, you don't want
> 'foobar123' or 'foobar' to be the result of that completion.

Right!

>> Might be more interesting to have a similar example where the prefix
>> changes while the (non-emtpy) suffix stays the same, but I think we
>> aren't going to support this in this c-style.
>>
>>> Finally, an interesting test could be the rust-analyzer one from the
>>> issue 1339 issue, which starts with.
>>> fn main() {
>>>       let v: usize = 1;
>>>       1234.1234567890
>>> }
>>> I've manually checked that neither the v.c1234.12345676890 nor the
>>> v.count_on1234.1234567890 case has regressed.
>>
>> Yep, that's the one I fixed in the last revision.
>>
>> 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 guess the difference might come from C-M-i always going through 
try-completion, whereas the Company popup relies on all-completions.

There's a similar difference when using gopls.

But Clangd actually looks special (just filed separate bug#72765).

 > This scenario
 > wasn't working a while ago and I'd like to protect it against
 > regression.

Would we have a test that launches rust-analyzer (and depends on it 
being installed, and a Cargo project initialized)?

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

[João Távora]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 22/08/2024 19:59, João Távora wrote:
>> Dmitry Gutov <dmitry@gutov.dev> writes:
>> 
>>> Like with the previous discussions about Eglot in Emacs 29, I think
>>> there will be a lot of users who just use the version of it that comes
>>> with the release, without upgrading.
>> Hopefully upgrading should be a matter of M-x eglot-upgrade-eglot.
>> But no problem on my side.  In a separate "don't merge" commit, look
>> into editing the etc/EGLOT-NEWS there which I've already bumped to
>> 1.17.30 (the version of Eglot to be bundled with Emacs 30).  I'll then
>> copy that text to master eventually and release 1.17 to GNU Elpa.
>
> 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).

> The current proposal doesn't fix it, though it probably makes things a
> bit better (simply because the "common" string is on average computed
> to be shorter). A somewhat complete solution would probably include
> per-language "stop character" map - where we could check the `probe'
> for chars such as "(" (or whatever the language uses) and shorten the
> return value accordingly if found.
>
> It would replace the currently added "string-prefix-p" check, I guess.

Sounds clever but I really doubt that would work in the general case.

I'm not getting my point across I think.  An LSP label is just a label.
Except for in very primitive uses of LSP, it's not meant to be inserted
or to be used in the computation of prefixes or commonality between
completions.  These primitives uses are deprecated as I hope the LSP
quote from the spec confirmed.

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.

In conclusion, Emacs's partial completion makes no sense in LSP (except
for the aforementioned fringe/deprecated cases).  The correct fix for
this separate issue to disable partial completion: either a full
completion can be inserted or a listing of all applicable completions
should be shown.

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.

>>> completing to 'foo_bar_' keeps the popup active, so I just press RET
>>> to pick the first completion.
>> With the above 'foobar(int x, ' example and with bare C-M-i it's
>> even
>> worse: it's hard (if not impossible) to complete to the full completion
>> and thus get the desired snippet expansion because the common prefix of
>> both completion labels ends with a space.
>
> 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.

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

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

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.

> I guess the difference might come from C-M-i always going through
> try-completion, whereas the Company popup relies on all-completions.
>
> There's a similar difference when using gopls.
>
> But Clangd actually looks special (just filed separate bug#72765).
>
>> This scenario
>> wasn't working a while ago and I'd like to protect it against
>> regression.
>
> Would we have a test that launches rust-analyzer (and depends on it
> being installed, and a Cargo project initialized)?

Yes.  Look in the existing eglot-tests.el: there is already such a test:
eglot-test-rust-analyzer-watches-files. I'd just cargo-cult the lines
until the "cargo init" call.

João

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

[Dmitry Gutov]

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

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.

[-- Attachment #2: eglot--dumb-tryc-more-checks-v3.diff --]
[-- Type: text/x-patch, Size: 7230 bytes --]

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
   "\

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

[João Távora]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 23/08/2024 13:23, João Távora wrote:
>
> 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.

No, it's not.  It's the kind of complexity Eglot isn't after.  It's a
losing game with the growing number of servers who aren't bound by these
rules in any way.

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

If it's easy, then we should definitely do it.

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

Yes, but they always lose.

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

That's more complexity.  And you can't easily know _if_ the LSP label is
or isn't a prefix of the thing selecting the completion would insert
without effectively simulating the completion (which is frequently).

> Alas, this check does not succeed:
>
>   (should (get-buffer-window "*Completions"))

Window management code, likely.

> 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 half-decent, I think.  Please use that.

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

Maybe.

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

Interesting, I thought deleting the 1234 was part of the edit, but the
user of that bug report was after some Emacs specific behaviour.
Anyway, I don't think the test is too brittle, and I think I'd like to
know anyway if something about that breaks anyway.  Maybe you can just
add a comment saying ";; this test might be a bit too brittle, but
interesting nonetheless".

I think you can commit the patch then.  Thanks.
João

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

[Dmitry Gutov]

On 25/08/2024 12:53, João Távora wrote:
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>> On 23/08/2024 13:23, João Távora wrote:
>>
>> 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.
> 
> No, it's not.  It's the kind of complexity Eglot isn't after.  It's a
> losing game with the growing number of servers who aren't bound by these
> rules in any way.
> 
>> 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.
> 
> If it's easy, then we should definitely do it.

The corresponding change probably looks like this:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 844fc634be9..fcf4e586ead 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3147,20 +3147,10 @@ eglot--dumb-flex

  (defun eglot--dumb-allc (pat table pred _point) (funcall table pat 
pred t))
  (defun eglot--dumb-tryc (pat table pred point)
-  (let ((probe (funcall table pat pred nil)))
-    (cond ((eq probe t) t)
-          (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))))))
+  ;; 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))


But I'd prefer to hold off on it until specific issues come up.

>> 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 half-decent, I think.  Please use that.

Okay.

>> 1) Completions are not filtered by suffix, ever.
>> 2) Completing count_ones() does not delete the suffix ("1234"
>> remains).
> 
> Interesting, I thought deleting the 1234 was part of the edit, but the
> user of that bug report was after some Emacs specific behaviour.
> Anyway, I don't think the test is too brittle, and I think I'd like to
> know anyway if something about that breaks anyway.  Maybe you can just
> add a comment saying ";; this test might be a bit too brittle, but
> interesting nonetheless".

Pushed to emacs-30 as two parts: new tests for existing behavior and the 
fix with its own tests (096730510cd, 713069dd7a8).

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

[Dmitry Gutov]

On 25/08/2024 12:53, João Távora wrote:
>>>> 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?
> Maybe.

Okay, I've tracked this one down to Company adhering to what completion 
style is being used.

When no completions match the suffix, completion-all-completions falls 
back to c-style 'emacs22' (if it's in completion-styles anyway). And 
when that style is used, we don't replace the suffix. There is a report 
for vanilla completion in bug#70968.

That seems to cover all related issues I've found so far.

Thanks, and closing.