GNU bug report logs - #72765
Eglot + Clangd + Company + non-empty suffix = duplicate text

Previous Next

To reply to this bug, email your comments to 72765 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: bug-gnu-emacs <at> gnu.org
Subject: Eglot + Clangd + Company + non-empty suffix = duplicate text
Date: Fri, 23 Aug 2024 02:07:59 +0300

Here's an example I came upon when testing:


test.c
```
int foo_bar_1;
int foo_bar_2;

int main() {foo_bar|456

```

Point is at |.

If you use completion-at-point, *Completions* buffer pops up, you choose 
one of the options with M-down and M-RET, "_1" is inserted. Good.

But if you use Company, type "_" (or backspace and re-add "r") - a popup 
comes up with "foo_bar_1" and "foo_bar_2", you choose one of the 
options, and the text becomes "foo_bar_1456456", suffix is duplicated.

This only happens with Clang, out of the servers I've tested.

You need a fairly recent Company to reproduce (from master), the 
previous versions simply didn't support completion in the middle of a 
symbol.

To add something that completion-at-point trips over, though: when 
"foo_bar_2" (existing var name) is already inserted, move point to the 
middle of it and press C-M-i:

```
  foo_|bar_2
```

will turn to

```
  foo_bar_2bar_2|
```

Message #8 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate
 text
Date: Thu, 29 Aug 2024 14:34:10 +0300

> Date: Fri, 23 Aug 2024 02:07:59 +0300
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> Here's an example I came upon when testing:
> 
> 
> test.c
> ```
> int foo_bar_1;
> int foo_bar_2;
> 
> int main() {foo_bar|456
> 
> ```
> 
> Point is at |.
> 
> If you use completion-at-point, *Completions* buffer pops up, you choose 
> one of the options with M-down and M-RET, "_1" is inserted. Good.
> 
> But if you use Company, type "_" (or backspace and re-add "r") - a popup 
> comes up with "foo_bar_1" and "foo_bar_2", you choose one of the 
> options, and the text becomes "foo_bar_1456456", suffix is duplicated.
> 
> This only happens with Clang, out of the servers I've tested.
> 
> You need a fairly recent Company to reproduce (from master), the 
> previous versions simply didn't support completion in the middle of a 
> symbol.
> 
> To add something that completion-at-point trips over, though: when 
> "foo_bar_2" (existing var name) is already inserted, move point to the 
> middle of it and press C-M-i:
> 
> ```
>    foo_|bar_2
> ```
> 
> will turn to
> 
> ```
>    foo_bar_2bar_2|
> ```

Is this an Eglot problem or a completion-at-point problem?

Message #11 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sat, 31 Aug 2024 00:23:32 +0300

On 29/08/2024 14:34, Eli Zaretskii wrote:
> Is this an Eglot problem or a completion-at-point problem?

Looks like it belongs to Eglot.

It might be difficult to fix, especially given slight behavior 
differences between servers, but I think it's worth investigating.

Message #14 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>, João Távora
 <joaotavora <at> gmail.com>
Cc: 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sat, 31 Aug 2024 09:47:45 +0300

> Date: Sat, 31 Aug 2024 00:23:32 +0300
> Cc: 72765 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> On 29/08/2024 14:34, Eli Zaretskii wrote:
> > Is this an Eglot problem or a completion-at-point problem?
> 
> Looks like it belongs to Eglot.
> 
> It might be difficult to fix, especially given slight behavior 
> differences between servers, but I think it's worth investigating.

João, do you have any comments or suggestions about this?

Message #17 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sat, 31 Aug 2024 13:03:38 +0100

On Sat, Aug 31, 2024 at 7:47 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > Date: Sat, 31 Aug 2024 00:23:32 +0300
> > Cc: 72765 <at> debbugs.gnu.org
> > From: Dmitry Gutov <dmitry <at> gutov.dev>
> >
> > On 29/08/2024 14:34, Eli Zaretskii wrote:
> > > Is this an Eglot problem or a completion-at-point problem?
> >
> > Looks like it belongs to Eglot.
> >
> > It might be difficult to fix, especially given slight behavior
> > differences between servers, but I think it's worth investigating.
>
> João, do you have any comments or suggestions about this?

Looks like a bug, somewhere. Somehow these new company
versions don't follow exactly the same protocol as completion-at-point.
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.

João

Message #20 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>,
 Eli Zaretskii <eliz <at> gnu.org>
Cc: 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sun, 1 Sep 2024 04:43:06 +0300

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

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. Or if we want 100% compatibility, we can use 'or':

  (or
   (funcall table pat pred t)
   (funcall table (substring pat 0 point) pred t))

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.

Message #23 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sun, 01 Sep 2024 10:43:22 +0100

Dmitry Gutov <dmitry <at> 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

Message #26 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
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.

