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

[Dmitry Gutov <dmitry@gutov.dev>]

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