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

["João Távora" <joaotavora@gmail.com>]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 31/08/2024 15:03, João Távora wrote:
>
>> Eglot aims primarily at that, since it's what's in Emacs proper. But
>> Eglot also aims at supporting Company in particular as fully
>> as possible.
>> 
>> Anyway, I don't have time to investigate this. The :exit-function in
>> eglot.el should be stepped through to understand exactly who's at
>> fault. And I don't think differences between servers matter:
>> clangd is likely following the spec correctly here.
>
> 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.

> When the completion style emacs22 is used, Company doesn't delete the 
> suffix text before inserting completion. Which is an improvement for 
> some other completion sources, but not Eglot, so far.
>
> To to fix this here, we can avoid a fail-over to emacs22 by only 
> matching the prefix in eglot--dumb-allc like this:
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 59d9c346424..20c15584d2d 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -3138,7 +3138,9 @@ eglot--dumb-flex
>                                         nil comp)
>              finally (cl-return comp)))
>
> -(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.

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

> 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.  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.  But same idea: make tests that demonstrate the
bugs, fix those bugs and verify all the existing tests still pass.

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

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

João