Message #29 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Tue, 3 Sep 2024 16:20:27 +0300

On 01/09/2024 17:28, Dmitry Gutov wrote:
>> * 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.

Yeah, I see it now - it succeeds in an interactive session and fails in 
batch mode. Not sure it was the same when the patch was committed 
(hopefully not).

Might be due to window configuration being different...

Message #32 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Tue, 3 Sep 2024 14:43:20 +0100

On Tue, Sep 3, 2024 at 2:20 PM Dmitry Gutov <dmitry <at> gutov.dev> wrote:
>
> On 01/09/2024 17:28, Dmitry Gutov wrote:
> >> * 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.
>
> Yeah, I see it now - it succeeds in an interactive session and fails in
> batch mode. Not sure it was the same when the patch was committed
> (hopefully not).
>
> Might be due to window configuration being different...

Yes, I was trying batch mode.  make -C test eglot-tests  or something
similar.  Please fix it or delete it (or disable it).

Message #35 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sun, 8 Sep 2024 05:41:45 +0300

On 03/09/2024 16:43, João Távora wrote:
> On Tue, Sep 3, 2024 at 2:20 PM Dmitry Gutov<dmitry <at> gutov.dev>  wrote:
>> On 01/09/2024 17:28, Dmitry Gutov wrote:
>>>> * 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.
>> Yeah, I see it now - it succeeds in an interactive session and fails in
>> batch mode. Not sure it was the same when the patch was committed
>> (hopefully not).
>>
>> Might be due to window configuration being different...
> Yes, I was trying batch mode.  make -C test eglot-tests  or something
> similar.  Please fix it or delete it (or disable it).

Looking at minibuffer-tests.el, the above might be a solution, but it 
gets me a core dump instead:

diff --git a/test/lisp/progmodes/eglot-tests.el 
b/test/lisp/progmodes/eglot-tests.el
index e0168baee54..fa3b63b38dc 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -711,7 +711,8 @@ eglot-test-rust-completion-exit-function
       (search-forward "v.count_on")
       (let ((minibuffer-message-timeout 0)
             ;; Fail at (ding) if completion fails.
-            (executing-kbd-macro t))
+            (executing-kbd-macro t)
+            (redisplay-skip-initial-frame nil))
         (when (buffer-live-p "*Completions*")
           (kill-buffer "*Completions*"))
         ;; The design is pretty brittle, we'll need to monitor the


Will follow up later if nobody beats me to it (can others reproduce the 
crash?)

Message #38 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Sun, 8 Sep 2024 16:51:37 +0100

[Message part 1 (text/plain, inline)]

On Sun, Sep 8, 2024 at 3:41 AM Dmitry Gutov <dmitry <at> gutov.dev> wrote:
>
> On 03/09/2024 16:43, João Távora wrote:
> > On Tue, Sep 3, 2024 at 2:20 PM Dmitry Gutov<dmitry <at> gutov.dev>  wrote:
> >> On 01/09/2024 17:28, Dmitry Gutov wrote:
> >>>> * 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.
> >> Yeah, I see it now - it succeeds in an interactive session and fails in
> >> batch mode. Not sure it was the same when the patch was committed
> >> (hopefully not).
> >>
> >> Might be due to window configuration being different...
> > Yes, I was trying batch mode.  make -C test eglot-tests  or something
> > similar.  Please fix it or delete it (or disable it).
>
> Looking at minibuffer-tests.el, the above might be a solution, but it
> gets me a core dump instead:
>
> diff --git a/test/lisp/progmodes/eglot-tests.el
> b/test/lisp/progmodes/eglot-tests.el
> index e0168baee54..fa3b63b38dc 100644
> --- a/test/lisp/progmodes/eglot-tests.el
> +++ b/test/lisp/progmodes/eglot-tests.el
> @@ -711,7 +711,8 @@ eglot-test-rust-completion-exit-function
>         (search-forward "v.count_on")
>         (let ((minibuffer-message-timeout 0)
>               ;; Fail at (ding) if completion fails.
> -            (executing-kbd-macro t))
> +            (executing-kbd-macro t)
> +            (redisplay-skip-initial-frame nil))
>           (when (buffer-live-p "*Completions*")
>             (kill-buffer "*Completions*"))
>           ;; The design is pretty brittle, we'll need to monitor the
>
>
> Will follow up later if nobody beats me to it (can others reproduce the
> crash?)

This now aborts (segfault?).  At least something different.

So, for the record, before this patch with the latest emacs-30, I get the
results in failure1.txt and with your last redisplay-skip-initial-frame patch
I get failure2.txt.

I've produced these files with

make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure1.txt
make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure2.txt

$ rust-analyzer --version
rust-analyzer 1 (0f7f68dad2 2024-08-27)

$ src/emacs --version
GNU Emacs 30.0.90
Development version 89c99891b2b3 on emacs-30 branch; build date 2024-09-08.
Copyright (C) 2024 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.

