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

[Dmitry Gutov <dmitry@gutov.dev>]

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