Subject: bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text

From: Dmitry Gutov <[email-masked]>
Date: Sun, 1 Sep 2024 17:28:30 +0300

On 01/09/2024 12:43, João Távora wrote:

>> It seems the difference comes from bug#70968 as well (which came up
>> recently).
> 
> Okay, but that presumed bug has nothing to do with Eglot, AFAICT.

One could argue that the current definition of the style (in Eglot) 
relies on buggy (or suboptimal) behavior in completion-at-point.

>> -(defun eglot--dumb-allc (pat table pred _point) (funcall table pat pred t))
>> +(defun eglot--dumb-allc (pat table pred point)
>> +  (funcall table (substring pat 0 point) pred t))
>> +
>>    (defun eglot--dumb-tryc (pat table pred point)
>>      (let ((probe (funcall table pat pred nil)))
>>        (cond ((eq probe t) t)
>>
>> That fixes the scenario in Company, with seemingly no change with
>> completion-at-point.
> 
> Like in that other recent bug, if you can add some Eglot test that
> demonstrates the bug and then apply this fix and verify that it passes
> the new tests and all the other tests you added recently, I'm fine with
> the change.

Sure.

>> Or if we want 100% compatibility, we can use 'or':
>>
>>     (or
>>      (funcall table pat pred t)
>>      (funcall table (substring pat 0 point) pred t))
> 
> I don't understand what 100% compatibility this refers to, but if it is
> a better change that also passes the aforementioned tests, I'm also fine
> with it.

One patch simply doesn't filter by the suffix, and another first tries 
filtering by prefix+suffix and if nothing matches falls back to 
filtering by prefix only.

>> But in any case this doesn't help with the completion-at-point behavior
>> described at the end of the report (where foo_|bar_2 turns into
>> foo_bar_2bar_2|). If we consider it okay - then the above patch fixes
>> the discrepancy with Company completion, and done. But if we think it a
>> problem, then the fix might be required somewhere in the area of
>>
>>                    (cond (textEdit
>>                           ;; Revert buffer back to state when the edit
>>                           ;; was obtained from server. If a `proxy'
>>
>> After (and if) that is done, we might not need to change the completion
>> style in the end.
> 
> Same criteria as above.

Alas, I have a fix which works for Company but not so well for 
completion-at-point:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 59d9c346424..197e7d9869d 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3353,7 +3353,6 @@ eglot-completion-at-point
                          ;; "foo.b", the LSP edit applies to that
                          ;; state, _not_ the current "foo.bar".
                          (delete-region orig-pos (point))
-                        (insert (substring bounds-string (- orig-pos 
(car bounds))))
                          (eglot--dbind ((TextEdit) range newText) textEdit
                            (pcase-let ((`(,beg . ,end)
                                         (eglot-range-region range)))

It fixes the main scenario with both UIs - but when the suffix is not 
matching, exit-function can delete too much text.

E.g. v.count|123.123456789 turns into v.count_ones()3456789

That's the example from the recently added test.

> What's currently working shall continue
> working.  I would advise generally to be conservative here: the bugs
> you're fixing seem to be somewhat academic edge cases and not reports by
> actual Eglot users.

I agree that this report is not very critical (and so can wait), but I 
don't think I'll be the only person to trigger it. Just hopefully it 
won't happen too often.

> The only thing I'd like to add is the following two notes:
> 
> * before any of this, you showed earlier a way to completely forbid
>    partial completions in Eglot.  That's a good change for reasons we've
>    already discussed and it prevents a number of bugs.  I'd like that
>    change to be commited first (presuming it does what you expect it to).

Said reasons were also more of "academic" nature, right?

That change would be removing a certain bit of functionality from the 
completion UIs, so I'd rather only do that in the face of hard evidence.

> * the rust-analyzer test you added recently -- and which you said was
>    very brittle -- is indeed very brittle: I cannot get it to pass.  We
>    should fix it, or just delete it and do those rust-analyzer tests
>    manually each time we touch this area.

Could you give more details? It is indeed more brittle in theory, but on 
my machine it's passing every time.

No failures from our CIs have been reported either, although that might 
not be saying much.