[failure1.txt (text/plain, attachment)]

[failure2.txt (text/plain, attachment)]

Message #41 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Mon, 9 Sep 2024 03:20:01 +0300

[Message part 1 (text/plain, inline)]

On 08/09/2024 18:51, João Távora wrote:
>> Looking at minibuffer-tests.el, the above might be a solution, but it
>> gets me a core dump instead:
>>
>> diff --git a/test/lisp/progmodes/eglot-tests.el
>> b/test/lisp/progmodes/eglot-tests.el
>> index e0168baee54..fa3b63b38dc 100644
>> --- a/test/lisp/progmodes/eglot-tests.el
>> +++ b/test/lisp/progmodes/eglot-tests.el
>> @@ -711,7 +711,8 @@ eglot-test-rust-completion-exit-function
>>          (search-forward "v.count_on")
>>          (let ((minibuffer-message-timeout 0)
>>                ;; Fail at (ding) if completion fails.
>> -            (executing-kbd-macro t))
>> +            (executing-kbd-macro t)
>> +            (redisplay-skip-initial-frame nil))
>>            (when (buffer-live-p "*Completions*")
>>              (kill-buffer "*Completions*"))
>>            ;; The design is pretty brittle, we'll need to monitor the
>>
>>
>> Will follow up later if nobody beats me to it (can others reproduce the
>> crash?)
> This now aborts (segfault?).  At least something different.
> 
> So, for the record, before this patch with the latest emacs-30, I get the
> results in failure1.txt and with your last redisplay-skip-initial-frame patch
> I get failure2.txt.
> 
> I've produced these files with
> 
> make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure1.txt

So it's reproducible. Great!

Could someone look into the segfault? The repro steps are simple:

1) apply the patch above,
2) run 'make -C test eglot-tests' or the longer command above which 
executes just one test from that file.

The backtrace that I managed to generate is attached.

[make_test_backtrace.log (text/x-log, attachment)]

Message #44 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 72765 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Mon, 09 Sep 2024 14:46:09 +0300

> Date: Mon, 9 Sep 2024 03:20:01 +0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> > This now aborts (segfault?).  At least something different.
> > 
> > So, for the record, before this patch with the latest emacs-30, I get the
> > results in failure1.txt and with your last redisplay-skip-initial-frame patch
> > I get failure2.txt.
> > 
> > I've produced these files with
> > 
> > make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure1.txt
> 
> So it's reproducible. Great!
> 
> Could someone look into the segfault? The repro steps are simple:
> 
> 1) apply the patch above,
> 2) run 'make -C test eglot-tests' or the longer command above which 
> executes just one test from that file.
> 
> The backtrace that I managed to generate is attached.

Thanks.  Please try the patch below.

P.S. I'm not at all sure this is the last segfault you will see
because you are playing with fire: you are not supposed to reset
redisplay-skip-initial-frame to nil in batch-mode tests that test
redisplay-related features!

diff --git a/src/xdisp.c b/src/xdisp.c
index bf7d84c..a1319e7 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -22089,7 +22089,8 @@ #define GIVE_UP(X) return 0
 
   /* Window must either use window-based redisplay or be full width.  */
   if (!FRAME_WINDOW_P (f)
-      && (!FRAME_LINE_INS_DEL_OK (f)
+      && (FRAME_INITIAL_P (f)
+	  || !FRAME_LINE_INS_DEL_OK (f)
 	  || !WINDOW_FULL_WIDTH_P (w)))
     GIVE_UP (4);
 

Message #47 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72765 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Tue, 10 Sep 2024 03:58:58 +0300

On 09/09/2024 14:46, Eli Zaretskii wrote:
>> Date: Mon, 9 Sep 2024 03:20:01 +0300
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
>> From: Dmitry Gutov <dmitry <at> gutov.dev>
>>
>>> This now aborts (segfault?).  At least something different.
>>>
>>> So, for the record, before this patch with the latest emacs-30, I get the
>>> results in failure1.txt and with your last redisplay-skip-initial-frame patch
>>> I get failure2.txt.
>>>
>>> I've produced these files with
>>>
>>> make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure1.txt
>>
>> So it's reproducible. Great!
>>
>> Could someone look into the segfault? The repro steps are simple:
>>
>> 1) apply the patch above,
>> 2) run 'make -C test eglot-tests' or the longer command above which
>> executes just one test from that file.
>>
>> The backtrace that I managed to generate is attached.
> 
> Thanks.  Please try the patch below.

Thanks! The patch takes care of the crash AFAICS (no core dump now), but 
alas not of the original test failure.

> P.S. I'm not at all sure this is the last segfault you will see
> because you are playing with fire: you are not supposed to reset
> redisplay-skip-initial-frame to nil in batch-mode tests that test
> redisplay-related features!

Isn't that the main/only purpose of this variable?

That's how the docstring reads to me, and it's also seems why 
minibuffer-test.el uses it.

In any case, this var is neither necessary nor sufficient (see my next 
email), so sorry if that wasted you time. The fix might still be worth 
installing, though.

> diff --git a/src/xdisp.c b/src/xdisp.c
> index bf7d84c..a1319e7 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -22089,7 +22089,8 @@ #define GIVE_UP(X) return 0
>   
>     /* Window must either use window-based redisplay or be full width.  */
>     if (!FRAME_WINDOW_P (f)
> -      && (!FRAME_LINE_INS_DEL_OK (f)
> +      && (FRAME_INITIAL_P (f)
> +	  || !FRAME_LINE_INS_DEL_OK (f)
>   	  || !WINDOW_FULL_WIDTH_P (w)))
>       GIVE_UP (4);
>   

Message #50 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72765 <at> debbugs.gnu.org
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Tue, 10 Sep 2024 04:40:08 +0300

Hi Joao,

On 08/09/2024 18:51, João Távora wrote:
> So, for the record, before this patch with the latest emacs-30, I get the
> results in failure1.txt

I've taken some more looks at the test output.

[eglot-tests] contents of `*EGLOT (cmpl-project/(rust-mode 
rust-ts-mode)) output*':
> [eglot-tests] contents of `nil':
> [eglot-tests] Killing (main.rs), wiping /tmp/eglot--fixture-XCmCqo
> Test eglot-test-rust-completion-exit-function backtrace:
>    set-buffer(#<killed buffer>)  eglot--call-with-fixture((("cmpl-project" ("main.rs" . "fn test() ->Test eglot-test-rust-completion-exit-function condition:
>      (error "Selecting deleted buffer")

This error comes from eglot--call-with-fixture, where one of the last

  (with-current-buffer buffer (buffer-string))

calls results in "Selecting deleted buffer" error. I'm not sure where 
that comes from (for the duration of the log the contents of stdout 
buffer are printed twice, and stderr and events just once -- somehow 
there are two elements inside the 'new-servers' var).

That hid the original problem, which was just that in the bare session 
yasnippet is not available. Not sure what to do about it - 
monkeypatching a replacements does not seem wise - but if we adjust the 
"expected" at the end not to have parens, then the test finally passes. 
It will fail interactively, though, when yasnippet is installed.

I've pushed that fix as 
https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-30&id=818c0cc9a51a1d678749404cdacdf640d6f32d6e 
now.

It makes that test more similar to 
'eglot-test-try-completion-inside-symbol', but testing rust-analyzer 
separately still seems like a plus.

Message #53 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 72765 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Tue, 10 Sep 2024 14:47:42 +0300

> Date: Tue, 10 Sep 2024 03:58:58 +0300
> Cc: joaotavora <at> gmail.com, 72765 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> >> The backtrace that I managed to generate is attached.
> > 
> > Thanks.  Please try the patch below.
> 
> Thanks! The patch takes care of the crash AFAICS (no core dump now), but 
> alas not of the original test failure.

I installed the patch on the emacs-30 branch.

> > P.S. I'm not at all sure this is the last segfault you will see
> > because you are playing with fire: you are not supposed to reset
> > redisplay-skip-initial-frame to nil in batch-mode tests that test
> > redisplay-related features!
> 
> Isn't that the main/only purpose of this variable?

You mean, playing with fire?  Yes, but you need to be aware of that.
This variable was introduced relatively recently for use by
test/src/xdisp-tests.el; anywhere else you are on your own ;-)

Message #56 received at 72765 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72765 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#72765: Eglot + Clangd + Company + non-empty suffix =
 duplicate text
Date: Tue, 10 Sep 2024 16:20:02 +0300

On 10/09/2024 14:47, Eli Zaretskii wrote:
>> Date: Tue, 10 Sep 2024 03:58:58 +0300
>> Cc:joaotavora <at> gmail.com,72765 <at> debbugs.gnu.org
>> From: Dmitry Gutov<dmitry <at> gutov.dev>
>>
>>>> The backtrace that I managed to generate is attached.
>>> Thanks.  Please try the patch below.
>> Thanks! The patch takes care of the crash AFAICS (no core dump now), but
>> alas not of the original test failure.
> I installed the patch on the emacs-30 branch.
> 
>>> P.S. I'm not at all sure this is the last segfault you will see
>>> because you are playing with fire: you are not supposed to reset
>>> redisplay-skip-initial-frame to nil in batch-mode tests that test
>>> redisplay-related features!
>> Isn't that the main/only purpose of this variable?
> You mean, playing with fire?  Yes, but you need to be aware of that.
> This variable was introduced relatively recently for use by
> test/src/xdisp-tests.el; anywhere else you are on your own 😉

Thanks!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.